Re: [PATCH] net: allow netdev_wait_allrefs() to run faster

[Stephen Hemminger <shemminger@vyatta.com>]

On Sat, 24 Oct 2009 06:35:53 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Paul E. McKenney a écrit :
> > On Wed, Oct 21, 2009 at 05:40:07PM +0200, Eric Dumazet wrote:
> >> [PATCH] net: allow netdev_wait_allrefs() to run faster
> >>
> >> netdev_wait_allrefs() waits that all references to a device vanishes.
> >>
> >> It currently uses a _very_ pessimistic 250 ms delay between each probe.
> >> Some users report that no more than 4 devices can be dismantled per second,
> >> this is a pretty serious problem for extreme setups.
> >>
> >> Most likely, references only wait for a rcu grace period that should come
> >> fast, so use a schedule_timeout_uninterruptible(1) to allow faster recovery.
> > 
> > Is this a place where synchronize_rcu_expedited() is appropriate?
> > (It went in to 2.6.32-rc1.)
> > 
> 
> Thanks for the tip Paul
> 
> I believe netdev_wait_allrefs() is not a perfect candidate, because 
> synchronize_sched_expedited() seems really expensive.
> 
> Maybe we could call it once only, if we had to call 1 times
> the jiffie delay ?
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fa88dcd..9b04b9a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4970,6 +4970,7 @@ EXPORT_SYMBOL(register_netdev);
>  static void netdev_wait_allrefs(struct net_device *dev)
>  {
>  	unsigned long rebroadcast_time, warning_time;
> +	unsigned int count = 0;
>  
>  	rebroadcast_time = warning_time = jiffies;
>  	while (atomic_read(&dev->refcnt) != 0) {
> @@ -4995,7 +4996,10 @@ static void netdev_wait_allrefs(struct net_device *dev)
>  			rebroadcast_time = jiffies;
>  		}
>  
> -		msleep(250);
> +		if (count++ == 1)
> +			synchronize_rcu_expedited();
> +		else
> +			schedule_timeout_uninterruptible(1);
>  
>  		if (time_after(jiffies, warning_time + 10 * HZ)) {
>  			printk(KERN_EMERG "unregister_netdevice: "

Actually, anything that requires more than one pass through the loop is
broken. Devices and protocols should be cleaning up on the first notifier.
The worst offender seems to be the dst cache gc code.


--