Hi, Hrvoje had some problems with to many network packets in the unlimited queues of the crypto task queues.
Removing the queues and just calling the crypto operations directly improves throughput by 20%. See the sys-crypto-dispatch-klock column. http://bluhm.genua.de/perform/results/2021-07-21T06:50:10Z/perform.html http://bluhm.genua.de/perform/results/2021-07-21T06:50:10Z/gnuplot/ipsec.html Even more important the functions inherit the locks and we have much less contention. With task queue: http://bluhm.genua.de/perform/results/2021-07-21T06:50:10Z/2021-07-21T00%3A00%3A00Z/btrace/ssh_perform%40lt13_iperf3_-c10.4.56.36_-P10_-t10-btrace-kstack.0.svg Direct call: http://bluhm.genua.de/perform/results/2021-07-21T06:50:10Z/patch-sys-crypto-dispatch-klock.1/btrace/ssh_perform%40lt13_iperf3_-c10.4.56.36_-P10_-t10-btrace-kstack.0.svg Note that I have software crypto. Softcrypto is so expensive that network operations are no longer visible. Before the latter were spinning for kernel lock. I would be interested in results with AES-NI. Maybe we needed the queues when sleeping hardware drivers were used and softnet was running as soft interrupt. This diff makes IPsec faster, less complex, more reliable, and more MP friendly. There is also a small bugfix. After crypto dispatch error we should goto drop to free the packet. But this error should happen anyway. ok? bluhm Index: crypto/crypto.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/crypto/crypto.c,v retrieving revision 1.84 diff -u -p -r1.84 crypto.c --- crypto/crypto.c 21 Jul 2021 11:11:41 -0000 1.84 +++ crypto/crypto.c 21 Jul 2021 16:58:57 -0000 @@ -387,23 +387,32 @@ crypto_unregister(u_int32_t driverid, in int crypto_dispatch(struct cryptop *crp) { - struct taskq *tq = crypto_taskq; - int error = 0, s; + int error = 0, lock = 1, s; u_int32_t hid; s = splvm(); hid = (crp->crp_sid >> 32) & 0xffffffff; if (hid < crypto_drivers_num) { if (crypto_drivers[hid].cc_flags & CRYPTOCAP_F_MPSAFE) - tq = crypto_taskq_mpsafe; + lock = 0; } splx(s); - if ((crp->crp_flags & CRYPTO_F_NOQUEUE) == 0) { + /* XXXSMP crypto_invoke() is not MP safe */ + lock = 1; + + if (crp->crp_flags & CRYPTO_F_NOQUEUE) { + if (lock) + KERNEL_LOCK(); + error = crypto_invoke(crp); + if (lock) + KERNEL_UNLOCK(); + } else { + struct taskq *tq; + + tq = lock ? crypto_taskq : crypto_taskq_mpsafe; task_set(&crp->crp_task, (void (*))crypto_invoke, crp); task_add(tq, &crp->crp_task); - } else { - error = crypto_invoke(crp); } return error; @@ -424,6 +433,8 @@ crypto_invoke(struct cryptop *crp) if (crp == NULL || crp->crp_callback == NULL) return EINVAL; + KERNEL_ASSERT_LOCKED(); + s = splvm(); if (crp->crp_ndesc < 1 || crypto_drivers == NULL) { crp->crp_etype = EINVAL; @@ -535,11 +546,16 @@ void crypto_done(struct cryptop *crp) { crp->crp_flags |= CRYPTO_F_DONE; + if (crp->crp_flags & CRYPTO_F_NOQUEUE) { /* not from the crypto queue, wakeup the userland process */ crp->crp_callback(crp); } else { + struct taskq *tq; + + tq = (crp->crp_flags & CRYPTO_F_MPSAFE) ? + crypto_taskq_mpsafe : crypto_taskq; task_set(&crp->crp_task, (void (*))crp->crp_callback, crp); - task_add(crypto_taskq, &crp->crp_task); + task_add(tq, &crp->crp_task); } } Index: crypto/cryptodev.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/crypto/cryptodev.h,v retrieving revision 1.73 diff -u -p -r1.73 cryptodev.h --- crypto/cryptodev.h 9 Jul 2021 20:43:28 -0000 1.73 +++ crypto/cryptodev.h 21 Jul 2021 16:58:57 -0000 @@ -171,6 +171,7 @@ struct cryptop { #define CRYPTO_F_IMBUF 0x0001 /* Input/output are mbuf chains, otherwise contig */ #define CRYPTO_F_IOV 0x0002 /* Input/output are uio */ +#define CRYPTO_F_MPSAFE 0x0004 /* Do not use kernel lock for callback */ #define CRYPTO_F_NOQUEUE 0x0008 /* Don't use crypto queue/thread */ #define CRYPTO_F_DONE 0x0010 /* request completed */ Index: netinet/ip_ah.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v retrieving revision 1.151 diff -u -p -r1.151 ip_ah.c --- netinet/ip_ah.c 18 Jul 2021 14:38:20 -0000 1.151 +++ netinet/ip_ah.c 21 Jul 2021 16:58:57 -0000 @@ -684,7 +684,7 @@ ah_input(struct mbuf *m, struct tdb *tdb /* Crypto operation descriptor. */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_input_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -699,9 +699,7 @@ ah_input(struct mbuf *m, struct tdb *tdb memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union)); tc->tc_rpl = tdb->tdb_rpl; - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: @@ -1134,7 +1132,7 @@ ah_output(struct mbuf *m, struct tdb *td /* Crypto operation descriptor. */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_output_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -1148,9 +1146,7 @@ ah_output(struct mbuf *m, struct tdb *td tc->tc_rdomain = tdb->tdb_rdomain; memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union)); - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: Index: netinet/ip_esp.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_esp.c,v retrieving revision 1.169 diff -u -p -r1.169 ip_esp.c --- netinet/ip_esp.c 18 Jul 2021 14:38:20 -0000 1.169 +++ netinet/ip_esp.c 21 Jul 2021 16:58:57 -0000 @@ -498,7 +498,7 @@ esp_input(struct mbuf *m, struct tdb *td /* Crypto operation descriptor */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_input_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -528,9 +528,7 @@ esp_input(struct mbuf *m, struct tdb *td crde->crd_len = m->m_pkthdr.len - (skip + hlen + alen); } - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: @@ -983,7 +981,7 @@ esp_output(struct mbuf *m, struct tdb *t /* Crypto operation descriptor. */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_output_cb; crp->crp_opaque = (caddr_t)tc; @@ -1015,9 +1013,7 @@ esp_output(struct mbuf *m, struct tdb *t crda->crd_len = m->m_pkthdr.len - (skip + alen); } - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: Index: netinet/ip_ipcomp.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipcomp.c,v retrieving revision 1.71 diff -u -p -r1.71 ip_ipcomp.c --- netinet/ip_ipcomp.c 8 Jul 2021 21:07:19 -0000 1.71 +++ netinet/ip_ipcomp.c 21 Jul 2021 16:58:57 -0000 @@ -174,7 +174,7 @@ ipcomp_input(struct mbuf *m, struct tdb /* Crypto operation descriptor */ crp->crp_ilen = m->m_pkthdr.len - (skip + hlen); - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_input_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -188,9 +188,7 @@ ipcomp_input(struct mbuf *m, struct tdb tc->tc_rdomain = tdb->tdb_rdomain; tc->tc_dst = tdb->tdb_dst; - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; } @@ -479,15 +477,13 @@ ipcomp_output(struct mbuf *m, struct tdb /* Crypto operation descriptor */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_output_cb; crp->crp_opaque = (caddr_t)tc; crp->crp_sid = tdb->tdb_cryptoid; - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: Index: netinet/ipsec_input.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v retrieving revision 1.176 diff -u -p -r1.176 ipsec_input.c --- netinet/ipsec_input.c 21 Jul 2021 12:23:32 -0000 1.176 +++ netinet/ipsec_input.c 21 Jul 2021 16:58:57 -0000 @@ -380,21 +380,19 @@ ipsec_input_cb(struct cryptop *crp) struct tdb *tdb = NULL; int clen, error; - KERNEL_ASSERT_LOCKED(); + NET_ASSERT_LOCKED(); if (m == NULL) { DPRINTF("bogus returned buffer from crypto"); ipsecstat_inc(ipsec_crypto); - goto droponly; + goto drop; } - - NET_LOCK(); tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto); if (tdb == NULL) { DPRINTF("TDB is expired while in crypto"); ipsecstat_inc(ipsec_notdb); - goto baddone; + goto drop; } /* Check for crypto errors */ @@ -403,18 +401,16 @@ ipsec_input_cb(struct cryptop *crp) /* Reset the session ID */ if (tdb->tdb_cryptoid != 0) tdb->tdb_cryptoid = crp->crp_sid; - NET_UNLOCK(); error = crypto_dispatch(crp); if (error) { DPRINTF("crypto dispatch error %d", error); - ipsecstat_inc(ipsec_idrops); - tdb->tdb_idrops++; + goto drop; } return; } DPRINTF("crypto error %d", crp->crp_etype); ipsecstat_inc(ipsec_noxform); - goto baddone; + goto drop; } /* Length of data after processing */ @@ -438,16 +434,13 @@ ipsec_input_cb(struct cryptop *crp) __func__, tdb->tdb_sproto); } - NET_UNLOCK(); if (error) { ipsecstat_inc(ipsec_idrops); tdb->tdb_idrops++; } return; - baddone: - NET_UNLOCK(); - droponly: + drop: ipsecstat_inc(ipsec_idrops); if (tdb != NULL) tdb->tdb_idrops++; Index: netinet/ipsec_output.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_output.c,v retrieving revision 1.83 diff -u -p -r1.83 ipsec_output.c --- netinet/ipsec_output.c 21 Jul 2021 11:11:41 -0000 1.83 +++ netinet/ipsec_output.c 21 Jul 2021 16:58:57 -0000 @@ -395,20 +395,19 @@ ipsec_output_cb(struct cryptop *crp) struct tdb *tdb = NULL; int error, ilen, olen; - KERNEL_ASSERT_LOCKED(); + NET_ASSERT_LOCKED(); if (m == NULL) { DPRINTF("bogus returned buffer from crypto"); ipsecstat_inc(ipsec_crypto); - goto droponly; + goto drop; } - NET_LOCK(); tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto); if (tdb == NULL) { DPRINTF("TDB is expired while in crypto"); ipsecstat_inc(ipsec_notdb); - goto baddone; + goto drop; } /* Check for crypto errors. */ @@ -417,18 +416,16 @@ ipsec_output_cb(struct cryptop *crp) /* Reset the session ID */ if (tdb->tdb_cryptoid != 0) tdb->tdb_cryptoid = crp->crp_sid; - NET_UNLOCK(); error = crypto_dispatch(crp); if (error) { DPRINTF("crypto dispatch error %d", error); - ipsecstat_inc(ipsec_odrops); - tdb->tdb_odrops++; + goto drop; } return; } DPRINTF("crypto error %d", crp->crp_etype); ipsecstat_inc(ipsec_noxform); - goto baddone; + goto drop; } olen = crp->crp_olen; @@ -452,16 +449,13 @@ ipsec_output_cb(struct cryptop *crp) __func__, tdb->tdb_sproto); } - NET_UNLOCK(); if (error) { ipsecstat_inc(ipsec_odrops); tdb->tdb_odrops++; } return; - baddone: - NET_UNLOCK(); - droponly: + drop: if (tdb != NULL) tdb->tdb_odrops++; m_freem(m); @@ -484,6 +478,8 @@ ipsp_process_done(struct mbuf *m, struct struct tdb_ident *tdbi; struct m_tag *mtag; int roff, error; + + NET_ASSERT_LOCKED(); tdb->tdb_last_used = gettime();
