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 <[email protected]>
---
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
> 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 <[email protected]> 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 <[email protected]> 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