Message-ID: <m162x5492h.fsf@fess.ebiederm.org>
Date:	Wed, 13 Oct 2010 22:20:28 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	David Miller <davem@...emloft.net>
Cc:	hans.schillstrom@...csson.com, daniel.lezcano@...e.fr,
	netdev@...r.kernel.org
Subject: Re: BUG ? ipip unregister_netdevice_many()

David Miller <davem@...emloft.net> writes:

> From: ebiederm@...ssion.com (Eric W. Biederman)
> Date: Wed, 13 Oct 2010 21:40:49 -0700
>
>> However I think the test should still be rt_is_expired(), because
>> that is what rt_do_flush() is doing removing the expired entries
>> from the list.
>
> I can't see a reason for that test.
>
> Everything calling into this code path has created a condition
> that requires that all routing cache entries for that namespace
> be deleted.
>
> This function is meant to unconditionally flush the entire table.
>
> I believe you added that extraneous test, and it never existed there
> before.

At the point network namespaces entered the picture the logic was:

	void rt_cache_flush(struct net *net, int delay)
	{
		rt_cache_invalidate();
		if (delay >= 0)
			rt_do_flush(!in_softirq());
	}
	
	/* Strictly speaking rt_is_expired was just open coded in
	 * rt_check_expire. But this is the check that was used.
	 */
	static inline int rt_is_expired(struct rtable *rth)
	{
		return rth->rt_genid != atomic_read(&rt_genid);
	}
	
	static void rt_cache_invalidate(void)
	{
	        unsigned char shuffle;
	 
		get_random_bytes(&shuffle, sizeof(shuffle));
		atomic_add(shuffle + 1U, &rt_genid);
	}
	
	static void rt_do_flush(int process_context)
	{
	        unsigned int i;
	        struct rtable *rth, *next;
	
	        for (i = 0; i <= rt_hash_mask; i++) {
			if (process_context && need_resched())
				cond_resched();
			rth = rt_hash_table[i].chain;
			if (!rth)
				continue;
			
			spin_lock_bh(rt_hash_lock_addr(i));
			rth = rt_hash_table[i].chain;
			rt_hash_table[i].chain = NULL;
			tail = NULL;
			spin_unlock_bh(rt_hash_lock_addr(i));
			
			for(; rth != tail; rth = next)
			{
				next = rth->dst.rt_next;
				rt_free(rth);
			}
		}
	}

Because of the rt_cache_invalidate() in rt_cache_flush() this
guaranteed that rt_is_expired() was true for every route cache entry,
and this also guaranteed that every routing cache entry we were flush
atomically became inaccessible.

So rt_is_expired() has always been valid, but in practice it was just
always optimized out as being redundant.

With the network namespace support we limit the scope of the test of
the invalidate to just a single network namespace, and as such
rt_is_expired stops being true for every cache entry.  So we cannot
unconditionally throw away entire chains.

All of which can be either done by network namespace equality or by
rt_is_expired().  Although Denis picked rt_is_expired() when he made
his change.

The only place it makes a noticable difference in practice is what
happens when we do batched deleletes of lots of network devices in
different network namespaces.

During batched network device deletes in fib_netdev_event we do
rt_cache_flush(dev_net(dev), -1) for each network device.  and then a
final rt_cache_flush_batch() to remove the invalidated entries.  These
devices can be from multiple network namespaces, so I suspect that is
a savings worth having.

So if we are going to change the tests we need to do something with
rt_cache_flush_batch().  Further I do not see what is confusing about
a test that asks if the routing cache entry is unusable.  Is
rt_cache_expired() a bad name?

Eric

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists