Re: [Ocf-linux-users] patch for OCF
Brought to you by:
david-m
Menu
▾
▴
Re: [Ocf-linux-users] patch for OCF
|
From: David M. <uc...@gm...> - 2013-04-23 11:59:22
|
hwang7 wrote the following:
> Hi david
>
> When using cryptosoft of OCF to do the crypto and run test scripts
> which includes the following commands on SMP platform(we do the test
> on fsl_p4080 which is a SMP processor with 8 cores):
>
> $ openssl speed -evp des -engine cryptodev -elapsed -multi 10
> $ openssl speed -evp des3 -engine cryptodev -elapsed -multi 10
> $ openssl speed -evp aes128 -engine cryptodev -elapsed -multi 10
>
> The system will give out call trace or hang.
>
> The root causes of this issue is:
> The swcr_sessions is not protected properly, and the three
> types of session operation "create session, use session, delete
> session" are also not protected properly.
>
> So:
> 1.add spin lock to protect swcr_sessions
> 2.add state/ref fields to session, use session state to prevent from
> deleting a session when it's in creating or using state, and use spin
> lock to protect the session state.
Thanks for sending in the patch. I did see it the first time you posted it,
I just haven't had time to digest it fully, sorry for not responding
earlier.
Does the fsl_p4080 have any HW acceleration underneath cryptosoft or are
you using purely software acceleration ?
The patch looks pretty good, but it would help me a lot if you could
describe what kind of re-entry/call sequence was causing the main problem.
Thanks,
Davidm
> diff -urbN ocf-linux-20120127/ocf/cryptosoft.c ocf-linux-20120127-1/ocf/cryptosoft.c
> --- ocf-linux-20120127/ocf/cryptosoft.c 2012-01-27 09:04:28.000000000 +0800
> +++ ocf-linux-20120127-1/ocf/cryptosoft.c 2013-01-30 12:41:25.554624164 +0800
> @@ -77,6 +77,11 @@
>
> #define SW_TYPE_INUSE 0x10000000
>
> +#define SW_SESSION_IDLE 0x0
> +#define SW_SESSION_CREATION_IN_PROGRESS 0x1
> +#define SW_SESSION_IN_USE 0x2
> +#define SW_SESSION_DELETION_IN_PROGRESS 0x3
> +
> /* We change some of the above if we have an async interface */
>
> #define SW_TYPE_ALG_AMASK (SW_TYPE_ALG_MASK | SW_TYPE_ASYNC)
> @@ -91,6 +96,8 @@
> struct work_struct workq;
> int sw_type;
> int sw_alg;
> + int sw_state;
> + int sw_ref;
> struct crypto_tfm *sw_tfm;
> spinlock_t sw_tfm_lock;
> union {
> @@ -283,6 +290,25 @@
> MODULE_PARM_DESC(swcr_debug, "Enable debug");
>
> static void swcr_process_req(struct swcr_req *req);
> +static DEFINE_SPINLOCK(session_lock);
> +
> +static inline void _swcr_set_state(struct swcr_data *swd, int state, int lock)
> +{
> + unsigned long flags;
> + if( lock )
> + spin_lock_irqsave(&session_lock, flags);
> +
> + if (swd != NULL)
> + swd->sw_state = state;
> +
> + if ( lock )
> + spin_unlock_irqrestore(&session_lock, flags);
> +}
> +
> +static inline void swcr_set_state(struct swcr_data *swd, int state)
> +{
> + _swcr_set_state(swd, state, 0);
> +}
>
> /*
> * somethings just need to be run with user context no matter whether
> @@ -339,14 +365,17 @@
> swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
> {
> struct swcr_data **swd;
> - u_int32_t i;
> + u_int32_t index, i;
> int error;
> char *algo;
> int mode;
> + unsigned long flags;
>
> + spin_lock_irqsave(&session_lock, flags);
> dprintk("%s()\n", __FUNCTION__);
> if (sid == NULL || cri == NULL) {
> dprintk("%s,%d - EINVAL\n", __FILE__, __LINE__);
> + spin_unlock_irqrestore(&session_lock, flags);
> return EINVAL;
> }
>
> @@ -372,6 +401,7 @@
> else
> swcr_sesnum /= 2;
> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
> + spin_unlock_irqrestore(&session_lock, flags);
> return ENOBUFS;
> }
> memset(swd, 0, swcr_sesnum * sizeof(struct swcr_data *));
> @@ -386,30 +416,39 @@
> swcr_sessions = swd;
> }
>
> - swd = &swcr_sessions[i];
> - *sid = i;
> + index = i;
> + swd = &swcr_sessions[index];
> + *sid = index;
>
> while (cri) {
> *swd = (struct swcr_data *) kmalloc(sizeof(struct swcr_data),
> SLAB_ATOMIC);
> if (*swd == NULL) {
> - swcr_freesession(NULL, i);
> + swcr_set_state(swcr_sessions[index], SW_SESSION_DELETION_IN_PROGRESS);
> + spin_unlock_irqrestore(&session_lock, flags);
> + swcr_freesession(NULL, index);
> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
> return ENOBUFS;
> }
> memset(*swd, 0, sizeof(struct swcr_data));
> + swcr_set_state(*swd, SW_SESSION_CREATION_IN_PROGRESS);
> + spin_unlock_irqrestore(&session_lock, flags);
>
> if (cri->cri_alg < 0 ||
> cri->cri_alg>=sizeof(crypto_details)/sizeof(crypto_details[0])){
> printk("cryptosoft: Unknown algorithm 0x%x\n", cri->cri_alg);
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return EINVAL;
> }
>
> algo = crypto_details[cri->cri_alg].alg_name;
> if (!algo || !*algo) {
> printk("cryptosoft: Unsupported algorithm 0x%x\n", cri->cri_alg);
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return EINVAL;
> }
>
> @@ -450,7 +489,9 @@
> algo,mode);
> err = IS_ERR((*swd)->sw_tfm) ? -(PTR_ERR((*swd)->sw_tfm)) : EINVAL;
> (*swd)->sw_tfm = NULL; /* ensure NULL */
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return err;
> }
>
> @@ -482,7 +523,9 @@
> if (error) {
> printk("cryptosoft: setkey failed %d (crt_flags=0x%x)\n", error,
> (*swd)->sw_tfm->crt_flags);
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return error;
> }
> } else if ((*swd)->sw_type & (SW_TYPE_HMAC | SW_TYPE_HASH)) {
> @@ -504,7 +547,9 @@
> if (!(*swd)->sw_tfm) {
> dprintk("cryptosoft: crypto_alloc_hash failed(%s,0x%x)\n",
> algo, mode);
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return EINVAL;
> }
>
> @@ -512,7 +557,9 @@
> (*swd)->u.hmac.sw_key = (char *)kmalloc((*swd)->u.hmac.sw_klen,
> SLAB_ATOMIC);
> if ((*swd)->u.hmac.sw_key == NULL) {
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
> return ENOBUFS;
> }
> @@ -532,24 +579,33 @@
> if (!(*swd)->sw_tfm) {
> dprintk("cryptosoft: crypto_alloc_comp failed(%s,0x%x)\n",
> algo, mode);
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return EINVAL;
> }
> (*swd)->u.sw_comp_buf = kmalloc(CRYPTO_MAX_DATA_LEN, SLAB_ATOMIC);
> if ((*swd)->u.sw_comp_buf == NULL) {
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> dprintk("%s,%d: ENOBUFS\n", __FILE__, __LINE__);
> return ENOBUFS;
> }
> } else {
> printk("cryptosoft: Unhandled sw_type %d\n", (*swd)->sw_type);
> - swcr_freesession(NULL, i);
> + _swcr_set_state(swcr_sessions[index],
> + SW_SESSION_DELETION_IN_PROGRESS, 1);
> + swcr_freesession(NULL, index);
> return EINVAL;
> }
>
> cri = cri->cri_next;
> swd = &((*swd)->sw_next);
> + spin_lock_irqsave(&session_lock, flags);
> }
> + swcr_set_state(swcr_sessions[index], SW_SESSION_IDLE);
> + spin_unlock_irqrestore(&session_lock, flags);
> return 0;
> }
>
> @@ -559,22 +615,42 @@
> static int
> swcr_freesession(device_t dev, u_int64_t tid)
> {
> + unsigned long flags;
> struct swcr_data *swd;
> u_int32_t sid = CRYPTO_SESID2LID(tid);
>
> + spin_lock_irqsave(&session_lock, flags);
> dprintk("%s()\n", __FUNCTION__);
> if (sid > swcr_sesnum || swcr_sessions == NULL ||
> swcr_sessions[sid] == NULL) {
> dprintk("%s,%d: EINVAL\n", __FILE__, __LINE__);
> + spin_unlock_irqrestore(&session_lock, flags);
> return(EINVAL);
> }
>
> /* Silently accept and return */
> - if (sid == 0)
> + if (sid == 0) {
> + spin_unlock_irqrestore(&session_lock, flags);
> return(0);
> + }
>
> while ((swd = swcr_sessions[sid]) != NULL) {
> + if (swd->sw_state == SW_SESSION_IN_USE
> + || swd->sw_state == SW_SESSION_CREATION_IN_PROGRESS) {
> + spin_unlock_irqrestore(&session_lock, flags);
> + return EAGAIN;
> + }
> + /*
> + * If the session is to be deleted, it should all be deleted
> + * so mark it as unuseable.
> + * In fact, only the first swcr_data's state is used as the
> + * session state now, so just change it to mark the session
> + * as being delete
> + */
> + swcr_set_state(swd, SW_SESSION_DELETION_IN_PROGRESS);
> swcr_sessions[sid] = swd->sw_next;
> + swcr_set_state(swcr_sessions[sid], SW_SESSION_DELETION_IN_PROGRESS);
> + spin_unlock_irqrestore(&session_lock, flags);
> if (swd->sw_tfm) {
> switch (swd->sw_type & SW_TYPE_ALG_AMASK) {
> #ifdef HAVE_AHASH
> @@ -615,16 +691,18 @@
> kfree(swd->u.hmac.sw_key);
> }
> kfree(swd);
> + spin_lock_irqsave(&session_lock, flags);
> }
> + spin_unlock_irqrestore(&session_lock, flags);
> return 0;
> }
>
> static void swcr_process_req_complete(struct swcr_req *req)
> {
> + unsigned long flags;
> dprintk("%s()\n", __FUNCTION__);
>
> if (req->sw->sw_type & SW_TYPE_INUSE) {
> - unsigned long flags;
> spin_lock_irqsave(&req->sw->sw_tfm_lock, flags);
> req->sw->sw_type &= ~SW_TYPE_INUSE;
> spin_unlock_irqrestore(&req->sw->sw_tfm_lock, flags);
> @@ -665,6 +743,12 @@
> }
>
> done:
> + spin_lock_irqsave(&session_lock, flags);
> + req->sw_head->sw_ref--;
> + if (req->sw_head->sw_ref <= 0) {
> + swcr_set_state(req->sw_head, SW_SESSION_IDLE);
> + }
> + spin_unlock_irqrestore(&session_lock, flags);
> dprintk("%s crypto_done %p\n", __FUNCTION__, req);
> crypto_done(req->crp);
> kmem_cache_free(swcr_req_cache, req);
> @@ -1162,6 +1246,7 @@
> {
> struct swcr_req *req = NULL;
> u_int32_t lid;
> + unsigned long flags;
>
> dprintk("%s()\n", __FUNCTION__);
> /* Sanity check */
> @@ -1179,10 +1264,12 @@
> }
>
> lid = crp->crp_sid & 0xffffffff;
> + spin_lock_irqsave(&session_lock, flags);
> if (lid >= swcr_sesnum || lid == 0 || swcr_sessions == NULL ||
> swcr_sessions[lid] == NULL) {
> crp->crp_etype = ENOENT;
> dprintk("%s,%d: ENOENT\n", __FILE__, __LINE__);
> + spin_unlock_irqrestore(&session_lock, flags);
> goto done;
> }
>
> @@ -1195,6 +1282,7 @@
> if (skb_shinfo(skb)->nr_frags >= SCATTERLIST_MAX) {
> printk("%s,%d: %d nr_frags > SCATTERLIST_MAX", __FILE__, __LINE__,
> skb_shinfo(skb)->nr_frags);
> + spin_unlock_irqrestore(&session_lock, flags);
> goto done;
> }
> } else if (crp->crp_flags & CRYPTO_F_IOV) {
> @@ -1202,6 +1290,7 @@
> if (uiop->uio_iovcnt > SCATTERLIST_MAX) {
> printk("%s,%d: %d uio_iovcnt > SCATTERLIST_MAX", __FILE__, __LINE__,
> uiop->uio_iovcnt);
> + spin_unlock_irqrestore(&session_lock, flags);
> goto done;
> }
> }
> @@ -1213,11 +1302,24 @@
> if (req == NULL) {
> dprintk("%s,%d: ENOMEM\n", __FILE__, __LINE__);
> crp->crp_etype = ENOMEM;
> + spin_unlock_irqrestore(&session_lock, flags);
> goto done;
> }
> memset(req, 0, sizeof(*req));
>
> req->sw_head = swcr_sessions[lid];
> + if (req->sw_head->sw_state == SW_SESSION_DELETION_IN_PROGRESS) {
> + crp->crp_etype = EINVAL;
> + spin_unlock_irqrestore(&session_lock, flags);
> + goto done;
> + } else if (req->sw_head->sw_state == SW_SESSION_CREATION_IN_PROGRESS) {
> + crp->crp_etype = EAGAIN;
> + spin_unlock_irqrestore(&session_lock, flags);
> + goto done;
> + }
> + swcr_set_state(req->sw_head, SW_SESSION_IN_USE);
> + req->sw_head->sw_ref++;
> + spin_unlock_irqrestore(&session_lock, flags);
> req->crp = crp;
> req->crd = crp->crp_desc;
>
>
> ------------------------------------------------------------------------------
> Try New Relic Now & We'll Send You this Cool Shirt
> New Relic is the only SaaS-based application performance monitoring service
> that delivers powerful full stack analytics. Optimize and monitor your
> browser, app, & servers with just a few lines of code. Try New Relic
> and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr
> _______________________________________________
> Ocf-linux-users mailing list
> Ocf...@li...
> https://lists.sourceforge.net/lists/listinfo/ocf-linux-users
--
David McCullough, da...@sp..., Ph: 0410 560 763
|
