net/dst: dst_dev_event() called after other notifiers

Le lundi 08 novembre 2010 à 16:20 -0800, Ben Greear a écrit :
> This is on an otherwise lightly loaded 2.6.36 + hacks system, 12 physical interfaces,
> and two VETH interfaces.
> 
> It's much faster to delete an interface when it has no IPv6 address:
> 
> [root@ct503-60 lanforge]# time ip link add link eth5 up name eth5#0 address 00:00:00:00:00:01 type macvlan
> 
> real	0m0.005s
> user	0m0.001s
> sys	0m0.004s
> [root@ct503-60 lanforge]# time ip link delete eth5#0
> 
> real	0m0.033s
> user	0m0.001s
> sys	0m0.005s
> [root@ct503-60 lanforge]# ip link add link eth5 up name eth5#0 address 00:00:00:00:00:01 type macvlan
> 
> [root@ct503-60 lanforge]# ip -6 addr add 2002::1/64 dev eth5#0
> [root@ct503-60 lanforge]# time ip link delete eth5#0
> 
> real	0m1.030s
> user	0m0.000s
> sys	0m0.013s
> 
> 
> Funny enough, if you explicitly remove the IPv6 addr first it seems
> to run at normal speed (adding both operation's times together)
> 
> [root@ct503-60 lanforge]# ip link add link eth5 up name eth5#0 address 00:00:00:00:00:01 type macvlan
> [root@ct503-60 lanforge]# ip -6 addr add 2002::1/64 dev eth5#0
> [root@ct503-60 lanforge]# time ip -6 addr delete 2002::1/64 dev eth5#0
> 
> real	0m0.001s
> user	0m0.000s
> sys	0m0.001s
> [root@ct503-60 lanforge]# time ip link delete eth5#0
> 
> real	0m0.028s
> user	0m0.001s
> sys	0m0.005s
> 

OK, I confirm I already knew how to correct the problem.

http://www.spinics.net/lists/netdev/msg140729.html

quote :

I also believe the order of netdevice notifiers is wrong (we dont set
priority), and that we should call fib_netdev_event() _before_
dst_dev_event(). This needs another patch.


Thanks

[PATCH] net/dst: dst_dev_event() called after other notifiers

Followup of commit ef885afbf8a37689 (net: use rcu_barrier() in
rollback_registered_many)

dst_dev_event() scans a garbage dst list that might be feeded by various
network notifiers at device dismantle time.

Its important to call dst_dev_event() after other notifiers, or we might
enter the infamous msleep(250) in netdev_wait_allrefs(), and wait one
second before calling again call_netdevice_notifiers(NETDEV_UNREGISTER,
dev) to properly remove last device references.

Use priority -10 to let dst_dev_notifier be called after other network
notifiers (they have the default 0 priority)

Reported-by: Ben Greear <greearb@candelatech.com>
Reported-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reported-by: Octavian Purdila <opurdila@ixiacom.com>
Reported-by: Benjamin LaHaise <bcrl@kvack.org>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/dst.c |    1 +
 1 files changed, 1 insertion(+)



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

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 09 Nov 2010 20:37:55 +0100

> [PATCH] net/dst: dst_dev_event() called after other notifiers

Nice, applied.

However, I had to apply this by hand:

>  static struct notifier_block dst_dev_notifier = {
>  	.notifier_call  = dst_dev_event,
> +	.priority = -10, /* must be called after other network notifiers */
>  };

The character after ".notifier_call" in my tree is a TAB character but
in your patch it is a sequence of spaces.  This isn't looking like the
usual email corruption, because the leading TAB characters on these
lines are properly there.

Please figure out why this happened so that it doesn't repeat in
future patches :-)

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

On 11/09/2010 11:48 AM, David Miller wrote:
> From: Eric Dumazet<eric.dumazet@gmail.com>
> Date: Tue, 09 Nov 2010 20:37:55 +0100
>
>> [PATCH] net/dst: dst_dev_event() called after other notifiers
>
> Nice, applied.
>
> However, I had to apply this by hand:
>
>>   static struct notifier_block dst_dev_notifier = {
>>   	.notifier_call  = dst_dev_event,
>> +	.priority = -10, /* must be called after other network notifiers */
>>   };
>
> The character after ".notifier_call" in my tree is a TAB character but
> in your patch it is a sequence of spaces.  This isn't looking like the
> usual email corruption, because the leading TAB characters on these
> lines are properly there.
>
> Please figure out why this happened so that it doesn't repeat in
> future patches :-)

I manually applied this as well and can confirm that interface deletion
with a global IPv6 address on it is now comparable to any other device
delete (about 30ms).

Tested-by:  Ben Greear <greearb@candelatech.com>

I'd love to test patches that made all interface deletes faster,
btw :)

Thanks,
Ben

Le mardi 09 novembre 2010 à 11:48 -0800, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 09 Nov 2010 20:37:55 +0100
> 
> > [PATCH] net/dst: dst_dev_event() called after other notifiers
> 
> Nice, applied.
> 
> However, I had to apply this by hand:
> 
> >  static struct notifier_block dst_dev_notifier = {
> >  	.notifier_call  = dst_dev_event,
> > +	.priority = -10, /* must be called after other network notifiers */
> >  };
> 
> The character after ".notifier_call" in my tree is a TAB character but
> in your patch it is a sequence of spaces.  This isn't looking like the
> usual email corruption, because the leading TAB characters on these
> lines are properly there.
> 
> Please figure out why this happened so that it doesn't repeat in
> future patches :-)
> 

I am very sorry David, I had to run yesterday night and did a stupid
hand editing right before doing so. It was a human error, not a tool
error. Next time, I'll delay the patch to next day :)

Thanks !


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