[PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com>

Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 net/ipv6/xfrm6_policy.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index f3ed8ca..93832e8 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -332,10 +332,10 @@ int __init xfrm6_init(void)
 	/*
 	 * We need a good default value for the xfrm6 gc threshold.
 	 * In ipv4 we set it to the route hash table size * 8, which
-	 * is half the size of the maximaum route cache for ipv4.  It
+	 * is half the size of the maximum route cache for ipv4.  It
 	 * would be good to do the same thing for v6, except the table is
 	 * constructed differently here.  Here each table for a net namespace
-	 * can have FIB_TABLE_HASHSZ entries, so lets go with the same
+	 * can have FIB6_TABLE_HASHSZ entries, so lets go with the same
 	 * computation that we used for ipv4 here.  Also, lets keep the initial
 	 * gc_thresh to a minimum of 1024, since, the ipv6 route cache defaults
 	 * to that as a minimum as well
-- 
1.7.4.1

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[Steffen Klassert]

On Thu, Nov 08, 2012 at 04:38:08PM +0800, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
>  net/ipv6/xfrm6_policy.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
> index f3ed8ca..93832e8 100644
> --- a/net/ipv6/xfrm6_policy.c
> +++ b/net/ipv6/xfrm6_policy.c
> @@ -332,10 +332,10 @@ int __init xfrm6_init(void)
>  	/*
>  	 * We need a good default value for the xfrm6 gc threshold.
>  	 * In ipv4 we set it to the route hash table size * 8, which
> -	 * is half the size of the maximaum route cache for ipv4.  It
> +	 * is half the size of the maximum route cache for ipv4.

The routing cache is removed, so this comment is obsolete. But it reminds
me that we set the gc threshold to ip_rt_max_size/2 in ipv4. With the
routing cache removal patch, ip_rt_max_size was set to INT_MAX. So the gc
starts to remove entries when a threshold of INT_MAX/2 is reached.

cat /proc/sys/net/ipv4/xfrm4_gc_thresh
1073741823

I guess this was not intentional.

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[David Miller]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Thu, 8 Nov 2012 15:19:36 +0100

> On Thu, Nov 08, 2012 at 04:38:08PM +0800, roy.qing.li@gmail.com wrote:
>> From: Li RongQing <roy.qing.li@gmail.com>
>> 
>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>> ---
>>  net/ipv6/xfrm6_policy.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
>> index f3ed8ca..93832e8 100644
>> --- a/net/ipv6/xfrm6_policy.c
>> +++ b/net/ipv6/xfrm6_policy.c
>> @@ -332,10 +332,10 @@ int __init xfrm6_init(void)
>>  	/*
>>  	 * We need a good default value for the xfrm6 gc threshold.
>>  	 * In ipv4 we set it to the route hash table size * 8, which
>> -	 * is half the size of the maximaum route cache for ipv4.  It
>> +	 * is half the size of the maximum route cache for ipv4.
> 
> The routing cache is removed, so this comment is obsolete. But it reminds
> me that we set the gc threshold to ip_rt_max_size/2 in ipv4. With the
> routing cache removal patch, ip_rt_max_size was set to INT_MAX. So the gc
> starts to remove entries when a threshold of INT_MAX/2 is reached.
> 
> cat /proc/sys/net/ipv4/xfrm4_gc_thresh
> 1073741823
> 
> I guess this was not intentional.

Do you mean for IPSEC routes?  For non-IPSEC routes on ipv4 there
is nothing to garbage collect.

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[Steffen Klassert]

On Thu, Nov 08, 2012 at 02:59:39PM -0500, David Miller wrote:
> From: Steffen Klassert <steffen.klassert@secunet.com>
> > 
> > The routing cache is removed, so this comment is obsolete. But it reminds
> > me that we set the gc threshold to ip_rt_max_size/2 in ipv4. With the
> > routing cache removal patch, ip_rt_max_size was set to INT_MAX. So the gc
> > starts to remove entries when a threshold of INT_MAX/2 is reached.
> > 
> > cat /proc/sys/net/ipv4/xfrm4_gc_thresh
> > 1073741823
> > 
> > I guess this was not intentional.
> 
> Do you mean for IPSEC routes?  For non-IPSEC routes on ipv4 there
> is nothing to garbage collect.

Yes, I mean IPsec routes. We still cache them at the flow cache
and at sockets, so we should do some garbage collecting.

We could either go back to a static threshold, as it was before

git commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
xfrm: select sane defaults for xfrm[4|6] gc_thresh

or do the same as ipv6 does. I'll take care of this.

Btw. it seems to me that the flow cache has similar limitations
as the routing cache had. At least I was able to fill the flow
cache with a nmap scan from a remote entity. In practice, it's
hard to DOS the flow cache because we use a Jenkins hash with
a random initialization value, but this was the same with the
routing cache.

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[Steffen Klassert]

On Fri, Nov 09, 2012 at 08:57:26AM +0100, Steffen Klassert wrote:
> On Thu, Nov 08, 2012 at 02:59:39PM -0500, David Miller wrote:
> > From: Steffen Klassert <steffen.klassert@secunet.com>
> > > 
> > > The routing cache is removed, so this comment is obsolete. But it reminds
> > > me that we set the gc threshold to ip_rt_max_size/2 in ipv4. With the
> > > routing cache removal patch, ip_rt_max_size was set to INT_MAX. So the gc
> > > starts to remove entries when a threshold of INT_MAX/2 is reached.
> > > 
> > > cat /proc/sys/net/ipv4/xfrm4_gc_thresh
> > > 1073741823
> > > 
> > > I guess this was not intentional.
> > 
> > Do you mean for IPSEC routes?  For non-IPSEC routes on ipv4 there
> > is nothing to garbage collect.
> 
> Yes, I mean IPsec routes. We still cache them at the flow cache
> and at sockets, so we should do some garbage collecting.
> 
> We could either go back to a static threshold, as it was before
> 
> git commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
> xfrm: select sane defaults for xfrm[4|6] gc_thresh
> 
> or do the same as ipv6 does. I'll take care of this.

Actually, the ipv6 side of the xfrm gc threshold is a bit confusing.
The calculation depends on FIB6_TABLE_HASHSZ which has nothing to
do with maximum number of routes. FIB6_TABLE_HASHSZ just reflects
how many different routing tables we can use. It depends on whether
policy routing is enabled and is either 1 or 256. The maximum number
of routes for ipv6 defaults to 4096. So maybe the xfrm gc threshold
should depend somehow on this value.

For the ipv4 side I plan to go back to a static xfrm gc threshold
as it was before we did this dynamically.

I'll apply the patch below to the ipsec tree if there no other
ideas on how to choose the xfrm gc threshold.


Subject: [PATCH] xfrm: Fix the gc threshold value for ipv4

The xfrm gc threshold value depends on ip_rt_max_size. This
value was set to INT_MAX with the routing cache removal patch,
so we start doing garbage collecting when we have INT_MAX/2
IPsec routes cached. Fix this by going back to the static
threshold of 1024 routes.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/xfrm.h      |    2 +-
 net/ipv4/route.c        |    2 +-
 net/ipv4/xfrm4_policy.c |   13 +------------
 3 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 6f0ba01..63445ed 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1351,7 +1351,7 @@ struct xfrm6_tunnel {
 };
 
 extern void xfrm_init(void);
-extern void xfrm4_init(int rt_hash_size);
+extern void xfrm4_init(void);
 extern int xfrm_state_init(struct net *net);
 extern void xfrm_state_fini(struct net *net);
 extern void xfrm4_state_init(void);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index a8c6512..200d287 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2597,7 +2597,7 @@ int __init ip_rt_init(void)
 		pr_err("Unable to create route proc files\n");
 #ifdef CONFIG_XFRM
 	xfrm_init();
-	xfrm4_init(ip_rt_max_size);
+	xfrm4_init();
 #endif
 	rtnl_register(PF_INET, RTM_GETROUTE, inet_rtm_getroute, NULL, NULL);
 
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 05c5ab8..3be0ac2 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -279,19 +279,8 @@ static void __exit xfrm4_policy_fini(void)
 	xfrm_policy_unregister_afinfo(&xfrm4_policy_afinfo);
 }
 
-void __init xfrm4_init(int rt_max_size)
+void __init xfrm4_init(void)
 {
-	/*
-	 * Select a default value for the gc_thresh based on the main route
-	 * table hash size.  It seems to me the worst case scenario is when
-	 * we have ipsec operating in transport mode, in which we create a
-	 * dst_entry per socket.  The xfrm gc algorithm starts trying to remove
-	 * entries at gc_thresh, and prevents new allocations as 2*gc_thresh
-	 * so lets set an initial xfrm gc_thresh value at the rt_max_size/2.
-	 * That will let us store an ipsec connection per route table entry,
-	 * and start cleaning when were 1/2 full
-	 */
-	xfrm4_dst_ops.gc_thresh = rt_max_size/2;
 	dst_entries_init(&xfrm4_dst_ops);
 
 	xfrm4_state_init();
-- 
1.7.9.5

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[David Miller]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Tue, 13 Nov 2012 10:00:06 +0100

> Subject: [PATCH] xfrm: Fix the gc threshold value for ipv4
> 
> The xfrm gc threshold value depends on ip_rt_max_size. This
> value was set to INT_MAX with the routing cache removal patch,
> so we start doing garbage collecting when we have INT_MAX/2
> IPsec routes cached. Fix this by going back to the static
> threshold of 1024 routes.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

This looks fine to me.

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[Steffen Klassert]

On Wed, Nov 14, 2012 at 06:55:28PM -0500, David Miller wrote:
> From: Steffen Klassert <steffen.klassert@secunet.com>
> Date: Tue, 13 Nov 2012 10:00:06 +0100
> 
> > Subject: [PATCH] xfrm: Fix the gc threshold value for ipv4
> > 
> > The xfrm gc threshold value depends on ip_rt_max_size. This
> > value was set to INT_MAX with the routing cache removal patch,
> > so we start doing garbage collecting when we have INT_MAX/2
> > IPsec routes cached. Fix this by going back to the static
> > threshold of 1024 routes.
> > 
> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> 
> This looks fine to me.

I've just applied this to the ipsec tree.

I'll do the same for the ipv6 side. ipv6 does not handle the maximum
number of routes dynamically, so no need to try to handle the IPsec
gc threshold dynamically.

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[David Miller]

From: roy.qing.li@gmail.com
Date: Thu,  8 Nov 2012 16:38:08 +0800

> From: Li RongQing <roy.qing.li@gmail.com>
> 
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>

As Steffen stated, this comment is largely obsolete.

Re: [PATCH] ipv6: fix two typos in a comment in xfrm6_init()

[RongQing Li]

2012/11/9 David Miller <davem@davemloft.net>:
> From: roy.qing.li@gmail.com
> Date: Thu,  8 Nov 2012 16:38:08 +0800
>
>> From: Li RongQing <roy.qing.li@gmail.com>
>>
>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>
> As Steffen stated, this comment is largely obsolete.

Thanks, I will resend new patch to remove this comments

-Roy