From 4acae0ac291bcad05207e5c25a25cd4206919779 Mon Sep 17 00:00:00 2001 From: Pawel Jakub Dawidek Date: Wed, 17 May 2006 18:12:44 +0000 Subject: [PATCH] - Make opencrypto more SMP friendly by dropping the queue lock around crypto_invoke(). This allows to serve multiple crypto requests in parallel and not bached requests are served lock-less. Drivers should not depend on the queue lock beeing held around crypto_invoke() and if they do, that's an error in the driver - it should do its own synchronization. - Don't forget to wakeup the crypto thread when new requests is queued and only if both symmetric and asymmetric queues are empty. - Symmetric requests use sessions and there is no way driver can disappear when there is an active session, so we don't need to check this, but assert this. This is also safe to not use the driver lock in this case. - Assymetric requests don't use sessions, so don't check the driver in crypto_kinvoke(). - Protect assymetric operation with the driver lock, because if there is no symmetric session, driver can disappear. - Don't send assymetric request to the driver if it is marked as blocked. - Add an XXX comment, because I don't think migration to another driver is safe when there are pending requests using freed session. - Remove 'hint' argument from crypto_kinvoke(), as it serves no purpose. - Don't hold the driver lock around kprocess method call, instead use cc_koperations to track number of in-progress requests. - Cleanup register/unregister code a bit. - Other small simplifications and cleanups. Reviewed by: sam --- sys/opencrypto/crypto.c | 284 ++++++++++++++++++------------------- sys/opencrypto/cryptodev.h | 1 + 2 files changed, 140 insertions(+), 145 deletions(-) diff --git a/sys/opencrypto/crypto.c b/sys/opencrypto/crypto.c index 62aa250d8b09..e1c7e99aef40 100644 --- a/sys/opencrypto/crypto.c +++ b/sys/opencrypto/crypto.c @@ -64,6 +64,7 @@ static TAILQ_HEAD(,cryptkop) crp_kq; static struct mtx crypto_q_mtx; #define CRYPTO_Q_LOCK() mtx_lock(&crypto_q_mtx) #define CRYPTO_Q_UNLOCK() mtx_unlock(&crypto_q_mtx) +#define CRYPTO_Q_EMPTY() (TAILQ_EMPTY(&crp_q) && TAILQ_EMPTY(&crp_kq)) /* * There are two queues for processing completed crypto requests; one @@ -98,8 +99,8 @@ static struct proc *cryptoproc; static void crypto_ret_proc(void); static struct proc *cryptoretproc; static void crypto_destroy(void); -static int crypto_invoke(struct cryptop *crp, int hint); -static int crypto_kinvoke(struct cryptkop *krp, int hint); +static int crypto_invoke(struct cryptocap *cap, struct cryptop *crp, int hint); +static int crypto_kinvoke(struct cryptkop *krp); static struct cryptostats cryptostats; SYSCTL_STRUCT(_kern, OID_AUTO, crypto_stats, CTLFLAG_RW, &cryptostats, @@ -328,6 +329,15 @@ crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard) return err; } +static void +crypto_remove(struct cryptocap *cap) +{ + + mtx_assert(&crypto_drivers_mtx, MA_OWNED); + if (cap->cc_sessions == 0 && cap->cc_koperations == 0) + bzero(cap, sizeof(*cap)); +} + /* * Delete an existing session (or a reserved session on an unregistered * driver). @@ -335,6 +345,7 @@ crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard) int crypto_freesession(u_int64_t sid) { + struct cryptocap *cap; u_int32_t hid; int err; @@ -352,24 +363,19 @@ crypto_freesession(u_int64_t sid) err = ENOENT; goto done; } + cap = &crypto_drivers[hid]; - if (crypto_drivers[hid].cc_sessions) - crypto_drivers[hid].cc_sessions--; + if (cap->cc_sessions) + cap->cc_sessions--; /* Call the driver cleanup routine, if available. */ - if (crypto_drivers[hid].cc_freesession) - err = crypto_drivers[hid].cc_freesession( - crypto_drivers[hid].cc_arg, sid); + if (cap->cc_freesession) + err = cap->cc_freesession(cap->cc_arg, sid); else err = 0; - /* - * If this was the last session of a driver marked as invalid, - * make the entry available for reuse. - */ - if ((crypto_drivers[hid].cc_flags & CRYPTOCAP_F_CLEANUP) && - crypto_drivers[hid].cc_sessions == 0) - bzero(&crypto_drivers[hid], sizeof(struct cryptocap)); + if (cap->cc_flags & CRYPTOCAP_F_CLEANUP) + crypto_remove(cap); done: CRYPTO_DRIVER_UNLOCK(); @@ -388,11 +394,12 @@ crypto_get_driverid(u_int32_t flags) CRYPTO_DRIVER_LOCK(); - for (i = 0; i < crypto_drivers_num; i++) + for (i = 0; i < crypto_drivers_num; i++) { if (crypto_drivers[i].cc_process == NULL && - (crypto_drivers[i].cc_flags & CRYPTOCAP_F_CLEANUP) == 0 && - crypto_drivers[i].cc_sessions == 0) + (crypto_drivers[i].cc_flags & CRYPTOCAP_F_CLEANUP) == 0) { break; + } + } /* Out of entries, allocate some more. */ if (i == crypto_drivers_num) { @@ -543,9 +550,9 @@ crypto_register(u_int32_t driverid, int alg, u_int16_t maxoplen, int crypto_unregister(u_int32_t driverid, int alg) { - int i, err; - u_int32_t ses; struct cryptocap *cap; + u_int32_t ses, kops; + int i, err; CRYPTO_DRIVER_LOCK(); @@ -563,13 +570,15 @@ crypto_unregister(u_int32_t driverid, int alg) if (i == CRYPTO_ALGORITHM_MAX + 1) { ses = cap->cc_sessions; - bzero(cap, sizeof(struct cryptocap)); - if (ses != 0) { + kops = cap->cc_koperations; + bzero(cap, sizeof(*cap)); + if (ses != 0 || kops != 0) { /* * If there are pending sessions, just mark as invalid. */ cap->cc_flags |= CRYPTOCAP_F_CLEANUP; cap->cc_sessions = ses; + cap->cc_koperations = kops; } } err = 0; @@ -590,9 +599,9 @@ crypto_unregister(u_int32_t driverid, int alg) int crypto_unregister_all(u_int32_t driverid) { - int i, err; - u_int32_t ses; struct cryptocap *cap; + u_int32_t ses, kops; + int i, err; CRYPTO_DRIVER_LOCK(); @@ -603,13 +612,15 @@ crypto_unregister_all(u_int32_t driverid) cap->cc_max_op_len[i] = 0; } ses = cap->cc_sessions; - bzero(cap, sizeof(struct cryptocap)); - if (ses != 0) { + kops = cap->cc_koperations; + bzero(cap, sizeof(*cap)); + if (ses != 0 || kops != 0) { /* * If there are pending sessions, just mark as invalid. */ cap->cc_flags |= CRYPTOCAP_F_CLEANUP; cap->cc_sessions = ses; + cap->cc_koperations = kops; } err = 0; } else @@ -657,8 +668,9 @@ crypto_unblock(u_int32_t driverid, int what) int crypto_dispatch(struct cryptop *crp) { - u_int32_t hid = CRYPTO_SESID2HID(crp->crp_sid); - int result, wasempty; + struct cryptocap *cap; + u_int32_t hid; + int result; cryptostats.cs_ops++; @@ -667,19 +679,22 @@ crypto_dispatch(struct cryptop *crp) binuptime(&crp->crp_tstamp); #endif - CRYPTO_Q_LOCK(); - wasempty = TAILQ_EMPTY(&crp_q); + hid = CRYPTO_SESID2HID(crp->crp_sid); + if ((crp->crp_flags & CRYPTO_F_BATCH) == 0) { - struct cryptocap *cap; /* * Caller marked the request to be processed * immediately; dispatch it directly to the * driver unless the driver is currently blocked. */ cap = crypto_checkdriver(hid); - if (cap && !cap->cc_qblocked) { - result = crypto_invoke(crp, 0); - if (result == ERESTART) { + /* Driver cannot disappeared when there is an active session. */ + KASSERT(cap != NULL, ("%s: Driver disappeared.", __func__)); + if (!cap->cc_qblocked) { + result = crypto_invoke(cap, crp, 0); + if (result != ERESTART) + return (result); + else { /* * The driver ran out of resources, mark the * driver ``blocked'' for cryptop's and put @@ -689,34 +704,17 @@ crypto_dispatch(struct cryptop *crp) * order is preserved but this can place them * behind batch'd ops. */ - crypto_drivers[hid].cc_qblocked = 1; - TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); + cap->cc_qblocked = 1; cryptostats.cs_blocks++; - result = 0; } - } else { - /* - * The driver is blocked, just queue the op until - * it unblocks and the kernel thread gets kicked. - */ - TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); - result = 0; } - } else { - /* - * Caller marked the request as ``ok to delay''; - * queue it for the dispatch thread. This is desirable - * when the operation is low priority and/or suitable - * for batching. - */ - TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); - result = 0; } - if (wasempty && !TAILQ_EMPTY(&crp_q)) + CRYPTO_Q_LOCK(); + if (CRYPTO_Q_EMPTY()) wakeup_one(&crp_q); + TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); CRYPTO_Q_UNLOCK(); - - return result; + return 0; } /* @@ -726,81 +724,73 @@ crypto_dispatch(struct cryptop *crp) int crypto_kdispatch(struct cryptkop *krp) { - struct cryptocap *cap; - int result, wasempty; + int result; cryptostats.cs_kops++; + result = crypto_kinvoke(krp); + if (result != ERESTART) + return (result); CRYPTO_Q_LOCK(); - wasempty = TAILQ_EMPTY(&crp_q); - cap = crypto_checkdriver(krp->krp_hid); - if (cap && !cap->cc_kqblocked) { - result = crypto_kinvoke(krp, 0); - if (result == ERESTART) { - /* - * The driver ran out of resources, mark the - * driver ``blocked'' for cryptkop's and put - * the request back in the queue. It would - * best to put the request back where we got - * it but that's hard so for now we put it - * at the front. This should be ok; putting - * it at the end does not work. - */ - crypto_drivers[krp->krp_hid].cc_kqblocked = 1; - TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); - cryptostats.cs_kblocks++; - } - } else { - /* - * The driver is blocked, just queue the op until - * it unblocks and the kernel thread gets kicked. - */ - TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); - result = 0; - } - if (wasempty && !TAILQ_EMPTY(&crp_kq)) + if (CRYPTO_Q_EMPTY()) wakeup_one(&crp_q); + TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); CRYPTO_Q_UNLOCK(); - return result; + return 0; } /* * Dispatch an assymetric crypto request to the appropriate crypto devices. */ static int -crypto_kinvoke(struct cryptkop *krp, int hint) +crypto_kinvoke(struct cryptkop *krp) { + struct cryptocap *cap = NULL; u_int32_t hid; - int error; + int error = 0; - mtx_assert(&crypto_q_mtx, MA_OWNED); - - /* Sanity checks. */ - if (krp == NULL) - return EINVAL; - if (krp->krp_callback == NULL) { - free(krp, M_XDATA); /* XXX allocated in cryptodev */ - return EINVAL; - } + KASSERT(krp != NULL, ("%s: krp == NULL", __func__)); + KASSERT(krp->krp_callback != NULL, + ("%s: krp->crp_callback == NULL", __func__)); + CRYPTO_DRIVER_LOCK(); for (hid = 0; hid < crypto_drivers_num; hid++) { - if ((crypto_drivers[hid].cc_flags & CRYPTOCAP_F_SOFTWARE) && - !crypto_devallowsoft) + cap = &crypto_drivers[hid]; + if (cap == NULL) continue; - if (crypto_drivers[hid].cc_kprocess == NULL) + if ((cap->cc_flags & CRYPTOCAP_F_SOFTWARE) && + !crypto_devallowsoft) { continue; - if ((crypto_drivers[hid].cc_kalg[krp->krp_op] & - CRYPTO_ALG_FLAG_SUPPORTED) == 0) + } + if (cap->cc_kprocess == NULL) continue; + if (!(cap->cc_kalg[krp->krp_op] & CRYPTO_ALG_FLAG_SUPPORTED)) + continue; + if (cap->cc_kqblocked) { + error = ERESTART; + continue; + } + error = 0; break; } + krp->krp_hid = hid; if (hid < crypto_drivers_num) { - krp->krp_hid = hid; - error = crypto_drivers[hid].cc_kprocess( - crypto_drivers[hid].cc_karg, krp, hint); - } else + cap->cc_koperations++; + CRYPTO_DRIVER_UNLOCK(); + error = cap->cc_kprocess(cap->cc_karg, krp, 0); + CRYPTO_DRIVER_LOCK(); + if (error == ERESTART) { + cap->cc_koperations--; + cap->cc_kqblocked = 1; + CRYPTO_DRIVER_UNLOCK(); + cryptostats.cs_kblocks++; + return (error); + } + } else { error = ENODEV; + } + CRYPTO_DRIVER_UNLOCK(); if (error) { krp->krp_status = error; @@ -839,45 +829,31 @@ crypto_tstat(struct cryptotstat *ts, struct bintime *bt) * Dispatch a crypto request to the appropriate crypto devices. */ static int -crypto_invoke(struct cryptop *crp, int hint) +crypto_invoke(struct cryptocap *cap, struct cryptop *crp, int hint) { - u_int32_t hid; - int (*process)(void*, struct cryptop *, int); + + KASSERT(crp != NULL, ("%s: crp == NULL", __func__)); + KASSERT(crp->crp_callback != NULL, + ("%s: crp->crp_callback == NULL", __func__)); + KASSERT(crp->crp_desc != NULL, ("%s: crp->crp_desc == NULL", __func__)); #ifdef CRYPTO_TIMING if (crypto_timing) crypto_tstat(&cryptostats.cs_invoke, &crp->crp_tstamp); #endif - /* Sanity checks. */ - if (crp == NULL) - return EINVAL; - if (crp->crp_callback == NULL) { - crypto_freereq(crp); - return EINVAL; - } - if (crp->crp_desc == NULL) { - crp->crp_etype = EINVAL; - crypto_done(crp); - return 0; - } - - hid = CRYPTO_SESID2HID(crp->crp_sid); - if (hid < crypto_drivers_num) { - if (crypto_drivers[hid].cc_flags & CRYPTOCAP_F_CLEANUP) - crypto_freesession(crp->crp_sid); - process = crypto_drivers[hid].cc_process; - } else { - process = NULL; - } - - if (process == NULL) { + if (cap->cc_flags & CRYPTOCAP_F_CLEANUP) { struct cryptodesc *crd; u_int64_t nid; /* * Driver has unregistered; migrate the session and return * an error to the caller so they'll resubmit the op. + * + * XXX: What if there are more already queued requests for this + * session? */ + crypto_freesession(crp->crp_sid); + for (crd = crp->crp_desc; crd->crd_next; crd = crd->crd_next) crd->CRD_INI.cri_next = &(crd->crd_next->CRD_INI); @@ -891,7 +867,7 @@ crypto_invoke(struct cryptop *crp, int hint) /* * Invoke the driver to process the request. */ - return (*process)(crypto_drivers[hid].cc_arg, crp, hint); + return cap->cc_process(cap->cc_arg, crp, hint); } } @@ -984,16 +960,13 @@ crypto_done(struct cryptop *crp) #endif crp->crp_callback(crp); } else { - int wasempty; /* * Normal case; queue the callback for the thread. */ CRYPTO_RETQ_LOCK(); - wasempty = TAILQ_EMPTY(&crp_ret_q); - TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next); - - if (wasempty) + if (TAILQ_EMPTY(&crp_ret_q)) wakeup_one(&crp_ret_q); /* shared wait channel */ + TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next); CRYPTO_RETQ_UNLOCK(); } } @@ -1004,16 +977,24 @@ crypto_done(struct cryptop *crp) void crypto_kdone(struct cryptkop *krp) { - int wasempty; + struct cryptocap *cap; if (krp->krp_status != 0) cryptostats.cs_kerrs++; + CRYPTO_DRIVER_LOCK(); + /* XXX: What if driver is loaded in the meantime? */ + if (krp->krp_hid < crypto_drivers_num) { + cap = &crypto_drivers[krp->krp_hid]; + cap->cc_koperations--; + KASSERT(cap->cc_koperations >= 0, ("cc_koperations < 0")); + if (cap->cc_flags & CRYPTOCAP_F_CLEANUP) + crypto_remove(cap); + } + CRYPTO_DRIVER_UNLOCK(); CRYPTO_RETQ_LOCK(); - wasempty = TAILQ_EMPTY(&crp_ret_kq); - TAILQ_INSERT_TAIL(&crp_ret_kq, krp, krp_next); - - if (wasempty) + if (TAILQ_EMPTY(&crp_ret_kq)) wakeup_one(&crp_ret_q); /* shared wait channel */ + TAILQ_INSERT_TAIL(&crp_ret_kq, krp, krp_next); CRYPTO_RETQ_UNLOCK(); } @@ -1071,6 +1052,7 @@ crypto_proc(void) struct cryptop *crp, *submit; struct cryptkop *krp; struct cryptocap *cap; + u_int32_t hid; int result, hint; CRYPTO_Q_LOCK(); @@ -1083,8 +1065,14 @@ crypto_proc(void) submit = NULL; hint = 0; TAILQ_FOREACH(crp, &crp_q, crp_next) { - u_int32_t hid = CRYPTO_SESID2HID(crp->crp_sid); + hid = CRYPTO_SESID2HID(crp->crp_sid); cap = crypto_checkdriver(hid); + /* + * Driver cannot disappeared when there is an active + * session. + */ + KASSERT(cap != NULL, ("%s: Driver disappeared.", + __func__)); if (cap == NULL || cap->cc_process == NULL) { /* Op needs to be migrated, process it. */ if (submit == NULL) @@ -1114,7 +1102,11 @@ crypto_proc(void) } if (submit != NULL) { TAILQ_REMOVE(&crp_q, submit, crp_next); - result = crypto_invoke(submit, hint); + CRYPTO_Q_UNLOCK(); + hid = CRYPTO_SESID2HID(submit->crp_sid); + cap = crypto_checkdriver(hid); + result = crypto_invoke(cap, submit, hint); + CRYPTO_Q_LOCK(); if (result == ERESTART) { /* * The driver ran out of resources, mark the @@ -1144,7 +1136,9 @@ crypto_proc(void) } if (krp != NULL) { TAILQ_REMOVE(&crp_kq, krp, krp_next); - result = crypto_kinvoke(krp, 0); + CRYPTO_Q_UNLOCK(); + result = crypto_kinvoke(krp); + CRYPTO_Q_LOCK(); if (result == ERESTART) { /* * The driver ran out of resources, mark the diff --git a/sys/opencrypto/cryptodev.h b/sys/opencrypto/cryptodev.h index 6791b3554956..b1999d736be4 100644 --- a/sys/opencrypto/cryptodev.h +++ b/sys/opencrypto/cryptodev.h @@ -303,6 +303,7 @@ struct cryptkop { /* Crypto capabilities structure */ struct cryptocap { u_int32_t cc_sessions; + u_int32_t cc_koperations; /* * Largest possible operator length (in bits) for each type of