[PATCH net-next 1/2] net: transition netdev reg state earlier in run_todo

[PATCH net-next 1/2] net: transition netdev reg state earlier in run_todo

[Jakub Kicinski]

In prep for unregistering netdevs out of order move the netdev
state validation and change outside of the loop.

While at it modernize this code and use WARN() instead of
pr_err() + dump_stack().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 909fb3815910..2749776e2dd2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9906,6 +9906,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
  */
 void netdev_run_todo(void)
 {
+	struct net_device *dev, *tmp;
 	struct list_head list;
 #ifdef CONFIG_LOCKDEP
 	struct list_head unlink_list;
@@ -9926,24 +9927,23 @@ void netdev_run_todo(void)
 
 	__rtnl_unlock();
 
-
 	/* Wait for rcu callbacks to finish before next phase */
 	if (!list_empty(&list))
 		rcu_barrier();
 
-	while (!list_empty(&list)) {
-		struct net_device *dev
-			= list_first_entry(&list, struct net_device, todo_list);
-		list_del(&dev->todo_list);
-
+	list_for_each_entry_safe(dev, tmp, &list, todo_list) {
 		if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
-			pr_err("network todo '%s' but state %d\n",
-			       dev->name, dev->reg_state);
-			dump_stack();
+			netdev_WARN(dev, "run_todo but not unregistering\n");
+			list_del(&dev->todo_list);
 			continue;
 		}
 
 		dev->reg_state = NETREG_UNREGISTERED;
+	}
+
+	while (!list_empty(&list)) {
+		dev = list_first_entry(&list, struct net_device, todo_list);
+		list_del(&dev->todo_list);
 
 		netdev_wait_allrefs(dev);
 
-- 
2.34.1

[PATCH net-next 2/2] net: allow out-of-order netdev unregistration

[Jakub Kicinski]

Sprinkle for each loops to allow netdevices to be unregistered
out of order, as their refs are released.

This prevents problems caused by dependencies between netdevs
which want to release references in their ->priv_destructor.
See commit d6ff94afd90b ("vlan: move dev_put into vlan_dev_uninit")
for example.

Eric has removed the only known ordering requirement in
commit c002496babfd ("Merge branch 'ipv6-loopback'")
so let's try this and see if anything explodes...

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/dev.c | 64 +++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 2749776e2dd2..05fa867f1878 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9811,8 +9811,8 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
 #define WAIT_REFS_MIN_MSECS 1
 #define WAIT_REFS_MAX_MSECS 250
 /**
- * netdev_wait_allrefs - wait until all references are gone.
- * @dev: target net_device
+ * netdev_wait_allrefs_any - wait until all references are gone.
+ * @list: list of net_devices to wait on
  *
  * This is called when unregistering network devices.
  *
@@ -9822,37 +9822,45 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
  * We can get stuck here if buggy protocols don't correctly
  * call dev_put.
  */
-static void netdev_wait_allrefs(struct net_device *dev)
+static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
 {
 	unsigned long rebroadcast_time, warning_time;
-	int wait = 0, refcnt;
+	struct net_device *dev;
+	int wait = 0;
 
-	linkwatch_forget_dev(dev);
+	list_for_each_entry(dev, list, todo_list)
+		linkwatch_forget_dev(dev);
 
 	rebroadcast_time = warning_time = jiffies;
-	refcnt = netdev_refcnt_read(dev);
 
-	while (refcnt != 1) {
+	list_for_each_entry(dev, list, todo_list)
+		if (netdev_refcnt_read(dev) == 1)
+			return dev;
+
+	while (true) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_lock();
 
 			/* Rebroadcast unregister notification */
-			call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
+			list_for_each_entry(dev, list, todo_list)
+				call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
 
 			__rtnl_unlock();
 			rcu_barrier();
 			rtnl_lock();
 
-			if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
-				     &dev->state)) {
-				/* We must not have linkwatch events
-				 * pending on unregister. If this
-				 * happens, we simply run the queue
-				 * unscheduled, resulting in a noop
-				 * for this device.
-				 */
-				linkwatch_run_queue();
-			}
+			list_for_each_entry(dev, list, todo_list)
+				if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
+					     &dev->state)) {
+					/* We must not have linkwatch events
+					 * pending on unregister. If this
+					 * happens, we simply run the queue
+					 * unscheduled, resulting in a noop
+					 * for this device.
+					 */
+					linkwatch_run_queue();
+					break;
+				}
 
 			__rtnl_unlock();
 
@@ -9867,14 +9875,18 @@ static void netdev_wait_allrefs(struct net_device *dev)
 			wait = min(wait << 1, WAIT_REFS_MAX_MSECS);
 		}
 
-		refcnt = netdev_refcnt_read(dev);
+		list_for_each_entry(dev, list, todo_list)
+			if (netdev_refcnt_read(dev) == 1)
+				return dev;
 
-		if (refcnt != 1 &&
-		    time_after(jiffies, warning_time +
+		if (time_after(jiffies, warning_time +
 			       netdev_unregister_timeout_secs * HZ)) {
-			pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
-				 dev->name, refcnt);
-			ref_tracker_dir_print(&dev->refcnt_tracker, 10);
+			list_for_each_entry(dev, list, todo_list) {
+				pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
+					 dev->name, netdev_refcnt_read(dev));
+				ref_tracker_dir_print(&dev->refcnt_tracker, 10);
+			}
+
 			warning_time = jiffies;
 		}
 	}
@@ -9942,11 +9954,9 @@ void netdev_run_todo(void)
 	}
 
 	while (!list_empty(&list)) {
-		dev = list_first_entry(&list, struct net_device, todo_list);
+		dev = netdev_wait_allrefs_any(&list);
 		list_del(&dev->todo_list);
 
-		netdev_wait_allrefs(dev);
-
 		/* paranoia */
 		BUG_ON(netdev_refcnt_read(dev) != 1);
 		BUG_ON(!list_empty(&dev->ptype_all));
-- 
2.34.1

Re: [PATCH net-next 1/2] net: transition netdev reg state earlier in run_todo

[Eric Dumazet]

On Tue, Feb 15, 2022 at 2:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> In prep for unregistering netdevs out of order move the netdev
> state validation and change outside of the loop.
>
> While at it modernize this code and use WARN() instead of
> pr_err() + dump_stack().
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 2/2] net: allow out-of-order netdev unregistration

[Eric Dumazet]

On Tue, Feb 15, 2022 at 2:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Sprinkle for each loops to allow netdevices to be unregistered
> out of order, as their refs are released.
>
> This prevents problems caused by dependencies between netdevs
> which want to release references in their ->priv_destructor.
> See commit d6ff94afd90b ("vlan: move dev_put into vlan_dev_uninit")
> for example.
>
> Eric has removed the only known ordering requirement in
> commit c002496babfd ("Merge branch 'ipv6-loopback'")
> so let's try this and see if anything explodes...
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>

SGTM, thanks !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 1/2] net: transition netdev reg state earlier in run_todo

[Xin Long]

On Wed, Feb 16, 2022 at 6:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> In prep for unregistering netdevs out of order move the netdev
> state validation and change outside of the loop.
>
> While at it modernize this code and use WARN() instead of
> pr_err() + dump_stack().
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/dev.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 909fb3815910..2749776e2dd2 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9906,6 +9906,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
>   */
>  void netdev_run_todo(void)
>  {
> +       struct net_device *dev, *tmp;
>         struct list_head list;
>  #ifdef CONFIG_LOCKDEP
>         struct list_head unlink_list;
> @@ -9926,24 +9927,23 @@ void netdev_run_todo(void)
>
>         __rtnl_unlock();
>
> -
>         /* Wait for rcu callbacks to finish before next phase */
>         if (!list_empty(&list))
>                 rcu_barrier();
>
> -       while (!list_empty(&list)) {
> -               struct net_device *dev
> -                       = list_first_entry(&list, struct net_device, todo_list);
> -               list_del(&dev->todo_list);
> -
> +       list_for_each_entry_safe(dev, tmp, &list, todo_list) {
>                 if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) {
> -                       pr_err("network todo '%s' but state %d\n",
> -                              dev->name, dev->reg_state);
> -                       dump_stack();
> +                       netdev_WARN(dev, "run_todo but not unregistering\n");
> +                       list_del(&dev->todo_list);
>                         continue;
>                 }
>
>                 dev->reg_state = NETREG_UNREGISTERED;
> +       }
> +
> +       while (!list_empty(&list)) {
> +               dev = list_first_entry(&list, struct net_device, todo_list);
> +               list_del(&dev->todo_list);
>
>                 netdev_wait_allrefs(dev);
>
> --
> 2.34.1
>
Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH net-next 2/2] net: allow out-of-order netdev unregistration

[Xin Long]

On Wed, Feb 16, 2022 at 6:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Sprinkle for each loops to allow netdevices to be unregistered
> out of order, as their refs are released.
>
> This prevents problems caused by dependencies between netdevs
> which want to release references in their ->priv_destructor.
> See commit d6ff94afd90b ("vlan: move dev_put into vlan_dev_uninit")
> for example.
>
> Eric has removed the only known ordering requirement in
> commit c002496babfd ("Merge branch 'ipv6-loopback'")
> so let's try this and see if anything explodes...
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/dev.c | 64 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2749776e2dd2..05fa867f1878 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9811,8 +9811,8 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
>  #define WAIT_REFS_MIN_MSECS 1
>  #define WAIT_REFS_MAX_MSECS 250
>  /**
> - * netdev_wait_allrefs - wait until all references are gone.
> - * @dev: target net_device
> + * netdev_wait_allrefs_any - wait until all references are gone.
> + * @list: list of net_devices to wait on
>   *
>   * This is called when unregistering network devices.
>   *
> @@ -9822,37 +9822,45 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
>   * We can get stuck here if buggy protocols don't correctly
>   * call dev_put.
>   */
> -static void netdev_wait_allrefs(struct net_device *dev)
> +static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
>  {
>         unsigned long rebroadcast_time, warning_time;
> -       int wait = 0, refcnt;
> +       struct net_device *dev;
> +       int wait = 0;
>
> -       linkwatch_forget_dev(dev);
> +       list_for_each_entry(dev, list, todo_list)
> +               linkwatch_forget_dev(dev);
>
>         rebroadcast_time = warning_time = jiffies;
> -       refcnt = netdev_refcnt_read(dev);
>
> -       while (refcnt != 1) {
> +       list_for_each_entry(dev, list, todo_list)
> +               if (netdev_refcnt_read(dev) == 1)
> +                       return dev;
> +
> +       while (true) {
>                 if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
>                         rtnl_lock();
>
>                         /* Rebroadcast unregister notification */
> -                       call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
> +                       list_for_each_entry(dev, list, todo_list)
> +                               call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
>
>                         __rtnl_unlock();
>                         rcu_barrier();
>                         rtnl_lock();
>
> -                       if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
> -                                    &dev->state)) {
> -                               /* We must not have linkwatch events
> -                                * pending on unregister. If this
> -                                * happens, we simply run the queue
> -                                * unscheduled, resulting in a noop
> -                                * for this device.
> -                                */
> -                               linkwatch_run_queue();
> -                       }
> +                       list_for_each_entry(dev, list, todo_list)
> +                               if (test_bit(__LINK_STATE_LINKWATCH_PENDING,
> +                                            &dev->state)) {
> +                                       /* We must not have linkwatch events
> +                                        * pending on unregister. If this
> +                                        * happens, we simply run the queue
> +                                        * unscheduled, resulting in a noop
> +                                        * for this device.
> +                                        */
> +                                       linkwatch_run_queue();
> +                                       break;
> +                               }
>
>                         __rtnl_unlock();
>
> @@ -9867,14 +9875,18 @@ static void netdev_wait_allrefs(struct net_device *dev)
>                         wait = min(wait << 1, WAIT_REFS_MAX_MSECS);
>                 }
>
> -               refcnt = netdev_refcnt_read(dev);
> +               list_for_each_entry(dev, list, todo_list)
> +                       if (netdev_refcnt_read(dev) == 1)
> +                               return dev;
>
> -               if (refcnt != 1 &&
> -                   time_after(jiffies, warning_time +
> +               if (time_after(jiffies, warning_time +
>                                netdev_unregister_timeout_secs * HZ)) {
> -                       pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
> -                                dev->name, refcnt);
> -                       ref_tracker_dir_print(&dev->refcnt_tracker, 10);
> +                       list_for_each_entry(dev, list, todo_list) {
> +                               pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
> +                                        dev->name, netdev_refcnt_read(dev));
> +                               ref_tracker_dir_print(&dev->refcnt_tracker, 10);
> +                       }
> +
>                         warning_time = jiffies;
>                 }
>         }
> @@ -9942,11 +9954,9 @@ void netdev_run_todo(void)
>         }
>
>         while (!list_empty(&list)) {
> -               dev = list_first_entry(&list, struct net_device, todo_list);
> +               dev = netdev_wait_allrefs_any(&list);
>                 list_del(&dev->todo_list);
>
> -               netdev_wait_allrefs(dev);
> -
>                 /* paranoia */
>                 BUG_ON(netdev_refcnt_read(dev) != 1);
>                 BUG_ON(!list_empty(&dev->ptype_all));
> --
> 2.34.1
>
Reviewed-by: Xin Long <lucien.xin@gmail.com>

Thanks for the fix, I will revert d6ff94afd90b once this gets applied.

Re: [PATCH net-next 1/2] net: transition netdev reg state earlier in run_todo

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 15 Feb 2022 14:53:09 -0800 you wrote:
> In prep for unregistering netdevs out of order move the netdev
> state validation and change outside of the loop.
> 
> While at it modernize this code and use WARN() instead of
> pr_err() + dump_stack().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: transition netdev reg state earlier in run_todo
    https://git.kernel.org/netdev/net-next/c/ae68db14b616
  - [net-next,2/2] net: allow out-of-order netdev unregistration
    https://git.kernel.org/netdev/net-next/c/faab39f63c1f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 2/2] net: allow out-of-order netdev unregistration

[Eric Dumazet]

On Tue, Feb 15, 2022 at 2:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Sprinkle for each loops to allow netdevices to be unregistered
> out of order, as their refs are released.
>
> This prevents problems caused by dependencies between netdevs
> which want to release references in their ->priv_destructor.
> See commit d6ff94afd90b ("vlan: move dev_put into vlan_dev_uninit")
> for example.
>
> Eric has removed the only known ordering requirement in
> commit c002496babfd ("Merge branch 'ipv6-loopback'")
> so let's try this and see if anything explodes...
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/dev.c | 64 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2749776e2dd2..05fa867f1878 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9811,8 +9811,8 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
>  #define WAIT_REFS_MIN_MSECS 1
>  #define WAIT_REFS_MAX_MSECS 250
>  /**
> - * netdev_wait_allrefs - wait until all references are gone.
> - * @dev: target net_device
> + * netdev_wait_allrefs_any - wait until all references are gone.
> + * @list: list of net_devices to wait on
>   *
>   * This is called when unregistering network devices.
>   *
> @@ -9822,37 +9822,45 @@ int netdev_unregister_timeout_secs __read_mostly = 10;
>   * We can get stuck here if buggy protocols don't correctly
>   * call dev_put.
>   */
> -static void netdev_wait_allrefs(struct net_device *dev)
> +static struct net_device *netdev_wait_allrefs_any(struct list_head *list)
>  {
>         unsigned long rebroadcast_time, warning_time;
> -       int wait = 0, refcnt;
> +       struct net_device *dev;
> +       int wait = 0;
>
> -       linkwatch_forget_dev(dev);


> +       list_for_each_entry(dev, list, todo_list)
> +               linkwatch_forget_dev(dev);

Jakub, this part of the code added quadratic behavior (and soft lockups)
when a large list of devices is dismantled at once,
because we call netdev_wait_allrefs_any() N times.

I will test this fix, unless I have missed something ?

diff --git a/net/core/dev.c b/net/core/dev.c
index 05fa867f18787e709dcaccfea1df350c424eff80..74e77f861e2c99127b0349aa0f8a4be412cc187e
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9828,9 +9828,6 @@ static struct net_device
*netdev_wait_allrefs_any(struct list_head *list)
        struct net_device *dev;
        int wait = 0;

-       list_for_each_entry(dev, list, todo_list)
-               linkwatch_forget_dev(dev);
-
        rebroadcast_time = warning_time = jiffies;

        list_for_each_entry(dev, list, todo_list)
@@ -9953,6 +9950,9 @@ void netdev_run_todo(void)
                dev->reg_state = NETREG_UNREGISTERED;
        }

+       list_for_each_entry(dev, &list, todo_list)
+               linkwatch_forget_dev(dev);
+
        while (!list_empty(&list)) {
                dev = netdev_wait_allrefs_any(&list);
                list_del(&dev->todo_list);