[PATCH net-next] net: stop napi kthreads when THREADED napi is disabled

[PATCH net-next] net: stop napi kthreads when THREADED napi is disabled

[Samiullah Khawaja]

Once the THREADED napi is disabled, the napi kthread should also be
stopped. Keeping the kthread intact after disabling THREADED napi makes
the PID of this kthread show up in the output of netlink 'napi-get' and
ps -ef output.

This was discussed in the following patch:
https://lore.kernel.org/all/20250502191548.559cc416@kernel.org

The napi THREADED state should be unset before stopping the thread. This
makes sure that this napi will not be scheduled again for threaded
polling (SCHED_THREADED). In the napi_thread_wait function we need to
make sure that the SCHED_THREADED is not set before stopping.  Once the
SCHED_THREADED is unset it means that the napi kthread doesn't own this
napi and it is safe to stop it.

Add a new test in nl_netdev to verify this behaviour.

Tested:
 ./tools/testing/selftests/net/nl_netdev.py
 TAP version 13
 1..6
 ok 1 nl_netdev.empty_check
 ok 2 nl_netdev.lo_check
 ok 3 nl_netdev.page_pool_check
 ok 4 nl_netdev.napi_list_check
 ok 5 nl_netdev.dev_set_threaded
 ok 6 nl_netdev.nsim_rxq_reset_down
 # Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
Reviewed-by: Wei Wang <weiwan@google.com>
---
 net/core/dev.c                           | 30 ++++++++++++++++++-
 tools/testing/selftests/net/nl_netdev.py | 38 ++++++++++++++++++++++--
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d0563ddff6ca..fec2dee9533f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6888,6 +6888,18 @@ static enum hrtimer_restart napi_watchdog(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
+static void dev_stop_napi_threads(struct net_device *dev)
+{
+	struct napi_struct *napi;
+
+	list_for_each_entry(napi, &dev->napi_list, dev_list) {
+		if (napi->thread) {
+			kthread_stop(napi->thread);
+			napi->thread = NULL;
+		}
+	}
+}
+
 int dev_set_threaded(struct net_device *dev, bool threaded)
 {
 	struct napi_struct *napi;
@@ -6926,6 +6938,12 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
 	list_for_each_entry(napi, &dev->napi_list, dev_list)
 		assign_bit(NAPI_STATE_THREADED, &napi->state, threaded);
 
+	/* Calling kthread_stop on napi threads should be safe now as the
+	 * threaded state is disabled.
+	 */
+	if (!threaded)
+		dev_stop_napi_threads(dev);
+
 	return err;
 }
 EXPORT_SYMBOL(dev_set_threaded);
@@ -7451,7 +7469,8 @@ static int napi_thread_wait(struct napi_struct *napi)
 {
 	set_current_state(TASK_INTERRUPTIBLE);
 
-	while (!kthread_should_stop()) {
+	/* Wait until we are scheduled or asked to stop. */
+	while (true) {
 		/* Testing SCHED_THREADED bit here to make sure the current
 		 * kthread owns this napi and could poll on this napi.
 		 * Testing SCHED bit is not enough because SCHED bit might be
@@ -7463,6 +7482,15 @@ static int napi_thread_wait(struct napi_struct *napi)
 			return 0;
 		}
 
+		/* Since the SCHED_THREADED is not set so we do not own this
+		 * napi and it is safe to stop here if we are asked to. Checking
+		 * the SCHED_THREADED before stopping here makes sure that this
+		 * napi was not schedule again while napi threaded was being
+		 * disabled.
+		 */
+		if (kthread_should_stop())
+			break;
+
 		schedule();
 		set_current_state(TASK_INTERRUPTIBLE);
 	}
diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
index beaee5e4e2aa..c9109627a741 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -2,8 +2,9 @@
 # SPDX-License-Identifier: GPL-2.0
 
 import time
+from os import system
 from lib.py import ksft_run, ksft_exit, ksft_pr
-from lib.py import ksft_eq, ksft_ge, ksft_busy_wait
+from lib.py import ksft_eq, ksft_ge, ksft_ne, ksft_busy_wait
 from lib.py import NetdevFamily, NetdevSimDev, ip
 
 
@@ -34,6 +35,39 @@ def napi_list_check(nf) -> None:
                 ksft_eq(len(napis), 100,
                         comment=f"queue count after reset queue {q} mode {i}")
 
+def dev_set_threaded(nf) -> None:
+    """
+    Test that verifies various cases of napi threaded
+    set and unset at device level using sysfs.
+    """
+    with NetdevSimDev(queue_count=2) as nsimdev:
+        nsim = nsimdev.nsims[0]
+
+        ip(f"link set dev {nsim.ifname} up")
+
+        napis = nf.napi_get({'ifindex': nsim.ifindex}, dump=True)
+        ksft_eq(len(napis), 2)
+
+        napi0_id = napis[0]['id']
+        napi1_id = napis[1]['id']
+
+        # set threaded
+        system(f"echo 1 > /sys/class/net/{nsim.ifname}/threaded")
+
+        # check napi threaded is set for both napis
+        napi0 = nf.napi_get({'id': napi0_id})
+        ksft_ne(napi0.get('pid'), None)
+        napi1 = nf.napi_get({'id': napi1_id})
+        ksft_ne(napi1.get('pid'), None)
+
+        # unset threaded
+        system(f"echo 0 > /sys/class/net/{nsim.ifname}/threaded")
+
+        # check napi threaded is unset for both napis
+        napi0 = nf.napi_get({'id': napi0_id})
+        ksft_eq(napi0.get('pid'), None)
+        napi1 = nf.napi_get({'id': napi1_id})
+        ksft_eq(napi1.get('pid'), None)
 
 def nsim_rxq_reset_down(nf) -> None:
     """
@@ -122,7 +156,7 @@ def page_pool_check(nf) -> None:
 def main() -> None:
     nf = NetdevFamily()
     ksft_run([empty_check, lo_check, page_pool_check, napi_list_check,
-              nsim_rxq_reset_down],
+              dev_set_threaded, nsim_rxq_reset_down],
              args=(nf, ))
     ksft_exit()
 

base-commit: b65999e7238e6f2a48dc77c8c2109c48318ff41b
-- 
2.49.0.1045.g170613ef41-goog

Re: [PATCH net-next] net: stop napi kthreads when THREADED napi is disabled

[Jakub Kicinski]

On Tue, 13 May 2025 00:26:31 +0000 Samiullah Khawaja wrote:
> @@ -7463,6 +7482,15 @@ static int napi_thread_wait(struct napi_struct *napi)
>  			return 0;
>  		}
>  
> +		/* Since the SCHED_THREADED is not set so we do not own this
> +		 * napi and it is safe to stop here if we are asked to. Checking
> +		 * the SCHED_THREADED before stopping here makes sure that this
> +		 * napi was not schedule again while napi threaded was being
> +		 * disabled.
> +		 */

Not sure if this works:

          CPU 0 (IRQ)             CPU 1 (NAPI thr)          CPU 2 (config)

  ____napi_schedule()
    if (test_bit(NAPI_STATE_THREADED))

                                                         clear_bit(NAPI_STATE_THREADED)
                                                         kthread_stop(thread)

                              if (test_bit(NAPI_STATE_SCHED_THREADED))
                                   ..
                              if (kthread_should_stop())
                                   exit

       set_bit(NAPI_STATE_SCHED_THREADED)
       wake_up_process(thread);



> +		if (kthread_should_stop())
> +			break;
> +
>  		schedule();
>  		set_current_state(TASK_INTERRUPTIBLE);

Re: [PATCH net-next] net: stop napi kthreads when THREADED napi is disabled

[Samiullah Khawaja]

On Tue, May 13, 2025 at 5:08 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 13 May 2025 00:26:31 +0000 Samiullah Khawaja wrote:
> > @@ -7463,6 +7482,15 @@ static int napi_thread_wait(struct napi_struct *napi)
> >                       return 0;
> >               }
> >
> > +             /* Since the SCHED_THREADED is not set so we do not own this
> > +              * napi and it is safe to stop here if we are asked to. Checking
> > +              * the SCHED_THREADED before stopping here makes sure that this
> > +              * napi was not schedule again while napi threaded was being
> > +              * disabled.
> > +              */
>
> Not sure if this works:
>
>           CPU 0 (IRQ)             CPU 1 (NAPI thr)          CPU 2 (config)
>
>   ____napi_schedule()
>     if (test_bit(NAPI_STATE_THREADED))
>
>                                                          clear_bit(NAPI_STATE_THREADED)
>                                                          kthread_stop(thread)
>
>                               if (test_bit(NAPI_STATE_SCHED_THREADED))
>                                    ..
>                               if (kthread_should_stop())
>                                    exit
>
>        set_bit(NAPI_STATE_SCHED_THREADED)
Thanks for pointing this out. I didn't consider this, I'll sort it out.
>        wake_up_process(thread);
>
>
>
> > +             if (kthread_should_stop())
> > +                     break;
> > +
> >               schedule();
> >               set_current_state(TASK_INTERRUPTIBLE);