tech-crypto archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

(almost working) patch to make opencrypto use mutex/condvar



On Fri, Feb 01, 2008 at 01:34:39PM -0500, Thor Lancelot Simon wrote:
>
> The enclosed doesn't work for me (disappointing, since I wrote it!)
> with the cryptosoft backend, but one of my coworkers swears it works
> fine for him with a hardware driver.  Comments?  This is the first
> code I've tried to convert to our modern synchronization primitives.

It's much better now, but somehow if I run several consumers at once,
e.g.:
        sysctl -w kern.cryptodevallowsoft=0
        openssl speed -engine cryptodev -multi 64 -evp des-ede3-cbc

Most of them end up sleeping on crydev.  I'm not sure how that's
possible, since it didn't happen with the old tsleep/wakeup_one()
code and the condvar code does the analogous operation in the same
place each time.

But it's suitable for playing with.  And if anyone sees where that
cv_signal() is going missing... I owe you one.

Attached, and at http://www.panix.com/~tls/ocf-mtx2.diff

Thor
Index: crypto.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/crypto.c,v
retrieving revision 1.22
diff -u -r1.22 crypto.c
--- crypto.c    1 Feb 2008 04:52:35 -0000       1.22
+++ crypto.c    1 Feb 2008 21:40:55 -0000
@@ -43,8 +43,10 @@
 #include <opencrypto/cryptodev.h>
 #include <opencrypto/xform.h>                  /* XXX for M_XDATA */
 
-  #define splcrypto splnet
-  /* below is kludges to check whats still missing */
+kcondvar_t cryptoret_cv;
+kmutex_t crypto_mtx;
+
+/* below are kludges for residual code wrtitten to FreeBSD interfaces */
   #define SWI_CRYPTO 17
   #define register_swi(lvl, fn)  \
   softint_establish(SOFTINT_NET, (void (*)(void*))fn, NULL)
@@ -89,7 +91,7 @@
  */
 struct pool cryptop_pool;
 struct pool cryptodesc_pool;
-int crypto_pool_initialized = 0;
+struct pool cryptkop_pool;
 
 int    crypto_usercrypto = 1;          /* userland may open /dev/crypto */
 int    crypto_userasymcrypto = 1;      /* userland may do asym crypto reqs */
@@ -178,6 +180,14 @@
 {
        int error;
 
+       mutex_init(&crypto_mtx, MUTEX_DEFAULT, IPL_NET);
+       cv_init(&cryptoret_cv, "crypto_wait");
+       pool_init(&cryptop_pool, sizeof(struct cryptop), 0, 0,  
+                 0, "cryptop", NULL, IPL_NET); 
+       pool_init(&cryptodesc_pool, sizeof(struct cryptodesc), 0, 0,
+                 0, "cryptodesc", NULL, IPL_NET);
+       pool_init(&cryptkop_pool, sizeof(struct cryptkop), 0, 0,
+                 0, "cryptkop", NULL, IPL_NET);
 
        crypto_drivers = malloc(CRYPTO_DRIVERS_INITIAL *
            sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT | M_ZERO);
@@ -225,9 +235,8 @@
        struct cryptoini *cr;
        u_int32_t hid, lid;
        int err = EINVAL;
-       int s;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        if (crypto_drivers == NULL)
                goto done;
@@ -288,7 +297,7 @@
                }
        }
 done:
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -301,9 +310,8 @@
 {
        u_int32_t hid;
        int err = 0;
-       int s;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        if (crypto_drivers == NULL) {
                err = EINVAL;
@@ -337,7 +345,7 @@
                bzero(&crypto_drivers[hid], sizeof(struct cryptocap));
 
 done:
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -349,11 +357,11 @@
 crypto_get_driverid(u_int32_t flags)
 {
        struct cryptocap *newdrv;
-       int i, s;
+       int i;
 
-       crypto_init();
+       crypto_init();          /* XXX oh, this is foul! */
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
        for (i = 0; i < crypto_drivers_num; i++)
                if (crypto_drivers[i].cc_process == NULL &&
                    (crypto_drivers[i].cc_flags & CRYPTOCAP_F_CLEANUP) == 0 &&
@@ -364,7 +372,7 @@
        if (i == crypto_drivers_num) {
                /* Be careful about wrap-around. */
                if (2 * crypto_drivers_num <= crypto_drivers_num) {
-                       splx(s);
+                       mutex_spin_exit(&crypto_mtx);
                        printf("crypto: driver count wraparound!\n");
                        return -1;
                }
@@ -372,7 +380,7 @@
                newdrv = malloc(2 * crypto_drivers_num *
                    sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
                if (newdrv == NULL) {
-                       splx(s);
+                       mutex_spin_exit(&crypto_mtx);
                        printf("crypto: no space to expand driver table!\n");
                        return -1;
                }
@@ -393,7 +401,7 @@
        if (bootverbose)
                printf("crypto: assign driver %u, flags %u\n", i, flags);
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 
        return i;
 }
@@ -415,11 +423,10 @@
     int (*kprocess)(void*, struct cryptkop *, int),
     void *karg)
 {
-       int s;
        struct cryptocap *cap;
        int err;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        cap = crypto_checkdriver(driverid);
        if (cap != NULL &&
@@ -446,7 +453,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -463,9 +470,9 @@
     void *arg)
 {
        struct cryptocap *cap;
-       int s, err;
+       int err;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        cap = crypto_checkdriver(driverid);
        /* NB: algorithms are in the range [1..max] */
@@ -498,7 +505,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -511,11 +518,11 @@
 int
 crypto_unregister(u_int32_t driverid, int alg)
 {
-       int i, err, s;
+       int i, err;
        u_int32_t ses;
        struct cryptocap *cap;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        cap = crypto_checkdriver(driverid);
        if (cap != NULL &&
@@ -544,7 +551,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -554,14 +561,19 @@
  * around so that subsequent calls using those sessions will
  * correctly detect the driver has been unregistered and reroute
  * requests.
+ *
+ * XXX careful.  Don't change this to call crypto_unregister() for each
+ * XXX registered algorithm unless you drop the mutex across the calls;
+ * XXX you can't take it recursively.
  */
 int
 crypto_unregister_all(u_int32_t driverid)
 {
-       int i, err, s = splcrypto();
+       int i, err;
        u_int32_t ses;
        struct cryptocap *cap;
 
+       mutex_spin_enter(&crypto_mtx);
        cap = crypto_checkdriver(driverid);
        if (cap != NULL) {
                for (i = CRYPTO_ALGORITHM_MIN; i <= CRYPTO_ALGORITHM_MAX; i++) {
@@ -581,7 +593,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -593,9 +605,9 @@
 crypto_unblock(u_int32_t driverid, int what)
 {
        struct cryptocap *cap;
-       int needwakeup, err, s;
+       int needwakeup, err;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
        cap = crypto_checkdriver(driverid);
        if (cap != NULL) {
                needwakeup = 0;
@@ -608,12 +620,13 @@
                        cap->cc_kqblocked = 0;
                }
                if (needwakeup) {
+                       mutex_spin_exit(&crypto_mtx);
                        setsoftcrypto(softintr_cookie);
                }
                err = 0;
        } else
                err = EINVAL;
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 
        return err;
 }
@@ -626,9 +639,9 @@
 crypto_dispatch(struct cryptop *crp)
 {
        u_int32_t hid = SESID2HID(crp->crp_sid);
-       int s, result;
+       int result;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        cryptostats.cs_ops++;
 
@@ -645,6 +658,7 @@
                 */
                cap = crypto_checkdriver(hid);
                if (cap && !cap->cc_qblocked) {
+                       mutex_spin_exit(&crypto_mtx);
                        result = crypto_invoke(crp, 0);
                        if (result == ERESTART) {
                                /*
@@ -652,10 +666,12 @@
                                 * driver ``blocked'' for cryptop's and put
                                 * the op on the queue.
                                 */
+                               mutex_spin_enter(&crypto_mtx);
                                crypto_drivers[hid].cc_qblocked = 1;
                                TAILQ_INSERT_HEAD(&crp_q, crp, crp_next);
                                cryptostats.cs_blocks++;
                        }
+                       goto out_released;
                } else {
                        /*
                         * The driver is blocked, just queue the op until
@@ -674,13 +690,17 @@
                 */
                TAILQ_INSERT_TAIL(&crp_q, crp, crp_next);
                if (wasempty) {
+                       mutex_spin_exit(&crypto_mtx);
                        setsoftcrypto(softintr_cookie);
+                       result = 0;
+                       goto out_released;
                }
 
                result = 0;
        }
-       splx(s);
 
+       mutex_spin_exit(&crypto_mtx);
+out_released:
        return result;
 }
 
@@ -692,13 +712,14 @@
 crypto_kdispatch(struct cryptkop *krp)
 {
        struct cryptocap *cap;
-       int s, result;
+       int result;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
        cryptostats.cs_kops++;
 
        cap = crypto_checkdriver(krp->krp_hid);
        if (cap && !cap->cc_kqblocked) {
+               mutex_spin_exit(&crypto_mtx);
                result = crypto_kinvoke(krp, 0);
                if (result == ERESTART) {
                        /*
@@ -706,6 +727,7 @@
                         * driver ``blocked'' for cryptop's and put
                         * the op on the queue.
                         */
+                       mutex_spin_enter(&crypto_mtx);
                        crypto_drivers[krp->krp_hid].cc_kqblocked = 1;
                        TAILQ_INSERT_HEAD(&crp_kq, krp, krp_next);
                        cryptostats.cs_kblocks++;
@@ -718,7 +740,7 @@
                TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next);
                result = 0;
        }
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 
        return result;
 }
@@ -736,7 +758,7 @@
        if (krp == NULL)
                return EINVAL;
        if (krp->krp_callback == NULL) {
-               free(krp, M_XDATA);             /* XXX allocated in cryptodev */
+               pool_put(&cryptkop_pool, krp);
                return EINVAL;
        }
 
@@ -857,20 +879,15 @@
 crypto_freereq(struct cryptop *crp)
 {
        struct cryptodesc *crd;
-       int s;
 
        if (crp == NULL)
                return;
 
-       s = splcrypto();
-
        while ((crd = crp->crp_desc) != NULL) {
                crp->crp_desc = crd->crd_next;
                pool_put(&cryptodesc_pool, crd);
        }
-
        pool_put(&cryptop_pool, crp);
-       splx(s);
 }
 
 /*
@@ -881,29 +898,17 @@
 {
        struct cryptodesc *crd;
        struct cryptop *crp;
-       int s;
-
-       s = splcrypto();
-
-       if (crypto_pool_initialized == 0) {
-               pool_init(&cryptop_pool, sizeof(struct cryptop), 0, 0,
-                   0, "cryptop", NULL, IPL_NET);
-               pool_init(&cryptodesc_pool, sizeof(struct cryptodesc), 0, 0,
-                   0, "cryptodesc", NULL, IPL_NET);
-               crypto_pool_initialized = 1;
-       }
 
        crp = pool_get(&cryptop_pool, 0);
        if (crp == NULL) {
-               splx(s);
                return NULL;
        }
        bzero(crp, sizeof(struct cryptop));
+       cv_init(&crp->crp_cv, "crydev");
 
        while (num--) {
                crd = pool_get(&cryptodesc_pool, 0);
                if (crd == NULL) {
-                       splx(s);
                        crypto_freereq(crp);
                        return NULL;
                }
@@ -913,7 +918,6 @@
                crp->crp_desc = crd;
        }
 
-       splx(s);
        return crp;
 }
 
@@ -934,7 +938,7 @@
         * has done its tsleep().
         */
        {
-               int s, wasempty;
+               int wasempty;
                /*
                 * Normal case; queue the callback for the thread.
                 *
@@ -943,12 +947,12 @@
                 * back to mark operations completed.  Thus we need
                 * to mask both while manipulating the return queue.
                 */
-               s = splcrypto();
+               mutex_spin_enter(&crypto_mtx);
                wasempty = TAILQ_EMPTY(&crp_ret_q);
                TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next);
                if (wasempty)
-                       wakeup_one(&crp_ret_q);
-               splx(s);
+                       cv_signal(&cryptoret_cv);
+               mutex_spin_exit(&crypto_mtx);
        }
 }
 
@@ -958,7 +962,7 @@
 void
 crypto_kdone(struct cryptkop *krp)
 {
-       int s, wasempty;
+       int wasempty;
 
        if (krp->krp_status != 0)
                cryptostats.cs_kerrs++;
@@ -968,21 +972,20 @@
         * back to mark operations completed.  Thus we need
         * to mask both while manipulating the return queue.
         */
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
        wasempty = TAILQ_EMPTY(&crp_ret_kq);
        TAILQ_INSERT_TAIL(&crp_ret_kq, krp, krp_next);
        if (wasempty)
-               wakeup_one(&crp_ret_q);
-       splx(s);
+               cv_signal(&cryptoret_cv);
+       mutex_spin_exit(&crypto_mtx);
 }
 
 int
 crypto_getfeat(int *featp)
 {
        int hid, kalg, feat = 0;
-       int s;
 
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
 
        if (crypto_userasymcrypto == 0)
                goto out;
@@ -1000,7 +1003,7 @@
                                feat |=  1 << kalg;
        }
 out:
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        *featp = feat;
        return (0);
 }
@@ -1014,11 +1017,11 @@
        struct cryptop *crp, *submit;
        struct cryptkop *krp;
        struct cryptocap *cap;
-       int result, hint, s;
+       int result, hint;
 
        printf("crypto softint\n");
        cryptostats.cs_intrs++;
-       s = splcrypto();
+       mutex_spin_enter(&crypto_mtx);
        do {
                /*
                 * Find the first element in the queue that can be
@@ -1059,7 +1062,11 @@
                }
                if (submit != NULL) {
                        TAILQ_REMOVE(&crp_q, submit, crp_next);
+                       mutex_spin_exit(&crypto_mtx);
                        result = crypto_invoke(submit, hint);
+                       /* we must take here as the TAILQ op or kinvoke
+                          may need this mutex below.  sigh. */
+                       mutex_spin_enter(&crypto_mtx);  
                        if (result == ERESTART) {
                                /*
                                 * The driver ran out of resources, mark the
@@ -1089,7 +1096,10 @@
                }
                if (krp != NULL) {
                        TAILQ_REMOVE(&crp_kq, krp, krp_next);
+                       mutex_spin_exit(&crypto_mtx);
                        result = crypto_kinvoke(krp, 0);
+                       /* the next iteration will want the mutex. :-/ */
+                       mutex_spin_enter(&crypto_mtx);
                        if (result == ERESTART) {
                                /*
                                 * The driver ran out of resources, mark the
@@ -1107,7 +1117,7 @@
                        }
                }
        } while (submit != NULL || krp != NULL);
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 }
 
 /*
@@ -1118,9 +1128,15 @@
 {
        struct cryptop *crp;
        struct cryptkop *krp;
-       int s;
 
-       s = splcrypto();
+       /*
+        * We take the mutex here the first time through.  On subsequent
+        * entries to the infinite loop below, we will still have the
+        * mutex from the prior iteration.
+        */
+
+       mutex_spin_enter(&crypto_mtx);
+
        for (;;) {
                crp = TAILQ_FIRST(&crp_ret_q);
                if (crp != NULL)
@@ -1130,7 +1146,7 @@
                        TAILQ_REMOVE(&crp_ret_kq, krp, krp_next);
 
                if (crp != NULL || krp != NULL) {
-                       splx(s);                /* lower ipl for callbacks */
+                       mutex_spin_exit(&crypto_mtx);   /* drop for callbacks */
                        if (crp != NULL) {
 #ifdef CRYPTO_TIMING
                                if (crypto_timing) {
@@ -1149,14 +1165,10 @@
                        }
                        if (krp != NULL)
                                krp->krp_callback(krp);
-                       s  = splcrypto();
+                       mutex_spin_enter(&crypto_mtx);
                } else {
-                       (void) tsleep(&crp_ret_q, PLOCK, "crypto_wait", 0);
+                       cv_wait(&cryptoret_cv, &crypto_mtx);
                        cryptostats.cs_rets++;
                }
        }
 }
-
-
-
-
Index: cryptodev.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptodev.c,v
retrieving revision 1.31
diff -u -r1.31 cryptodev.c
--- cryptodev.c 1 Feb 2008 04:52:35 -0000       1.31
+++ cryptodev.c 1 Feb 2008 21:40:57 -0000
@@ -55,7 +55,6 @@
 #include <opencrypto/cryptodev.h>
 #include <opencrypto/xform.h>
 
-  #define splcrypto splnet
 #ifdef CRYPTO_DEBUG
 #define DPRINTF(a) uprintf a
 #else
@@ -368,7 +367,7 @@
 {
        struct cryptop *crp = NULL;
        struct cryptodesc *crde = NULL, *crda = NULL;
-       int error, s;
+       int error;
 
        if (cop->len > 256*1024-4)
                return (E2BIG);
@@ -474,11 +473,21 @@
                crp->crp_mac=cse->tmp_mac;
        }
 
-       s = splcrypto();        /* NB: only needed with CRYPTO_F_CBIMM */
+       /*
+        * XXX there was a comment here which said that we went to
+        * XXX splcrypto() but needed to only if CRYPTO_F_CBIMM,
+        * XXX disabled on NetBSD since 1.6O due to a race condition.
+        * XXX But crypto_dispatch went to splcrypto() itself!  (And
+        * XXX now takes the crypto_mtx mutex itself).  We do, however,
+        * XXX need to hold the mutex across the call to cv_wait().
+        * XXX     (should we arrange for crypto_dispatch to return to
+        * XXX      us with it held?  it seems quite ugly to do so.)
+        */
        error = crypto_dispatch(crp);
+       mutex_spin_enter(&crypto_mtx);
        if (error == 0 && (crp->crp_flags & CRYPTO_F_DONE) == 0)
-               error = tsleep(crp, PSOCK, "crydev", 0);
-       splx(s);
+               cv_wait(&crp->crp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
+       mutex_spin_exit(&crypto_mtx);
        if (error) {
                goto bail;
        }
@@ -519,7 +528,9 @@
        cse->error = crp->crp_etype;
        if (crp->crp_etype == EAGAIN)
                return crypto_dispatch(crp);
-       wakeup_one(crp);
+       mutex_spin_enter(&crypto_mtx);
+       cv_signal(&crp->crp_cv);
+       mutex_spin_exit(&crypto_mtx);
        return (0);
 }
 
@@ -528,7 +539,9 @@
 {
        struct cryptkop *krp = (struct cryptkop *) op;
 
-       wakeup_one(krp);
+       mutex_spin_enter(&crypto_mtx);
+       cv_signal(&krp->krp_cv);
+       mutex_spin_exit(&crypto_mtx);
        return (0);
 }
 
@@ -594,10 +607,11 @@
                return (EINVAL);
        }
 
-       krp = (struct cryptkop *)malloc(sizeof *krp, M_XDATA, M_WAITOK);
+       krp = pool_get(&cryptkop_pool, PR_WAITOK);
        if (!krp)
                return (ENOMEM);
        bzero(krp, sizeof *krp);
+       cv_init(&krp->krp_cv, "crykdev");
        krp->krp_op = kop->crk_op;
        krp->krp_status = kop->crk_status;
        krp->krp_iparams = kop->crk_iparams;
@@ -621,10 +635,13 @@
 
        error = crypto_kdispatch(krp);
        if (error == 0)
-               error = tsleep(krp, PSOCK, "crydev", 0);
-       if (error)
+               mutex_spin_enter(&crypto_mtx);
+               cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
+               mutex_spin_exit(&crypto_mtx);
+#if 0
+       if (error)                      /* see XXX above -- wait intr? */
                goto fail;
-
+#endif
        if (krp->krp_status != 0) {
                error = krp->krp_status;
                goto fail;
@@ -646,7 +663,7 @@
                        if (krp->krp_param[i].crp_p)
                                FREE(krp->krp_param[i].crp_p, M_XDATA);
                }
-               free(krp, M_XDATA);
+               pool_put(&cryptkop_pool, krp);
        }
        return (error);
 }
Index: cryptodev.h
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptodev.h,v
retrieving revision 1.10
diff -u -r1.10 cryptodev.h
--- cryptodev.h 1 Feb 2008 04:52:35 -0000       1.10
+++ cryptodev.h 1 Feb 2008 21:40:57 -0000
@@ -288,6 +288,7 @@
 
        void *          crp_mac;
        struct timespec crp_tstamp;     /* performance time stamp */
+       kcondvar_t      crp_cv;
 };
 
 #define CRYPTO_BUF_CONTIG      0x0
@@ -312,6 +313,7 @@
        u_int32_t       krp_hid;
        struct crparam  krp_param[CRK_MAXPARAM];        /* kvm */
        int             (*krp_callback)(struct cryptkop *);
+       kcondvar_t      krp_cv;
 };
 
 /* Crypto capabilities structure */
@@ -391,6 +393,17 @@
 extern int crypto_userasymcrypto;      /* userland may do asym crypto reqs */
 extern int crypto_devallowsoft;        /* only use hardware crypto */
 
+/*
+ * Asymmetric operations are allocated in cryptodev.c but can be
+ * freed in crypto.c.
+ */
+extern struct pool     cryptkop_pool;
+
+/*
+ * Mutual exclusion and its unwelcome friends.
+ */
+
+extern kmutex_t        crypto_mtx;
 
 /*
  * initialize the crypto framework subsystem (not the pseudo-device).


Home | Main Index | Thread Index | Old Index