pfkey_add: Optimize SA adds and algorithm probes
Subsystems (3)
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
@@ -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;
Herbert Xu
maintainerQuoted 10 lines · Mark Huth
This patch provides a performance optimization in the
pfkey_addpath. 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 toxfrm_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, structsadb_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
@@ -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);
Quoted 31 lines · Herbert Xu
Quoted 10 lines · Mark Huth
This patch provides a performance optimization in the
pfkey_addpath. 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 toxfrm_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, structsadb_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_statewill return-ENOSYSso 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_addrather than allThis 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 foraf_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states inaf_key.Signed-off-by Herbert Xu <[email protected]>Cheers,
Thanks for the patch. I'll try it later today and confirm that it fixes our problem.
Mark Huth
Herbert Xu
maintainerQuoted 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
@@ -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
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_addrather than allThis 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 foraf_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states inaf_key.Signed-off-by Herbert Xu <[email protected]>
Applied, thanks Herbert.
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_addrather than allThis 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 foraf_key. In other words, we load the algorithm requested by the user rather than everything when adding xfrm states inaf_key.Signed-off-by Herbert Xu <[email protected]>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
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
Herbert Xu
maintainerQuoted 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.txtDavid Miller
maintainerFrom: 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.