tech-kern 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