Skip to contentSkip to conversation

pfkey_add: Optimize SA adds and algorithm probes

PATCH Applied Mark Huth 3 participants 8 messages May 11, 2007 – May 25, 2007

Subsystems (3)
lore
Share & tools
lore

Mark Huth

author
#0 lore raw

PATCH 1/1 pfkey_add: Optimize SA adds and algorithm probes

Sorry about previous html/non-inline version which escaped.

This patch provides a performance optimization in the pfkey_add path. Prior versions have a serious performance problem when adding a large number of SAs to a node. For example, if a backup node needs to be loaded with the SAs previously held by a failed active node, thousands of SAs may need to be added as rapidly as possible. Tests show that without this patch, such additions may take several minutes. The cause is that the available algorithm modules are probed each time instead of only when needed. This patch changes the unconditional call to xfrm_probe_algs() to only be done when it may be needed.

An example that loads 2000 SAs gives the following results:

Without patch

real 0m42.643s
user 0m0.120s
sys 0m0.800s

With patch

real 0m0.537s
user 0m0.076s
sys 0m0.276s

===============================================

net/key/af_key.c +0 -0
index a994441..c970d5e 100644
@@ -1445,16 +1445,15 @@ static int key_notify_sa(struct xfrm_state *x, struct km_event *c)
 static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct 

sadb_msg *hdr, void **ext_hdrs)
{
struct xfrm_state *x;
- int err;
+ int err, probe_done = 0;
struct km_event c;

- xfrm_probe_algs();
-
x = pfkey_msg2xfrm_state(hdr, ext_hdrs);
if (IS_ERR(x))
return PTR_ERR(x);

xfrm_state_hold(x);
+try_again:
if (hdr->sadb_msg_type == SADB_ADD)
err = xfrm_state_add(x);
else
@@ -1464,6 +1463,11 @@ static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr,
AUDIT_MAC_IPSEC_ADDSA, err ? 0 : 1, NULL, x);

if (err < 0) {
+ if (!probe_done) {
+ xfrm_probe_algs();
+ probe_done = 1;
+ goto try_again;
+ }
x->km.state = XFRM_STATE_DEAD;
__xfrm_state_put(x);
goto out;

maintainer
#1 lore raw
Quoted 10 lines · Mark Huth

This patch provides a performance optimization in the pfkey_add path. Prior versions have a serious performance problem when adding a large number of SAs to a node. For example, if a backup node needs to be loaded with the SAs previously held by a failed active node, thousands of SAs may need to be added as rapidly as possible. Tests show that without this patch, such additions may take several minutes. The cause is that the available algorithm modules are probed each time instead of only when needed. This patch changes the unconditional call to xfrm_probe_algs() to only be done when it may be needed.

Thanks for the patch!

Review

Quoted 13 lines

static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct
sadb_msg *hdr, void **ext_hdrs)

 {
       struct xfrm_state *x;
(quoted diff) +1 -3
-       int err;
+       int err, probe_done = 0;
       struct km_event c;
 
-       xfrm_probe_algs();
-
       x = pfkey_msg2xfrm_state(hdr, ext_hdrs);
       if (IS_ERR(x))
               return PTR_ERR(x);

I don't think it works when then algorithm isn't loaded though :) If the algorithm isn't present pfkey_msg2xfrm_state will return -ENOSYS so we need to do the probe here.

Actually, I think we should just probe for the specific algorithm requested rather than everything. See patch below.

[IPSEC] pfkey: Load specific algorithm in pfkey_add rather than all

This is a natural extension of the changeset

[XFRM]: Probe selected algorithm only.

which only removed the probe call for xfrm_user. This patch does exactly the same thing for af_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states in af_key.

Cheers,

Signature · 5 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
net/xfrm/xfrm_algo.c +75 -65
index 6249a94..8a72def 100644
@@ -347,67 +347,44 @@ static inline int calg_entries(void)
 	return ARRAY_SIZE(calg_list);
 }
 
-/* Todo: generic iterators */
-struct xfrm_algo_desc *xfrm_aalg_get_byid(int alg_id)
-{
-	int i;
-
-	for (i = 0; i < aalg_entries(); i++) {
-		if (aalg_list[i].desc.sadb_alg_id == alg_id) {
-			if (aalg_list[i].available)
-				return &aalg_list[i];
-			else
-				break;
-		}
-	}
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(xfrm_aalg_get_byid);
-
-struct xfrm_algo_desc *xfrm_ealg_get_byid(int alg_id)
-{
-	int i;
+struct xfrm_algo_list {
+	struct xfrm_algo_desc *algs;
+	int entries;
+	u32 type;
+	u32 mask;
+};
 
-	for (i = 0; i < ealg_entries(); i++) {
-		if (ealg_list[i].desc.sadb_alg_id == alg_id) {
-			if (ealg_list[i].available)
-				return &ealg_list[i];
-			else
-				break;
-		}
-	}
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(xfrm_ealg_get_byid);
+static const struct xfrm_algo_list xfrm_aalg_list = {
+	.algs = aalg_list,
+	.entries = ARRAY_SIZE(aalg_list),
+	.type = CRYPTO_ALG_TYPE_HASH,
+	.mask = CRYPTO_ALG_TYPE_HASH_MASK | CRYPTO_ALG_ASYNC,
+};
 
-struct xfrm_algo_desc *xfrm_calg_get_byid(int alg_id)
-{
-	int i;
+static const struct xfrm_algo_list xfrm_ealg_list = {
+	.algs = ealg_list,
+	.entries = ARRAY_SIZE(ealg_list),
+	.type = CRYPTO_ALG_TYPE_BLKCIPHER,
+	.mask = CRYPTO_ALG_TYPE_MASK | CRYPTO_ALG_ASYNC,
+};
 
-	for (i = 0; i < calg_entries(); i++) {
-		if (calg_list[i].desc.sadb_alg_id == alg_id) {
-			if (calg_list[i].available)
-				return &calg_list[i];
-			else
-				break;
-		}
-	}
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(xfrm_calg_get_byid);
+static const struct xfrm_algo_list xfrm_calg_list = {
+	.algs = calg_list,
+	.entries = ARRAY_SIZE(calg_list),
+	.type = CRYPTO_ALG_TYPE_COMPRESS,
+	.mask = CRYPTO_ALG_TYPE_MASK | CRYPTO_ALG_ASYNC,
+};
 
-static struct xfrm_algo_desc *xfrm_get_byname(struct xfrm_algo_desc *list,
-					      int entries, u32 type, u32 mask,
-					      char *name, int probe)
+static struct xfrm_algo_desc *xfrm_find_algo(
+	const struct xfrm_algo_list *algo_list,
+	int match(const struct xfrm_algo_desc *entry, const void *data),
+	const void *data, int probe)
 {
+	struct xfrm_algo_desc *list = algo_list->algs;
 	int i, status;
 
-	if (!name)
-		return NULL;
-
-	for (i = 0; i < entries; i++) {
-		if (strcmp(name, list[i].name) &&
-		    (!list[i].compat || strcmp(name, list[i].compat)))
+	for (i = 0; i < algo_list->entries; i++) {
+		if (!match(list + i, data))
 			continue;
 
 		if (list[i].available)
@@ -416,8 +393,8 @@ static struct xfrm_algo_desc *xfrm_get_byname(struct xfrm_algo_desc *list,
 		if (!probe)
 			break;
 
-		status = crypto_has_alg(list[i].name, type,
-					mask | CRYPTO_ALG_ASYNC);
+		status = crypto_has_alg(list[i].name, algo_list->type,
+					algo_list->mask);
 		if (!status)
 			break;
 
@@ -427,27 +404,60 @@ static struct xfrm_algo_desc *xfrm_get_byname(struct xfrm_algo_desc *list,
 	return NULL;
 }
 
+static int xfrm_alg_id_match(const struct xfrm_algo_desc *entry,
+			     const void *data)
+{
+	return entry->desc.sadb_alg_id == (int)data;
+}
+
+struct xfrm_algo_desc *xfrm_aalg_get_byid(int alg_id)
+{
+	return xfrm_find_algo(&xfrm_aalg_list, xfrm_alg_id_match,
+			      (void *)alg_id, 1);
+}
+EXPORT_SYMBOL_GPL(xfrm_aalg_get_byid);
+
+struct xfrm_algo_desc *xfrm_ealg_get_byid(int alg_id)
+{
+	return xfrm_find_algo(&xfrm_ealg_list, xfrm_alg_id_match,
+			      (void *)alg_id, 1);
+}
+EXPORT_SYMBOL_GPL(xfrm_ealg_get_byid);
+
+struct xfrm_algo_desc *xfrm_calg_get_byid(int alg_id)
+{
+	return xfrm_find_algo(&xfrm_calg_list, xfrm_alg_id_match,
+			      (void *)alg_id, 1);
+}
+EXPORT_SYMBOL_GPL(xfrm_calg_get_byid);
+
+static int xfrm_alg_name_match(const struct xfrm_algo_desc *entry,
+			       const void *data)
+{
+	const char *name = data;
+
+	return name && (!strcmp(name, entry->name) ||
+			(entry->compat && !strcmp(name, entry->compat)));
+}
+
 struct xfrm_algo_desc *xfrm_aalg_get_byname(char *name, int probe)
 {
-	return xfrm_get_byname(aalg_list, aalg_entries(),
-			       CRYPTO_ALG_TYPE_HASH, CRYPTO_ALG_TYPE_HASH_MASK,
-			       name, probe);
+	return xfrm_find_algo(&xfrm_aalg_list, xfrm_alg_name_match, name,
+			      probe);
 }
 EXPORT_SYMBOL_GPL(xfrm_aalg_get_byname);
 
 struct xfrm_algo_desc *xfrm_ealg_get_byname(char *name, int probe)
 {
-	return xfrm_get_byname(ealg_list, ealg_entries(),
-			       CRYPTO_ALG_TYPE_BLKCIPHER, CRYPTO_ALG_TYPE_MASK,
-			       name, probe);
+	return xfrm_find_algo(&xfrm_ealg_list, xfrm_alg_name_match, name,
+			      probe);
 }
 EXPORT_SYMBOL_GPL(xfrm_ealg_get_byname);
 
 struct xfrm_algo_desc *xfrm_calg_get_byname(char *name, int probe)
 {
-	return xfrm_get_byname(calg_list, calg_entries(),
-			       CRYPTO_ALG_TYPE_COMPRESS, CRYPTO_ALG_TYPE_MASK,
-			       name, probe);
+	return xfrm_find_algo(&xfrm_calg_list, xfrm_alg_name_match, name,
+			      probe);
 }
 EXPORT_SYMBOL_GPL(xfrm_calg_get_byname);
 
author
#2 lore raw
Quoted 31 lines · Herbert Xu
Quoted 10 lines · Mark Huth

This patch provides a performance optimization in the pfkey_add path. Prior versions have a serious performance problem when adding a large number of SAs to a node. For example, if a backup node needs to be loaded with the SAs previously held by a failed active node, thousands of SAs may need to be added as rapidly as possible. Tests show that without this patch, such additions may take several minutes. The cause is that the available algorithm modules are probed each time instead of only when needed. This patch changes the unconditional call to xfrm_probe_algs() to only be done when it may be needed.

Thanks for the patch!

Quoted 5 lines

static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct
sadb_msg *hdr, void **ext_hdrs)

 {
       struct xfrm_state *x;

Review

Quoted 9 lines
(quoted diff) +1 -3
-       int err;
+       int err, probe_done = 0;
       struct km_event c;
 
-       xfrm_probe_algs();
-
       x = pfkey_msg2xfrm_state(hdr, ext_hdrs);
       if (IS_ERR(x))
               return PTR_ERR(x);
Quoted 4 lines

I don't think it works when then algorithm isn't loaded though :) If the algorithm isn't present pfkey_msg2xfrm_state will return -ENOSYS so we need to do the probe here.

Okay. I tested with the algorithms not loaded, and they were all loaded after the test, but I'm sure you understand this much better than I do.

Quoted 9 lines

Actually, I think we should just probe for the specific algorithm requested rather than everything. See patch below.

[IPSEC] pfkey: Load specific algorithm in pfkey_add rather than all

This is a natural extension of the changeset

[XFRM]: Probe selected algorithm only.

Quoted 8 lines

which only removed the probe call for xfrm_user. This patch does exactly the same thing for af_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states in af_key.

Cheers,

Thanks for the patch. I'll try it later today and confirm that it fixes our problem.

Mark Huth

maintainer
#3 lore raw
In reply to Herbert Xu
Quoted 3 lines · Herbert Xu

Actually, I think we should just probe for the specific algorithm requested rather than everything. See patch below.

Doh, forgot to actually remove the probe call :)

[IPSEC] pfkey: Load specific algorithm in pfkey_add rather than all

This is a natural extension of the changeset

[XFRM]: Probe selected algorithm only.

which only removed the probe call for xfrm_user. This patch does exactly the same thing for af_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states in af_key.

Cheers,

Signature · 5 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
net/key/af_key.c +0 -2
index a994441..d302dda 100644
@@ -1448,8 +1448,6 @@ static int pfkey_add(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr,
 	int err;
 	struct km_event c;
 
-	xfrm_probe_algs();
-
 	x = pfkey_msg2xfrm_state(hdr, ext_hdrs);
 	if (IS_ERR(x))
 		return PTR_ERR(x);
net/xfrm/xfrm_algo.c +75 -65

177 lines not on this page · raw message

maintainer Applied
#4 lore raw

From: Herbert Xu <[email protected]>
Date: Sat, 19 May 2007 07:21:48 +1000

Quoted 11 lines · Herbert Xu
Quoted 3 lines

Actually, I think we should just probe for the specific algorithm requested rather than everything. See patch below.

Doh, forgot to actually remove the probe call :)

[IPSEC] pfkey: Load specific algorithm in pfkey_add rather than all

This is a natural extension of the changeset

Quoted 7 lines

[XFRM]: Probe selected algorithm only.

which only removed the probe call for xfrm_user. This patch does exactly the same thing for af_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states in af_key.

Applied, thanks Herbert.

author
#5 lore raw
In reply to Herbert Xu
Quoted 12 lines · Herbert Xu
Quoted 2 lines · Herbert Xu

Actually, I think we should just probe for the specific algorithm requested rather than everything. See patch below.

Doh, forgot to actually remove the probe call :)

[IPSEC] pfkey: Load specific algorithm in pfkey_add rather than all

This is a natural extension of the changeset

[XFRM]: Probe selected algorithm only.

Quoted 7 lines

which only removed the probe call for xfrm_user. This patch does exactly the same thing for af_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states in af_key.

Cheers,

[... snip]

Herbert,

I can verify that this works. The test adds 2000 instances of SAs using hmac-md5 for authentication and rijndael-cbc for encryption. Test output is:

[email protected]:~# lsmod
Module                  Size  Used by
[email protected]:~#time setkey -f SA_test.txt
real    0m1.072s
user    0m0.048s
sys     0m0.632s
[email protected]:~#lsmod
Module                  Size  Used by
twofish                10112  0
twofish_common         40192  1 twofish
camellia               32768  0
serpent                25216  0
blowfish                9984  0
ecb                     3712  0
aes                    28864  2000
xcbc                    5768  0
sha256                 12416  0
crypto_null             3456  0

[email protected]:~#

Prior to the patch time was over 42 seconds (possibly longer on 2.6.21).

I'm a bit curious why all of the crypto modules got loaded, but it doesn't matter.

Thanks for the patch.

Mark Huth

maintainer
#6 lore raw
Quoted 3 lines · Mark Huth

I'm a bit curious why all of the crypto modules got loaded, but it doesn't matter.

If anybody does a PFKEY_REGISTER call then all the algorithms will get loaded so that the key manager can find out what algorithms are actually available.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#7 lore raw
In reply to Mark Huth

From: Mark Huth <[email protected]>
Date: Thu, 24 May 2007 17:15:30 -0700

Quoted 2 lines

I'm a bit curious why all of the crypto modules got loaded, but it doesn't matter.

When SADB_REGISTER is performed on a PF_KEY socket, it calls xfrm_probe_algs() which iterates over the lists and loads all the availble modules.