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 uses exponential backoff instead of the fixed msleep(250)
to get out of the loop faster.
Time with this patch on a 5.4 kernel:
real 0m8.199s
user 0m0.402s
sys 0m1.213s
Time without this patch:
real 0m31.522s
user 0m0.438s
sys 0m1.156s
v2: use exponential backoff instead of trying to wake up
netdev_wait_allrefs.
v3: preserve reverse christmas tree ordering of local variables
Signed-off-by: Francesco Ruggeri <[email protected]>
---
net/core/dev.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 4086d335978c..e5fa60cb8832 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9986,9 +9986,12 @@ EXPORT_SYMBOL(netdev_refcnt_read);
* We can get stuck here if buggy protocols don't correctly
* call dev_put.
*/
+#define MIN_MSLEEP ((unsigned int)16)
+#define MAX_MSLEEP ((unsigned int)250)
static void netdev_wait_allrefs(struct net_device *dev)
{
unsigned long rebroadcast_time, warning_time;
+ unsigned int wait = MIN_MSLEEP;
int refcnt;
linkwatch_forget_dev(dev);
@@ -10023,7 +10026,8 @@ static void netdev_wait_allrefs(struct net_device *dev)
rebroadcast_time = jiffies;
}
- msleep(250);
+ msleep(wait);
+ wait = min(wait << 1, MAX_MSLEEP);
refcnt = netdev_refcnt_read(dev);
--
2.28.0
On Thu, 17 Sep 2020 16:49:53 -0700
[email protected] (Francesco Ruggeri) wrote:
> 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:
>
Is there anyway to make RCU trigger faster?
On Thu, Sep 17, 2020 at 5:02 PM Stephen Hemminger
<[email protected]> wrote:
> Is there anyway to make RCU trigger faster?
This is a case of the networking code requiring multiple cascading grace periods
(functions executing at the end of a period scheduling more functions
for the end
of the next period), so it's a matter of how many steps are required,
rather than
how fast each step is, if that is what you are suggesting. I don't
think that expediting
rcu periods would help in this case, but I will defer to people with better
knowledge of the networking code than me to comment.
On Fri, Sep 18, 2020 at 1:49 AM Francesco Ruggeri <[email protected]> wrote:
>
> 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 uses exponential backoff instead of the fixed msleep(250)
> to get out of the loop faster.
>
> Time with this patch on a 5.4 kernel:
>
> real 0m8.199s
> user 0m0.402s
> sys 0m1.213s
>
> Time without this patch:
>
> real 0m31.522s
> user 0m0.438s
> sys 0m1.156s
>
> v2: use exponential backoff instead of trying to wake up
> netdev_wait_allrefs.
> v3: preserve reverse christmas tree ordering of local variables
>
> Signed-off-by: Francesco Ruggeri <[email protected]>
> ---
> net/core/dev.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4086d335978c..e5fa60cb8832 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9986,9 +9986,12 @@ EXPORT_SYMBOL(netdev_refcnt_read);
> * We can get stuck here if buggy protocols don't correctly
> * call dev_put.
> */
> +#define MIN_MSLEEP ((unsigned int)16)
> +#define MAX_MSLEEP ((unsigned int)250)
No need for a cast, also I would use names less likely to collide with
include files, and I would start at 1 ms.
#define WAIT_REFS_MIN_MSECS 1
#define WAIT_REFS_MAX_MSECS 250
>
> static void netdev_wait_allrefs(struct net_device *dev)
> {
> unsigned long rebroadcast_time, warning_time;
> + unsigned int wait = MIN_MSLEEP;
int wait = WAIT_REFS_MIN_MSECS;
>
> int refcnt;
>
> linkwatch_forget_dev(dev);
> @@ -10023,7 +10026,8 @@ static void netdev_wait_allrefs(struct net_device *dev)
> rebroadcast_time = jiffies;
> }
>
> - msleep(250);
> + msleep(wait);
> + wait = min(wait << 1, MAX_MSLEEP);
wait = min(wait << 1, WAIT_REFS_MAX_MSECS);
>
>
> refcnt = netdev_refcnt_read(dev);
>
> --
> 2.28.0
Also, I would try using synchronize_rcu() instead of the first
msleep(), this might avoid all msleep() calls in your case.
Patch without the macros to see the general idea :
diff --git a/net/core/dev.c b/net/core/dev.c
index 266073e300b5fc21440ea8f8ffc9306a1fc9f370..2d3b65034bc0dd99017dea846e6c0a966f1207ee
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9989,7 +9989,7 @@ EXPORT_SYMBOL(netdev_refcnt_read);
static void netdev_wait_allrefs(struct net_device *dev)
{
unsigned long rebroadcast_time, warning_time;
- int refcnt;
+ int wait = 0, refcnt;
linkwatch_forget_dev(dev);
@@ -10023,8 +10023,13 @@ static void netdev_wait_allrefs(struct net_device *dev)
rebroadcast_time = jiffies;
}
- msleep(250);
-
+ if (!wait) {
+ synchronize_rcu();
+ wait = 1;
+ } else {
+ msleep(wait);
+ wait = min(wait << 1, 250);
+ }
refcnt = netdev_refcnt_read(dev);
if (refcnt && time_after(jiffies, warning_time + 10 * HZ)) {
On Fri, Sep 18, 2020 at 10:48 AM Eric Dumazet <[email protected]> wrote:
>
>
> Also, I would try using synchronize_rcu() instead of the first
s/synchronize_rcu/rcu_barrier/ of course :/
> msleep(), this might avoid all msleep() calls in your case.
>
> Patch without the macros to see the general idea :
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 266073e300b5fc21440ea8f8ffc9306a1fc9f370..2d3b65034bc0dd99017dea846e6c0a966f1207ee
> 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9989,7 +9989,7 @@ EXPORT_SYMBOL(netdev_refcnt_read);
> static void netdev_wait_allrefs(struct net_device *dev)
> {
> unsigned long rebroadcast_time, warning_time;
> - int refcnt;
> + int wait = 0, refcnt;
>
> linkwatch_forget_dev(dev);
>
> @@ -10023,8 +10023,13 @@ static void netdev_wait_allrefs(struct net_device *dev)
> rebroadcast_time = jiffies;
> }
>
> - msleep(250);
> -
> + if (!wait) {
> + synchronize_rcu();
rcu_barrier();
> + wait = 1;
> + } else {
> + msleep(wait);
> + wait = min(wait << 1, 250);
> + }
> refcnt = netdev_refcnt_read(dev);
>
> if (refcnt && time_after(jiffies, warning_time + 10 * HZ)) {