| Message ID | 20200917020021.0860995C06B9@us180.sjc.aristanetworks.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: make netdev_wait_allrefs wake-able | expand |
> static inline void dev_put(struct net_device *dev) > { > + struct task_struct *destroy_task = dev->destroy_task; > + > this_cpu_dec(*dev->pcpu_refcnt); > + if (destroy_task) > + wake_up_process(destroy_task); > } I just realized that this introduces a race, if dev_put drops the last reference, an already running netdev_wait_allrefs runs to completion and then dev_put tries to wake it up. Any suggestions on how to avoid this without resorting to locking? Thanks, Francesco
On Thu, Sep 17, 2020 at 8:33 AM Francesco Ruggeri <fruggeri@arista.com> wrote: > > > static inline void dev_put(struct net_device *dev) > > { > > + struct task_struct *destroy_task = dev->destroy_task; > > + > > this_cpu_dec(*dev->pcpu_refcnt); > > + if (destroy_task) > > + wake_up_process(destroy_task); > > } > > I just realized that this introduces a race, if dev_put drops the last > reference, an already running netdev_wait_allrefs runs to completion > and then dev_put tries to wake it up. > Any suggestions on how to avoid this without resorting to > locking? > Honestly I would not touch dev_put() at all. Simply change the msleep(250) to something better, with maybe exponential backoff.
On Wed, Sep 16, 2020 at 11:51 PM Eric Dumazet <edumazet@google.com> wrote: > > Honestly I would not touch dev_put() at all. > > Simply change the msleep(250) to something better, with maybe > exponential backoff. OK, I will try that. Francesco
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index b0e303f6603f..3bbae238c11d 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1795,6 +1795,7 @@ enum netdev_priv_flags { * * @needs_free_netdev: Should unregister perform free_netdev? * @priv_destructor: Called from unregister + * @destroy_task: Task waiting for refcount to drop to 0 * @npinfo: XXX: need comments on this one * @nd_net: Network namespace this network device is inside * @@ -2089,6 +2090,7 @@ struct net_device { bool needs_free_netdev; void (*priv_destructor)(struct net_device *dev); + struct task_struct *destroy_task; #ifdef CONFIG_NETPOLL struct netpoll_info __rcu *npinfo; @@ -3873,7 +3875,11 @@ void netdev_run_todo(void); */ static inline void dev_put(struct net_device *dev) { + struct task_struct *destroy_task = dev->destroy_task; + this_cpu_dec(*dev->pcpu_refcnt); + if (destroy_task) + wake_up_process(destroy_task); } /** diff --git a/net/core/dev.c b/net/core/dev.c index 4086d335978c..795c3d39e807 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9994,6 +9994,7 @@ static void netdev_wait_allrefs(struct net_device *dev) linkwatch_forget_dev(dev); rebroadcast_time = warning_time = jiffies; + dev->destroy_task = current; refcnt = netdev_refcnt_read(dev); while (refcnt != 0) { @@ -10023,7 +10024,8 @@ static void netdev_wait_allrefs(struct net_device *dev) rebroadcast_time = jiffies; } - msleep(250); + set_current_state(TASK_UNINTERRUPTIBLE); + schedule_timeout(HZ/4); refcnt = netdev_refcnt_read(dev); @@ -10033,6 +10035,7 @@ static void netdev_wait_allrefs(struct net_device *dev) warning_time = jiffies; } } + dev->destroy_task = NULL; } /* The sequence is:
The combination of aca_free_rcu, introduced in commit 2384d02520ff ("net/ipv6: Add anycast addresses to a global hashtable"), and fib6_info_destroy_rcu, introduced in commit 9b0a8da8c4c6 ("net/ipv6: respect rcu grace period before freeing fib6_info"), can result in an extra rcu grace period being needed when deleting an interface, with the result that netdev_wait_allrefs ends up hitting the msleep(250), which is considerably longer than the required grace period. This can result in long delays when deleting a large number of interfaces, and it can be observed with this script: ns=dummy-ns NIFS=100 ip netns add $ns ip netns exec $ns ip link set lo up ip netns exec $ns sysctl net.ipv6.conf.default.disable_ipv6=0 ip netns exec $ns sysctl net.ipv6.conf.default.forwarding=1 for ((i=0; i<$NIFS; i++)) do if=eth$i ip netns exec $ns ip link add $if type dummy ip netns exec $ns ip link set $if up ip netns exec $ns ip -6 addr add 2021:$i::1/120 dev $if done for ((i=0; i<$NIFS; i++)) do if=eth$i ip netns exec $ns ip link del $if done ip netns del $ns This patch converts the msleep(250) into a wake-able wait, allowing dev_put to wake up netdev_wait_allrefs. Time with this patch on a 5.4 kernel: real 0m7.494s user 0m0.403s sys 0m1.197s Time without this patch: real 0m31.522s user 0m0.438s sys 0m1.156s Signed-off-by: Francesco Ruggeri <fruggeri@arista.com> --- include/linux/netdevice.h | 6 ++++++ net/core/dev.c | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-)