[PATCH net] net: yield the CPU on every exit of the threaded NAPI poll loop

[PATCH net] net: yield the CPU on every exit of the threaded NAPI poll loop

[Vitaliy Sochnev]

napi_threaded_poll_loop() only reaches cond_resched() when it is about to
iterate again. When __napi_poll() clears repoll the loop breaks first, so
the last iteration returns without any voluntary preemption point.

Control then goes back to napi_threaded_poll(), which calls
napi_thread_wait(). If work is already pending that helper returns without
ever calling schedule(). Under a receive load that keeps arriving at least
as fast as it is drained, this repeats indefinitely and the kthread holds
its CPU without a single reschedule. On CONFIG_PREEMPT_NONE nothing else
on that CPU gets to run.

Everything that waits for deferred work on that CPU then blocks: RCU grace
periods, per-CPU work items and RCU callbacks. Deleting a network device
hits all three - synchronize_net(), flush_all_backlogs() -> flush_work()
and rcu_barrier() from netdev_run_todo() - which is how this was found.
The backlog kthread runs the same loop via run_backlog_napi(), so it can
be held off in the same way.

rcu_softirq_qs_periodic() does not help here. It reports a quiescent state
but does not schedule, so the work items and the callbacks still wait, and
it is skipped on the exit path anyway.

Yield on both exits. rcu_softirq_qs_periodic() keeps its place on the
iterating path, where the RCU annotation is what is needed.

Measured on a Nokia XG-040G-MF (Airoha AN7583, dual core Cortex-A53,
CONFIG_PREEMPT_NONE, HZ=100, airoha_eth with threaded NAPI) while the
board terminates a 985 Mbit/s TCP receive load. 60 minute runs, timing
"ip link del" of a dummy interface:

                    before        after
  mean               1.22 s       0.10 s
  worst             60.32 s       0.16 s
  over 1 s        7 of 172      0 of 179
  RCU stalls             2            0
  receive       985 Mbit/s   984 Mbit/s
  packet rate   82094 p/s    82027 p/s

The packet rate is the control: the same work is done in both runs, so
the difference is not a lighter load.

The test kernel was 6.18, where this loop has no busy_poll_last_qs
parameter. With that pointer NULL the two versions of the loop are the
same code - the initialiser falls back to jiffies, the gro_flush_normal()
call and the write-back are skipped, and the tail condition reduces to
"if (repoll)" - so the change under test is this one. The busy-poll path
is not covered by these runs; this patch does not alter it, as
cond_resched() was already reached there.

An earlier variant that only moved the quiescent-state report, without
changing where the CPU is yielded, was not enough: the delay simply
migrated from synchronize_net() to flush_work() and rcu_barrier().

Without the patch the kernel reports the thread holding the CPU:

  rcu: INFO: rcu_sched self-detected stall on CPU
  rcu:  0-....: (5999 ticks this GP) ... (t=6001 jiffies g=50657 q=400)
  CPU: 0 UID: 0 PID: 241 Comm: napi/qdma_eth-0 Tainted: G W O 6.18.41 #0
  Tainted: [W]=WARN, [O]=OOT_MODULE
  Hardware name: Nokia XG-040G-MF (DT)
  pc : gro_receive_skb+0x0/0x1b8
  lr : gro_cell_poll+0x58/0xc0

Reproducing it needs the loop to be re-entered tens of thousands of times
a second, so it depends on the driver and on the shape of the load. It
did not reproduce on mtk_eth_soc, whose net_dim moderation coalesces the
same packet rate into far fewer interrupts, nor at loads where the queue
drains completely on each poll and the thread sleeps.

The taint is an out-of-tree GPIO button module and an earlier PHY warning,
both unrelated to the networking path above.

Fixes: 29863d41bb6e ("net: implement threaded-able napi poll loop support")
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
 net/core/dev.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ece6700536d9..884aa2dcadbe 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7876,11 +7876,14 @@ static void napi_threaded_poll_loop(struct napi_struct *napi,
 			gro_flush_normal(&napi->gro, HZ >= 1000);
 		local_bh_enable();
 
-		/* Call cond_resched here to avoid watchdog warnings. */
-		if (repoll || busy_poll_last_qs) {
+		if (repoll || busy_poll_last_qs)
 			rcu_softirq_qs_periodic(last_qs);
-			cond_resched();
-		}
+
+		/* Yield on every exit from the loop, not only when it iterates:
+		 * napi_thread_wait() can return without scheduling, so a thread
+		 * that keeps finding work would never give up the CPU.
+		 */
+		cond_resched();
 
 		if (!repoll)
 			break;
-- 
2.55.0