Optimisation in IPSEC(4):

- Remove contention on ISR during the crypto operation by using rwlock(9).
- Remove a second lookup of the SA in the callback.

Gain on 6 cores CPU with SHA1/AES128 can be up to 30%.

Reviewed by:	vanhu
MFC after:	1 month

7 files changed, 69 insertions, 75 deletions

diff --git a/sys/netipsec/ipsec.h b/sys/netipsec/ipsec.h
index 090255a..65faab8 100644
--- a/sys/netipsec/ipsec.h
+++ b/sys/netipsec/ipsec.h
@@ -123,7 +123,7 @@ struct ipsecrequest {
 
 	struct secasvar *sav;	/* place holder of SA for use */
 	struct secpolicy *sp;	/* back pointer to SP */
-	struct mtx lock;	/* to interlock updates */
+	struct rwlock lock;	/* to interlock updates */
 };
 
 /*
@@ -132,11 +132,15 @@ struct ipsecrequest {
  * hard it is to remove this...
  */
 #define	IPSECREQUEST_LOCK_INIT(_isr) \
-	mtx_init(&(_isr)->lock, "ipsec request", NULL, MTX_DEF | MTX_RECURSE)
-#define	IPSECREQUEST_LOCK(_isr)		mtx_lock(&(_isr)->lock)
-#define	IPSECREQUEST_UNLOCK(_isr)	mtx_unlock(&(_isr)->lock)
-#define	IPSECREQUEST_LOCK_DESTROY(_isr)	mtx_destroy(&(_isr)->lock)
-#define	IPSECREQUEST_LOCK_ASSERT(_isr)	mtx_assert(&(_isr)->lock, MA_OWNED)
+	rw_init_flags(&(_isr)->lock, "ipsec request", RW_RECURSE)
+#define	IPSECREQUEST_LOCK(_isr)		rw_rlock(&(_isr)->lock)
+#define	IPSECREQUEST_UNLOCK(_isr)	rw_runlock(&(_isr)->lock)
+#define	IPSECREQUEST_WLOCK(_isr)	rw_wlock(&(_isr)->lock)
+#define	IPSECREQUEST_WUNLOCK(_isr)	rw_wunlock(&(_isr)->lock)
+#define	IPSECREQUEST_UPGRADE(_isr)	rw_try_upgrade(&(_isr)->lock)
+#define	IPSECREQUEST_DOWNGRADE(_isr)	rw_downgrade(&(_isr)->lock)
+#define	IPSECREQUEST_LOCK_DESTROY(_isr)	rw_destroy(&(_isr)->lock)
+#define	IPSECREQUEST_LOCK_ASSERT(_isr)	rw_assert(&(_isr)->lock, RA_LOCKED)
 
 /* security policy in PCB */
 struct inpcbpolicy {
diff --git a/sys/netipsec/key.c b/sys/netipsec/key.c
index 56942e7..7329539 100644
--- a/sys/netipsec/key.c
+++ b/sys/netipsec/key.c
@@ -809,6 +809,7 @@ key_checkrequest(struct ipsecrequest *isr, const struct secasindex *saidx)
 {
 	u_int level;
 	int error;
+	struct secasvar *sav;
 
 	IPSEC_ASSERT(isr != NULL, ("null isr"));
 	IPSEC_ASSERT(saidx != NULL, ("null saidx"));
@@ -826,45 +827,31 @@ key_checkrequest(struct ipsecrequest *isr, const struct secasindex *saidx)
 
 	/* get current level */
 	level = ipsec_get_reqlevel(isr);
-#if 0
-	/*
-	 * We do allocate new SA only if the state of SA in the holder is
-	 * SADB_SASTATE_DEAD.  The SA for outbound must be the oldest.
-	 */
-	if (isr->sav != NULL) {
-		if (isr->sav->sah == NULL)
-			panic("%s: sah is null.\n", __func__);
-		if (isr->sav == (struct secasvar *)LIST_FIRST(
-			    &isr->sav->sah->savtree[SADB_SASTATE_DEAD])) {
-			KEY_FREESAV(&isr->sav);
-			isr->sav = NULL;
-		}
-	}
-#else
+
 	/*
-	 * we free any SA stashed in the IPsec request because a different
+	 * We check new SA in the IPsec request because a different
 	 * SA may be involved each time this request is checked, either
 	 * because new SAs are being configured, or this request is
 	 * associated with an unconnected datagram socket, or this request
 	 * is associated with a system default policy.
 	 *
-	 * The operation may have negative impact to performance.  We may
-	 * want to check cached SA carefully, rather than picking new SA
-	 * every time.
-	 */
-	if (isr->sav != NULL) {
-		KEY_FREESAV(&isr->sav);
-		isr->sav = NULL;
-	}
-#endif
-
-	/*
-	 * new SA allocation if no SA found.
 	 * key_allocsa_policy should allocate the oldest SA available.
 	 * See key_do_allocsa_policy(), and draft-jenkins-ipsec-rekeying-03.txt.
 	 */
-	if (isr->sav == NULL)
-		isr->sav = key_allocsa_policy(saidx);
+	sav = key_allocsa_policy(saidx);
+	if (sav != isr->sav) {
+		/* SA need to be updated. */
+		if (!IPSECREQUEST_UPGRADE(isr)) {
+			/* Kick everyone off. */
+			IPSECREQUEST_UNLOCK(isr);
+			IPSECREQUEST_WLOCK(isr);
+		}
+		if (isr->sav != NULL)
+			KEY_FREESAV(&isr->sav);
+		isr->sav = sav;
+		IPSECREQUEST_DOWNGRADE(isr);
+	} else if (sav != NULL)
+		KEY_FREESAV(&sav);
 
 	/* When there is SA. */
 	if (isr->sav != NULL) {
@@ -1240,6 +1227,16 @@ key_freesp_so(struct secpolicy **sp)
 	KEY_FREESP(sp);
 }
 
+void
+key_addrefsa(struct secasvar *sav, const char* where, int tag)
+{
+
+	IPSEC_ASSERT(sav != NULL, ("null sav"));
+	IPSEC_ASSERT(sav->refcnt > 0, ("refcount must exist"));
+
+	sa_addref(sav);
+}
+
 /*
  * Must be called after calling key_allocsa().
  * This function is called by key_freesp() to free some SA allocated
diff --git a/sys/netipsec/key.h b/sys/netipsec/key.h
index e85acd1..f246dbc 100644
--- a/sys/netipsec/key.h
+++ b/sys/netipsec/key.h
@@ -76,10 +76,13 @@ extern void _key_freesp(struct secpolicy **, const char*, int);
 
 extern struct secasvar *key_allocsa(union sockaddr_union *, u_int, u_int32_t,
 	const char*, int);
+extern void key_addrefsa(struct secasvar *, const char*, int);
 extern void key_freesav(struct secasvar **, const char*, int);
 
 #define	KEY_ALLOCSA(dst, proto, spi)				\
 	key_allocsa(dst, proto, spi, __FILE__, __LINE__)
+#define	KEY_ADDREFSA(sav)					\
+	key_addrefsa(sav, __FILE__, __LINE__)
 #define	KEY_FREESAV(psav)					\
 	key_freesav(psav, __FILE__, __LINE__)
 
diff --git a/sys/netipsec/xform.h b/sys/netipsec/xform.h
index 47e2cfe..e389cab 100644
--- a/sys/netipsec/xform.h
+++ b/sys/netipsec/xform.h
@@ -75,6 +75,7 @@ struct tdb_crypto {
 	int			tc_protoff;	/* current protocol offset */
 	int			tc_skip;	/* data offset */
 	caddr_t			tc_ptr;		/* associated crypto data */
+	struct secasvar 	*tc_sav;	/* related SA */
 };
 
 struct secasvar;
diff --git a/sys/netipsec/xform_ah.c b/sys/netipsec/xform_ah.c
index 6a2d351..726e025 100644
--- a/sys/netipsec/xform_ah.c
+++ b/sys/netipsec/xform_ah.c
@@ -715,6 +715,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
 	tc->tc_protoff = protoff;
 	tc->tc_skip = skip;
 	tc->tc_ptr = (caddr_t) mtag; /* Save the mtag we've identified. */
+	KEY_ADDREFSA(sav);
+	tc->tc_sav = sav;
 
 	if (mtag == NULL)
 		return crypto_dispatch(crp);
@@ -764,13 +766,8 @@ ah_input_cb(struct cryptop *crp)
 	mtag = (struct m_tag *) tc->tc_ptr;
 	m = (struct mbuf *) crp->crp_buf;
 
-	sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi);
-	if (sav == NULL) {
-		V_ahstat.ahs_notdb++;
-		DPRINTF(("%s: SA expired while in crypto\n", __func__));
-		error = ENOBUFS;		/*XXX*/
-		goto bad;
-	}
+	sav = tc->tc_sav;
+	IPSEC_ASSERT(sav != NULL, ("null SA!"));
 
 	saidx = &sav->sah->saidx;
 	IPSEC_ASSERT(saidx->dst.sa.sa_family == AF_INET ||
@@ -785,7 +782,6 @@ ah_input_cb(struct cryptop *crp)
 			sav->tdb_cryptoid = crp->crp_sid;
 
 		if (crp->crp_etype == EAGAIN) {
-			KEY_FREESAV(&sav);
 			error = crypto_dispatch(crp);
 			return error;
 		}
@@ -1111,6 +1107,8 @@ ah_output(
 
 	/* These are passed as-is to the callback. */
 	tc->tc_isr = isr;
+	KEY_ADDREFSA(sav);
+	tc->tc_sav = sav;
 	tc->tc_spi = sav->spi;
 	tc->tc_dst = sav->sah->saidx.dst;
 	tc->tc_proto = sav->sah->saidx.proto;
@@ -1147,14 +1145,14 @@ ah_output_cb(struct cryptop *crp)
 
 	isr = tc->tc_isr;
 	IPSECREQUEST_LOCK(isr);
-	sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi);
-	if (sav == NULL) {
+	sav = tc->tc_sav;
+	/* With the isr lock released SA pointer can be updated. */
+	if (sav != isr->sav) {
 		V_ahstat.ahs_notdb++;
 		DPRINTF(("%s: SA expired while in crypto\n", __func__));
 		error = ENOBUFS;		/*XXX*/
 		goto bad;
 	}
-	IPSEC_ASSERT(isr->sav == sav, ("SA changed\n"));
 
 	/* Check for crypto errors. */
 	if (crp->crp_etype) {
@@ -1162,7 +1160,6 @@ ah_output_cb(struct cryptop *crp)
 			sav->tdb_cryptoid = crp->crp_sid;
 
 		if (crp->crp_etype == EAGAIN) {
-			KEY_FREESAV(&sav);
 			IPSECREQUEST_UNLOCK(isr);
 			error = crypto_dispatch(crp);
 			return error;
diff --git a/sys/netipsec/xform_esp.c b/sys/netipsec/xform_esp.c
index ddfdbcc..97eeefd 100644
--- a/sys/netipsec/xform_esp.c
+++ b/sys/netipsec/xform_esp.c
@@ -429,6 +429,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
 	tc->tc_proto = sav->sah->saidx.proto;
 	tc->tc_protoff = protoff;
 	tc->tc_skip = skip;
+	KEY_ADDREFSA(sav);
+	tc->tc_sav = sav;
 
 	/* Decryption descriptor */
 	if (espx) {
@@ -490,15 +492,8 @@ esp_input_cb(struct cryptop *crp)
 	mtag = (struct m_tag *) tc->tc_ptr;
 	m = (struct mbuf *) crp->crp_buf;
 
-	sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi);
-	if (sav == NULL) {
-		V_espstat.esps_notdb++;
-		DPRINTF(("%s: SA gone during crypto (SA %s/%08lx proto %u)\n",
-		    __func__, ipsec_address(&tc->tc_dst),
-		    (u_long) ntohl(tc->tc_spi), tc->tc_proto));
-		error = ENOBUFS;		/*XXX*/
-		goto bad;
-	}
+	sav = tc->tc_sav;
+	IPSEC_ASSERT(sav != NULL, ("null SA!"));
 
 	saidx = &sav->sah->saidx;
 	IPSEC_ASSERT(saidx->dst.sa.sa_family == AF_INET ||
@@ -515,7 +510,6 @@ esp_input_cb(struct cryptop *crp)
 			sav->tdb_cryptoid = crp->crp_sid;
 
 		if (crp->crp_etype == EAGAIN) {
-			KEY_FREESAV(&sav);
 			error = crypto_dispatch(crp);
 			return error;
 		}
@@ -883,6 +877,8 @@ esp_output(
 
 	/* Callback parameters */
 	tc->tc_isr = isr;
+	KEY_ADDREFSA(sav);
+	tc->tc_sav = sav;
 	tc->tc_spi = sav->spi;
 	tc->tc_dst = saidx->dst;
 	tc->tc_proto = saidx->proto;
@@ -932,8 +928,9 @@ esp_output_cb(struct cryptop *crp)
 
 	isr = tc->tc_isr;
 	IPSECREQUEST_LOCK(isr);
-	sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi);
-	if (sav == NULL) {
+	sav = tc->tc_sav;
+	/* With the isr lock released SA pointer can be updated. */
+	if (sav != isr->sav) {
 		V_espstat.esps_notdb++;
 		DPRINTF(("%s: SA gone during crypto (SA %s/%08lx proto %u)\n",
 		    __func__, ipsec_address(&tc->tc_dst),
@@ -941,8 +938,6 @@ esp_output_cb(struct cryptop *crp)
 		error = ENOBUFS;		/*XXX*/
 		goto bad;
 	}
-	IPSEC_ASSERT(isr->sav == sav,
-		("SA changed was %p now %p\n", isr->sav, sav));
 
 	/* Check for crypto errors. */
 	if (crp->crp_etype) {
@@ -951,7 +946,6 @@ esp_output_cb(struct cryptop *crp)
 			sav->tdb_cryptoid = crp->crp_sid;
 
 		if (crp->crp_etype == EAGAIN) {
-			KEY_FREESAV(&sav);
 			IPSECREQUEST_UNLOCK(isr);
 			error = crypto_dispatch(crp);
 			return error;
diff --git a/sys/netipsec/xform_ipcomp.c b/sys/netipsec/xform_ipcomp.c
index d528d2a..5b2032a 100644
--- a/sys/netipsec/xform_ipcomp.c
+++ b/sys/netipsec/xform_ipcomp.c
@@ -37,6 +37,7 @@
 #include <sys/mbuf.h>
 #include <sys/lock.h>
 #include <sys/mutex.h>
+#include <sys/rwlock.h>
 #include <sys/socket.h>
 #include <sys/kernel.h>
 #include <sys/protosw.h>
@@ -185,6 +186,8 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
 	tc->tc_proto = sav->sah->saidx.proto;
 	tc->tc_protoff = protoff;
 	tc->tc_skip = skip;
+	KEY_ADDREFSA(sav);
+	tc->tc_sav = sav;
 
 	return crypto_dispatch(crp);
 }
@@ -228,13 +231,8 @@ ipcomp_input_cb(struct cryptop *crp)
 	mtag = (struct mtag *) tc->tc_ptr;
 	m = (struct mbuf *) crp->crp_buf;
 
-	sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi);
-	if (sav == NULL) {
-		V_ipcompstat.ipcomps_notdb++;
-		DPRINTF(("%s: SA expired while in crypto\n", __func__));
-		error = ENOBUFS;		/*XXX*/
-		goto bad;
-	}
+	sav = tc->tc_sav;
+	IPSEC_ASSERT(sav != NULL, ("null SA!"));
 
 	saidx = &sav->sah->saidx;
 	IPSEC_ASSERT(saidx->dst.sa.sa_family == AF_INET ||
@@ -248,7 +246,6 @@ ipcomp_input_cb(struct cryptop *crp)
 			sav->tdb_cryptoid = crp->crp_sid;
 
 		if (crp->crp_etype == EAGAIN) {
-			KEY_FREESAV(&sav);
 			return crypto_dispatch(crp);
 		}
 		V_ipcompstat.ipcomps_noxform++;
@@ -431,6 +428,8 @@ ipcomp_output(
 	}
 
 	tc->tc_isr = isr;
+	KEY_ADDREFSA(sav);
+	tc->tc_sav = sav;
 	tc->tc_spi = sav->spi;
 	tc->tc_dst = sav->sah->saidx.dst;
 	tc->tc_proto = sav->sah->saidx.proto;
@@ -471,14 +470,14 @@ ipcomp_output_cb(struct cryptop *crp)
 
 	isr = tc->tc_isr;
 	IPSECREQUEST_LOCK(isr);
-	sav = KEY_ALLOCSA(&tc->tc_dst, tc->tc_proto, tc->tc_spi);
-	if (sav == NULL) {
+	sav = tc->tc_sav;
+	/* With the isr lock released SA pointer can be updated. */
+	if (sav != isr->sav) {
 		V_ipcompstat.ipcomps_notdb++;
 		DPRINTF(("%s: SA expired while in crypto\n", __func__));
 		error = ENOBUFS;		/*XXX*/
 		goto bad;
 	}
-	IPSEC_ASSERT(isr->sav == sav, ("SA changed\n"));
 
 	/* Check for crypto errors */
 	if (crp->crp_etype) {
@@ -487,7 +486,6 @@ ipcomp_output_cb(struct cryptop *crp)
 			sav->tdb_cryptoid = crp->crp_sid;
 
 		if (crp->crp_etype == EAGAIN) {
-			KEY_FREESAV(&sav);
 			IPSECREQUEST_UNLOCK(isr);
 			return crypto_dispatch(crp);
 		}