Re: [PATCH] ipv4 routing: Fixes to allow route cache entries to work when route caching is disabled

[Jarek Poplawski <jarkao2@gmail.com>]

On Mon, Jun 22, 2009 at 01:03:27PM -0400, Neil Horman wrote:
> On Mon, Jun 22, 2009 at 06:51:45PM +0200, Jarek Poplawski wrote:
> > On Mon, Jun 22, 2009 at 11:23:14AM -0400, Neil Horman wrote:
> > > Hey all-
> > > 	As we've been discussing recently, There are a few bugs with routing if
> > > we exceed our route cache rebuild count, and subsequently disable route caching.
> > > An oops was reported to me, which has been subsequently fixed, and then
> > > subsequently a route cache leak and failure to forward frames was reported to me
> > > when rt_caching returns false.  I've reproduced these on a local system, and
> > > tracked down the cause.  This patch fixes both of these problems for me on my
> > > test system.
> > > 
> > > 
> > >     Ensure that route cache entries are usable and reclaimable when caching is off
> > >     
> > >     When route caching is disabled (rt_caching returns false), We still use route
> > >     cache entries that are created and passed into rt_intern_hash once.  These
> > >     routes need to be made usable for the one call path that holds a reference to
> > >     them, and they need to be reclaimed when they're finished with their use.  To be
> > >     made usable, they need to be associated with a neighbor table entry (which they
> > >     currently are not), otherwise iproute_finish2 just discards the packet, since we
> > >     don't know which L2 peer to send the packet to.  To do this binding, we need to
> > >     follow the path a bit higher up in rt_intern_hash, which calls
> > >     arp_bind_neighbour, but not assign the route entry to the hash table.
> > >     Currently, if caching is off, we simply assign the route to the rp pointer and
> > >     are reutrn success.  This patch associates us with a neighbor entry first.
> > >     
> > >     Secondly, we need to make sure that any single use routes like this are known to
> > >     the garbage collector when caching is off.  If caching is off, and we try to
> > >     hash in a route, it will leak when its refcount reaches zero.  To avoid this,
> > >     this patch calls rt_free on the route cache entry passed into rt_intern_hash.
> > >     This places us on the gc list for the route cache garbage collector, so that
> > >     when its refcount reaches zero, it will be reclaimed (Thanks to Alexey for this
> > >     suggestion).
> > >     
> > >     I've tested this on a local system here, and with these patches in place, I'm
> > >     able to maintain routed connectivity to remote systems, even if I set
> > >     /proc/sys/net/ipv4/rt_cache_rebuild_count to -1, which forces rt_caching to
> > >     return false.
> > >     
> > >     Best
> > >     Neil
> > >     
> > >     Signed-off-by: Neil Horman <nhorman@redhat.com>
> > >     Reported-by: Jarek Poplawski <jarkao2@gmail.com>
> > >     Reported-by: Maxime Bizon <mbizon@freebox.fr>
> > > 
> > > 
> > >  route.c |   44 ++++++++++++++++++++++++++------------------
> > >  1 file changed, 26 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> > > index 65b3a8b..4b21513 100644
> > > --- a/net/ipv4/route.c
> > > +++ b/net/ipv4/route.c
> > > @@ -1076,6 +1076,7 @@ static int rt_intern_hash(unsigned hash, struct rtable *rt,
> > >  	u32 		min_score;
> > >  	int		chain_length;
> > >  	int attempts = !in_softirq();
> > > +	int caching = rt_caching(dev_net(rt->u.dst.dev));
> > >  
> > >  restart:
> > >  	chain_length = 0;
> > > @@ -1084,7 +1085,7 @@ restart:
> > >  	candp = NULL;
> > >  	now = jiffies;
> > >  
> > > -	if (!rt_caching(dev_net(rt->u.dst.dev))) {
> > > +	if (!caching) {
> > >  		/*
> > >  		 * If we're not caching, just tell the caller we
> > >  		 * were successful and don't touch the route.  The
> > > @@ -1093,8 +1094,12 @@ restart:
> > >  		 * If we drop it here, the callers have no way to resolve routes
> > >  		 * when we're not caching.  Instead, just point *rp at rt, so
> > >  		 * the caller gets a single use out of the route
> > > +		 * Note that we do rt_free on this new route entry, so that
> > > +		 * once its refcount hits zero, we are still able to reap it
> > > +		 * (Thanks Alexey)
> > 
> > I hope Alexey Dobriyan won't be confused... 
> > 
> > >  		 */
> > > -		goto report_and_exit;
> > > +		rt_free(rt);
> > 
> > To save some coulds & woulds in the future I'd(!) prefer here
> > dst_free() yet.
> Not sure I see the advantage.  The path winds up being the same regardless, the
> typing matches up with the rt_free call, and by using the RCU path we are given
> the possibility to batch a bunch of spinlocks in the cache at an RCU quiesence
> point.

IMHO it's simply misleading. I don't think call_rcu() is a proper way
to improve cache performance.

> 
> 
> > 
> > > +		goto skip_hashing;
> > 
> > Aren't we jumping over a spin_lock here?
> > 
> We are jumping over a spinlock, both the acquire and release, which is exactly
> what we want, since when rt_caching returns false, we're not adding the route
> cache entry into the hash table, which is what that lock protects.

+skip_hashing:
>  	if (rt->rt_type == RTN_UNICAST || rt->fl.iif == 0) {
>  		int err = arp_bind_neighbour(&rt->u.dst);
>  		if (err) {

Even if (err) is impossible here with skip_hashing I think it's
_extremely_ unreadable. Why can't we inline these lines in the above
'if (!caching)' block and save one such if later?

Jarek P.