[PATCH v2 net-next 0/4] net: reduce RTNL pressure in unregister_netdevice()

[PATCH v2 net-next 0/4] net: reduce RTNL pressure in unregister_netdevice()

[Eric Dumazet]

One major source of RTNL contention resides in unregister_netdevice()

Due to RCU protection of various network structures, and
unregister_netdevice() being a synchronous function,
it is calling potentially slow functions while holding RTNL.

I think we can release RTNL in two points, so that three
slow functions are called while RTNL can be used
by other threads.

v2: Only temporarily release RTNL from cleanup_net()
v1: https://lore.kernel.org/netdev/20250107130906.098fc8d6@kernel.org/T/#m398c95f5778e1ff70938e079d3c4c43c050ad2a6

Eric Dumazet (4):
  net: expedite synchronize_net() for cleanup_net()
  net: no longer assume RTNL is held in flush_all_backlogs()
  net: no longer hold RTNL while calling flush_all_backlogs()
  net: reduce RTNL hold duration in unregister_netdevice_many_notify()

 include/net/net_namespace.h |  2 +
 net/core/dev.c              | 78 +++++++++++++++++++++++++++----------
 net/core/net_namespace.c    |  5 +++
 3 files changed, 65 insertions(+), 20 deletions(-)

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 net-next 1/4] net: expedite synchronize_net() for cleanup_net()

[Eric Dumazet]

cleanup_net() is the single thread responsible
for netns dismantles, and a serious bottleneck.

Before we can get per-netns RTNL, make sure
all synchronize_net() called from this thread
are using rcu_synchronize_expedited().

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/net_namespace.h | 2 ++
 net/core/dev.c              | 7 ++++++-
 net/core/net_namespace.c    | 5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 5a2a0df8ad91b677b515b392869c6c755be5c868..a3009bdd7efec0a3b665cbf51c159c323458410a 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -565,4 +565,6 @@ void net_ns_init(void);
 static inline void net_ns_init(void) {}
 #endif
 
+extern struct task_struct *cleanup_net_task;
+
 #endif /* __NET_NET_NAMESPACE_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index efbe2c4d94580a2ce9401adb85784c9c1c862df9..76ad68b129eed0407686e8696102aeed9a8b30ec 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11415,6 +11415,11 @@ struct net_device *alloc_netdev_dummy(int sizeof_priv)
 }
 EXPORT_SYMBOL_GPL(alloc_netdev_dummy);
 
+static bool from_cleanup_net(void)
+{
+	return current == cleanup_net_task;
+}
+
 /**
  *	synchronize_net -  Synchronize with packet receive processing
  *
@@ -11424,7 +11429,7 @@ EXPORT_SYMBOL_GPL(alloc_netdev_dummy);
 void synchronize_net(void)
 {
 	might_sleep();
-	if (rtnl_is_locked())
+	if (from_cleanup_net() || rtnl_is_locked())
 		synchronize_rcu_expedited();
 	else
 		synchronize_rcu();
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index b5cd3ae4f04cf28d43f8401a3dafebac4a297123..cb39a12b2f8295c605f08b5589932932150a1644 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -588,6 +588,8 @@ static void unhash_nsid(struct net *net, struct net *last)
 
 static LLIST_HEAD(cleanup_list);
 
+struct task_struct *cleanup_net_task;
+
 static void cleanup_net(struct work_struct *work)
 {
 	const struct pernet_operations *ops;
@@ -596,6 +598,8 @@ static void cleanup_net(struct work_struct *work)
 	LIST_HEAD(net_exit_list);
 	LIST_HEAD(dev_kill_list);
 
+	cleanup_net_task = current;
+
 	/* Atomically snapshot the list of namespaces to cleanup */
 	net_kill_list = llist_del_all(&cleanup_list);
 
@@ -670,6 +674,7 @@ static void cleanup_net(struct work_struct *work)
 		put_user_ns(net->user_ns);
 		net_free(net);
 	}
+	cleanup_net_task = NULL;
 }
 
 /**
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 net-next 1/4] net: expedite synchronize_net() for cleanup_net()

[kernel test robot]

Hi Eric,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Dumazet/net-expedite-synchronize_net-for-cleanup_net/20250109-002516
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250108162255.1306392-2-edumazet%40google.com
patch subject: [PATCH v2 net-next 1/4] net: expedite synchronize_net() for cleanup_net()
config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20250109/202501092210.1YCfipFl-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501092210.1YCfipFl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501092210.1YCfipFl-lkp@intel.com/

All errors (new ones prefixed by >>):

   or1k-linux-ld: net/core/dev.o: in function `synchronize_net':
>> dev.c:(.text+0x22c8): undefined reference to `cleanup_net_task'
>> or1k-linux-ld: dev.c:(.text+0x22d0): undefined reference to `cleanup_net_task'
   or1k-linux-ld: net/core/dev.o: in function `dev_remove_pack':
   dev.c:(.text+0x7058): undefined reference to `cleanup_net_task'
   or1k-linux-ld: dev.c:(.text+0x7060): undefined reference to `cleanup_net_task'
   or1k-linux-ld: net/core/dev.o: in function `free_netdev':
   dev.c:(.text+0x7130): undefined reference to `cleanup_net_task'
   or1k-linux-ld: net/core/dev.o:dev.c:(.text+0x713c): more undefined references to `cleanup_net_task' follow

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 net-next 1/4] net: expedite synchronize_net() for cleanup_net()

[kernel test robot]

Hi Eric,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Dumazet/net-expedite-synchronize_net-for-cleanup_net/20250109-002516
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250108162255.1306392-2-edumazet%40google.com
patch subject: [PATCH v2 net-next 1/4] net: expedite synchronize_net() for cleanup_net()
config: arm-sama5_defconfig (https://download.01.org/0day-ci/archive/20250109/202501092222.Kn4MEI0i-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250109/202501092222.Kn4MEI0i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501092222.Kn4MEI0i-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: net/core/dev.o: in function `synchronize_net':
   dev.c:(.text+0x1484): undefined reference to `cleanup_net_task'
>> arm-linux-gnueabi-ld: dev.c:(.text+0x1488): undefined reference to `cleanup_net_task'
   arm-linux-gnueabi-ld: net/core/dev.o: in function `dev_remove_pack':
   dev.c:(.text+0x4fa8): undefined reference to `cleanup_net_task'
   arm-linux-gnueabi-ld: dev.c:(.text+0x4fac): undefined reference to `cleanup_net_task'
   arm-linux-gnueabi-ld: net/core/dev.o: in function `free_netdev':
   dev.c:(.text+0x5054): undefined reference to `cleanup_net_task'
   arm-linux-gnueabi-ld: net/core/dev.o:dev.c:(.text+0x5058): more undefined references to `cleanup_net_task' follow

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 net-next 2/4] net: no longer assume RTNL is held in flush_all_backlogs()

[Eric Dumazet]

flush_all_backlogs() uses per-cpu and static data to hold its
temporary data, on the assumption it is called under RTNL
protection.

Following patch in the series will break this assumption.

Use instead a dynamically allocated piece of memory.

In the unlikely case the allocation fails,
use a boot-time allocated memory.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 53 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 76ad68b129eed0407686e8696102aeed9a8b30ec..8ff288cf25dceb5856496388f83f409fcb6f8e5d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5978,8 +5978,6 @@ void netif_receive_skb_list(struct list_head *head)
 }
 EXPORT_SYMBOL(netif_receive_skb_list);
 
-static DEFINE_PER_CPU(struct work_struct, flush_works);
-
 /* Network device is going away, flush any packets still pending */
 static void flush_backlog(struct work_struct *work)
 {
@@ -6036,36 +6034,54 @@ static bool flush_required(int cpu)
 	return true;
 }
 
+struct flush_backlogs {
+	cpumask_t		flush_cpus;
+	struct work_struct	w[];
+};
+
+static struct flush_backlogs *flush_backlogs_alloc(void)
+{
+	return kmalloc(struct_size_t(struct flush_backlogs, w, nr_cpu_ids),
+		       GFP_KERNEL);
+}
+
+static struct flush_backlogs *flush_backlogs_fallback;
+static DEFINE_MUTEX(flush_backlogs_mutex);
+
 static void flush_all_backlogs(void)
 {
-	static cpumask_t flush_cpus;
+	struct flush_backlogs *ptr = flush_backlogs_alloc();
 	unsigned int cpu;
 
-	/* since we are under rtnl lock protection we can use static data
-	 * for the cpumask and avoid allocating on stack the possibly
-	 * large mask
-	 */
-	ASSERT_RTNL();
+	if (!ptr) {
+		mutex_lock(&flush_backlogs_mutex);
+		ptr = flush_backlogs_fallback;
+	}
+	cpumask_clear(&ptr->flush_cpus);
 
 	cpus_read_lock();
 
-	cpumask_clear(&flush_cpus);
 	for_each_online_cpu(cpu) {
 		if (flush_required(cpu)) {
-			queue_work_on(cpu, system_highpri_wq,
-				      per_cpu_ptr(&flush_works, cpu));
-			cpumask_set_cpu(cpu, &flush_cpus);
+			INIT_WORK(&ptr->w[cpu], flush_backlog);
+			queue_work_on(cpu, system_highpri_wq, &ptr->w[cpu]);
+			__cpumask_set_cpu(cpu, &ptr->flush_cpus);
 		}
 	}
 
 	/* we can have in flight packet[s] on the cpus we are not flushing,
 	 * synchronize_net() in unregister_netdevice_many() will take care of
-	 * them
+	 * them.
 	 */
-	for_each_cpu(cpu, &flush_cpus)
-		flush_work(per_cpu_ptr(&flush_works, cpu));
+	for_each_cpu(cpu, &ptr->flush_cpus)
+		flush_work(&ptr->w[cpu]);
 
 	cpus_read_unlock();
+
+	if (ptr != flush_backlogs_fallback)
+		kfree(ptr);
+	else
+		mutex_unlock(&flush_backlogs_mutex);
 }
 
 static void net_rps_send_ipi(struct softnet_data *remsd)
@@ -12259,12 +12275,13 @@ static int __init net_dev_init(void)
 	 *	Initialise the packet receive queues.
 	 */
 
+	flush_backlogs_fallback = flush_backlogs_alloc();
+	if (!flush_backlogs_fallback)
+		goto out;
+
 	for_each_possible_cpu(i) {
-		struct work_struct *flush = per_cpu_ptr(&flush_works, i);
 		struct softnet_data *sd = &per_cpu(softnet_data, i);
 
-		INIT_WORK(flush, flush_backlog);
-
 		skb_queue_head_init(&sd->input_pkt_queue);
 		skb_queue_head_init(&sd->process_queue);
 #ifdef CONFIG_XFRM_OFFLOAD
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 net-next 3/4] net: no longer hold RTNL while calling flush_all_backlogs()

[Eric Dumazet]

flush_all_backlogs() is called from unregister_netdevice_many_notify()
as part of netdevice dismantles.

This is currently called under RTNL, and can last up to 20ms on busy hosts.

There is no reason to hold RTNL at this stage, if our caller
is cleanup_net() : netns are no more visible, devices
are in NETREG_UNREGISTERING state and no other thread
could mess our state while RTNL is temporarily released.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8ff288cf25dceb5856496388f83f409fcb6f8e5d..86fa9ae29d31a25dca8204c75fd39974ef84707d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11436,6 +11436,18 @@ static bool from_cleanup_net(void)
 	return current == cleanup_net_task;
 }
 
+static void rtnl_drop_if_cleanup_net(void)
+{
+	if (from_cleanup_net())
+		__rtnl_unlock();
+}
+
+static void rtnl_acquire_if_cleanup_net(void)
+{
+	if (from_cleanup_net())
+		rtnl_lock();
+}
+
 /**
  *	synchronize_net -  Synchronize with packet receive processing
  *
@@ -11548,8 +11560,10 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		unlist_netdevice(dev);
 		WRITE_ONCE(dev->reg_state, NETREG_UNREGISTERING);
 	}
-	flush_all_backlogs();
 
+	rtnl_drop_if_cleanup_net();
+	flush_all_backlogs();
+	rtnl_acquire_if_cleanup_net();
 	synchronize_net();
 
 	list_for_each_entry(dev, head, unreg_list) {
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 net-next 4/4] net: reduce RTNL hold duration in unregister_netdevice_many_notify()

[Eric Dumazet]

Two synchronize_net() calls are currently done while holding RTNL.

This is source of RTNL contention in workloads adding and deleting
many network namespaces per second, because synchronize_rcu()
and synchronize_rcu_expedited() can use 10+ ms in some cases.

For cleanup_net() use, temporarily release RTNL
while calling synchronize_net().

This should be safe, because devices are no longer visible
to other threads after the call to unlist_netdevice(),
and the netns are in dismantle phase.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 86fa9ae29d31a25dca8204c75fd39974ef84707d..dde5d40e8f6b1ad5309ebc6a6e163ddb96f66f73 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11563,8 +11563,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
 
 	rtnl_drop_if_cleanup_net();
 	flush_all_backlogs();
-	rtnl_acquire_if_cleanup_net();
 	synchronize_net();
+	rtnl_acquire_if_cleanup_net();
 
 	list_for_each_entry(dev, head, unreg_list) {
 		struct sk_buff *skb = NULL;
@@ -11624,7 +11624,9 @@ void unregister_netdevice_many_notify(struct list_head *head,
 #endif
 	}
 
+	rtnl_drop_if_cleanup_net();
 	synchronize_net();
+	rtnl_acquire_if_cleanup_net();
 
 	list_for_each_entry(dev, head, unreg_list) {
 		netdev_put(dev, &dev->dev_registered_tracker);
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 net-next 0/4] net: reduce RTNL pressure in unregister_netdevice()

[Eric Dumazet]

On Wed, Jan 8, 2025 at 5:22 PM Eric Dumazet <edumazet@google.com> wrote:
>
> One major source of RTNL contention resides in unregister_netdevice()
>
> Due to RCU protection of various network structures, and
> unregister_netdevice() being a synchronous function,
> it is calling potentially slow functions while holding RTNL.
>
> I think we can release RTNL in two points, so that three
> slow functions are called while RTNL can be used
> by other threads.
>
> v2: Only temporarily release RTNL from cleanup_net()
> v1: https://lore.kernel.org/netdev/20250107130906.098fc8d6@kernel.org/T/#m398c95f5778e1ff70938e079d3c4c43c050ad2a6

Hmmm. more work is needed, because __rtnl_unlock() might see a

 WARN_ON(!list_empty(&net_todo_list));

I will send a v3 later.