linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: make netdev_wait_allrefs wake-able
@ 2020-09-17  2:00 Francesco Ruggeri
  2020-09-17  6:33 ` Francesco Ruggeri
  0 siblings, 1 reply; 4+ messages in thread
From: Francesco Ruggeri @ 2020-09-17  2:00 UTC (permalink / raw)
  To: linux-kernel, netdev, xiyou.wangcong, ap420073, andriin,
	edumazet, jiri, ast, kuba, davem, fruggeri

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(-)

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:
-- 
2.28.0



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: make netdev_wait_allrefs wake-able
  2020-09-17  2:00 [PATCH] net: make netdev_wait_allrefs wake-able Francesco Ruggeri
@ 2020-09-17  6:33 ` Francesco Ruggeri
  2020-09-17  6:51   ` Eric Dumazet
  0 siblings, 1 reply; 4+ messages in thread
From: Francesco Ruggeri @ 2020-09-17  6:33 UTC (permalink / raw)
  To: open list, netdev, xiyou.wangcong, ap420073, andriin,
	Eric Dumazet, jiri, ast, Jakub Kicinski, David Miller,
	Francesco Ruggeri

>  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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: make netdev_wait_allrefs wake-able
  2020-09-17  6:33 ` Francesco Ruggeri
@ 2020-09-17  6:51   ` Eric Dumazet
  2020-09-17 16:17     ` Francesco Ruggeri
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2020-09-17  6:51 UTC (permalink / raw)
  To: Francesco Ruggeri
  Cc: open list, netdev, Cong Wang, Taehee Yoo, Andrii Nakryiko,
	Jiri Pirko, Alexei Starovoitov, Jakub Kicinski, David Miller

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: make netdev_wait_allrefs wake-able
  2020-09-17  6:51   ` Eric Dumazet
@ 2020-09-17 16:17     ` Francesco Ruggeri
  0 siblings, 0 replies; 4+ messages in thread
From: Francesco Ruggeri @ 2020-09-17 16:17 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: open list, netdev, Cong Wang, Taehee Yoo, Andrii Nakryiko,
	Jiri Pirko, Alexei Starovoitov, Jakub Kicinski, David Miller

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-17 19:18 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17  2:00 [PATCH] net: make netdev_wait_allrefs wake-able Francesco Ruggeri
2020-09-17  6:33 ` Francesco Ruggeri
2020-09-17  6:51   ` Eric Dumazet
2020-09-17 16:17     ` Francesco Ruggeri

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).