David Miller | 18 Jul 2012 20:24
Favicon

[PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.


If we have an output route that lacks nexthop exceptions, we can cache
it in the FIB info nexthop.

Such routes will have DST_HOST cleared because such routes refer to a
family of destinations, rather than just one.

The sequence of the handling of exceptions during route lookup is
adjusted to make the logic work properly.

Before we allocate the route, we lookup the exception.

Then we know if we will cache this route or not, and therefore whether
DST_HOST should be set on the allocated route.

Then we use DST_HOST to key off whether we should store the resulting
route, during rt_set_nexthop(), in the FIB nexthop cache.

To counter adding a new argument to rt_set_nexthop() I'm removing the
'fl4' arg to rt_set_nexthop() and rt_init_metrics(), which is no
longer used.

Signed-off-by: David S. Miller <davem <at> davemloft.net>
---
 include/net/ip_fib.h     |    2 +
 net/ipv4/fib_semantics.c |    2 +
 net/ipv4/route.c         |  109 ++++++++++++++++++++++++++++++++--------------
 3 files changed, 80 insertions(+), 33 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
(Continue reading)

Eric Dumazet | 18 Jul 2012 22:34
Picon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.

On Wed, 2012-07-18 at 11:24 -0700, David Miller wrote:
> If we have an output route that lacks nexthop exceptions, we can cache
> it in the FIB info nexthop.
> 
> Such routes will have DST_HOST cleared because such routes refer to a
> family of destinations, rather than just one.

> -			   const struct fib_result *res,
> +static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
> +{
> +	static DEFINE_SPINLOCK(fib_cache_lock);
> +	struct rtable **p = &nh->nh_rth_output;
> +
> +	if (*p)
> +		return;
> +
> +	spin_lock_bh(&fib_cache_lock);
> +	if (!*p) {
> +		*p = rt;
> +		dst_clone(&rt->dst);
> +	}
> +	spin_unlock_bh(&fib_cache_lock);
> +}
> +

This probably should use cmpxchg()

static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
{
	struct rtable **p = &nh->nh_rth_output;
(Continue reading)

David Miller | 18 Jul 2012 22:36
Favicon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.

From: Eric Dumazet <eric.dumazet <at> gmail.com>
Date: Wed, 18 Jul 2012 22:34:19 +0200

> On Wed, 2012-07-18 at 11:24 -0700, David Miller wrote:
>> If we have an output route that lacks nexthop exceptions, we can cache
>> it in the FIB info nexthop.
>> 
>> Such routes will have DST_HOST cleared because such routes refer to a
>> family of destinations, rather than just one.
> 
> 
>> -			   const struct fib_result *res,
>> +static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
>> +{
>> +	static DEFINE_SPINLOCK(fib_cache_lock);
>> +	struct rtable **p = &nh->nh_rth_output;
>> +
>> +	if (*p)
>> +		return;
>> +
>> +	spin_lock_bh(&fib_cache_lock);
>> +	if (!*p) {
>> +		*p = rt;
>> +		dst_clone(&rt->dst);
>> +	}
>> +	spin_unlock_bh(&fib_cache_lock);
>> +}
>> +
> 
> This probably should use cmpxchg()
(Continue reading)

Eric Dumazet | 18 Jul 2012 22:40
Picon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.

On Wed, 2012-07-18 at 13:36 -0700, David Miller wrote:

> Do you really think it's faster/better in this case?  I did consider
> it.

No, but code is smaller ;)

David Miller | 18 Jul 2012 22:49
Favicon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.

From: Eric Dumazet <eric.dumazet <at> gmail.com>
Date: Wed, 18 Jul 2012 22:40:32 +0200

> On Wed, 2012-07-18 at 13:36 -0700, David Miller wrote:
> 
>> Do you really think it's faster/better in this case?  I did consider
>> it.
> 
> No, but code is smaller ;)

Ok, I'm convinced :)
Steffen Klassert | 19 Jul 2012 13:38
Favicon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.

On Wed, Jul 18, 2012 at 11:24:04AM -0700, David Miller wrote:
> +
> +static void rt_bind_exception(struct rtable *rt, struct fib_nh_exception *fnhe)
> +{
> +	if (fnhe->fnhe_pmtu) {
> +		unsigned long expires = fnhe->fnhe_expires;
> +		unsigned long diff = jiffies - expires;

This should be diff = expires - jiffies

With that changed, everything seems to work fine :)

> +
> +		if (time_before(jiffies, expires)) {
> +			rt->rt_pmtu = fnhe->fnhe_pmtu;
> +			dst_set_expires(&rt->dst, diff);
>  		}
>  	}
> +	if (fnhe->fnhe_gw) {
> +		rt->rt_flags |= RTCF_REDIRECTED;
> +		rt->rt_gateway = fnhe->fnhe_gw;
> +	}
> +	fnhe->fnhe_stamp = jiffies;
>  }
>  
David Miller | 19 Jul 2012 17:39
Favicon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.

From: Steffen Klassert <steffen.klassert <at> secunet.com>
Date: Thu, 19 Jul 2012 13:38:10 +0200

> On Wed, Jul 18, 2012 at 11:24:04AM -0700, David Miller wrote:
>> +
>> +static void rt_bind_exception(struct rtable *rt, struct fib_nh_exception *fnhe)
>> +{
>> +	if (fnhe->fnhe_pmtu) {
>> +		unsigned long expires = fnhe->fnhe_expires;
>> +		unsigned long diff = jiffies - expires;
> 
> This should be diff = expires - jiffies
> 
> With that changed, everything seems to work fine :)

Thanks a lot for catching this bug, I'll fix it up right now.

David Miller | 19 Jul 2012 17:52
Favicon

Re: [PATCH 09/15] ipv4: Cache output routes in fib_info nexthops.


In a dream I found a bug in this patch and the next one.

When we fetch a cached route from the FIB info, we have to
check if it has been invalidated by a PMTU event or similar.
And if so, cmpxchg() it with NULL and release it, so we
can build and install a new cached route there.

I'll fix this up when I integrate everyone's feedback later
today.

Gmane