[PATCH] io_uring/napi: drop the single NAPI busy poll special case

[PATCH] io_uring/napi: drop the single NAPI busy poll special case

[Cinq Prairie]

io_napi_blocking_busy_loop() has a special case for a single registered
NAPI: it passes io_napi_busy_loop_should_end() as loop_end to
napi_busy_loop_rcu() and runs its own loop only once, relying on the
inner __napi_busy_loop() to spin until that predicate says to stop.

napi_busy_loop_rcu() sets NAPI_F_END_ON_RESCHED, so __napi_busy_loop()
also returns when need_resched() becomes true. io_uring treats that
return as the end of the busy-poll period and falls through to
schedule_hrtimeout() for the remainder of the wait timeout. The queue
is then serviced by the gro_flush_timeout softirq timer instead of
being actively polled (when napi_defer_hard_irqs/gro_flush_timeout are
set).

With two or more NAPIs, the inner loop runs for a single iteration and
io_uring's outer loop controls the busy-poll period, honoring
busy_poll_to. As a result, the same registration behaves differently
depending on whether one or multiple NAPIs are registered.

Remove the special case so a single NAPI follows the same path as
multiple NAPIs: the inner loop runs for a single iteration, while
io_uring continues busy polling until a completion, a signal or the
deadline. Since commit df8599ee18c0 ("io_uring/napi: cap busy_poll_to
10 msec"), the deadline is at most 10 ms on every path, so this does not
extend the busy-poll period beyond what the multi-NAPI path already
allows.

First observed on a 5.14-based distribution kernel carrying the 6.13
io_uring NAPI backport, reproduced on io_uring-7.4 at 9c207aa58e54,
x86-64, HZ=1000, PREEMPT_DYNAMIC (lazy), no CPU isolation, low RX
traffic, r8169 with one RX queue, prefer_busy_poll, busy_poll_to = wait
timeout = 10 ms, with a SCHED_FIFO task pinned to the polling CPU
waking every 1 ms:

  before, 1 NAPI:  busy 0.5-1.5 ms per 10 ms wait, ~0.8M polls/s, CPU ~5%
  before, 2 NAPIs: busy 10 ms per 10 ms wait,      ~7.2M polls/s, CPU ~100%
  after,  1 NAPI:  busy 10 ms per 10 ms wait,      ~7.4M polls/s, CPU ~100%

Fixes: 8d0c12a80cde ("io-uring: add napi busy poll support")
Cc: stable@vger.kernel.org
Signed-off-by: Cinq Prairie <notprairie@gmail.com>
---
How the numbers were taken:
  - bpftrace on entry/return of __io_napi_busy_loop() to measure busy
  time per wait period
  - bpftrace on napi:napi_poll tracepoint to count poll rate
  - bpftrace on the return value of io_napi_busy_loop_should_end() to
  see whether the loop ended on io_uring's own condition. Before the
  patch, with one NAPI, it never returned true and every exit came from
  __napi_busy_loop() breaking on need_resched().
Code and raw data available on request.

The asymmetry was found because registering a second dummy NAPI id
(0xFFFFFFFF, which napi_by_id() never matches) made the busy period
honor busy_poll_to again. That is what we run as a workaround today.

On the original arm64/CentOs Stream 9/ena system (isolated CPU,
nohz_full) the trigger was most likely RCU's urgent quiescent-state
resched_cpu() (not traced) rather than a wakeup, resulting in ~97% of a
1 s busy_poll_to loss.

Alternatives considered: re-entering the singular loop after an early
return is not viable, as __napi_busy_loop() restarts its deadline on
entry and would overshoot busy_poll_to. Keeping the singular fast path
and falling back to the single-shot loop only after an early return
works, but leaves two code paths for a case whose only benefit was the
one it was broken for. Happy to switch if preferred.

Only static tracking was tested. Dynamic tracking goes through the same
loop and is unchanged.

 io_uring/napi.c | 38 ++++++++++----------------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/io_uring/napi.c b/io_uring/napi.c
index 7e698720cd51..5362690a9d17 100644
--- a/io_uring/napi.c
+++ b/io_uring/napi.c
@@ -166,28 +166,24 @@ static bool io_napi_busy_loop_should_end(void *data,
 /*
  * never report stale entries
  */
-static bool static_tracking_do_busy_loop(struct io_ring_ctx *ctx,
-					 bool (*loop_end)(void *, unsigned long),
-					 void *loop_end_arg)
+static bool static_tracking_do_busy_loop(struct io_ring_ctx *ctx)
 {
 	struct io_napi_entry *e;
 
 	list_for_each_entry_rcu(e, &ctx->napi_list, list)
-		napi_busy_loop_rcu(e->napi_id, loop_end, loop_end_arg,
+		napi_busy_loop_rcu(e->napi_id, NULL, NULL,
 				   ctx->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
 	return false;
 }
 
 static bool
-dynamic_tracking_do_busy_loop(struct io_ring_ctx *ctx,
-			      bool (*loop_end)(void *, unsigned long),
-			      void *loop_end_arg)
+dynamic_tracking_do_busy_loop(struct io_ring_ctx *ctx)
 {
 	struct io_napi_entry *e;
 	bool is_stale = false;
 
 	list_for_each_entry_rcu(e, &ctx->napi_list, list) {
-		napi_busy_loop_rcu(e->napi_id, loop_end, loop_end_arg,
+		napi_busy_loop_rcu(e->napi_id, NULL, NULL,
 				   ctx->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
 
 		if (time_after(jiffies, READ_ONCE(e->timeout)))
@@ -198,15 +194,13 @@ dynamic_tracking_do_busy_loop(struct io_ring_ctx *ctx,
 }
 
 static inline bool
-__io_napi_do_busy_loop(struct io_ring_ctx *ctx,
-		       bool (*loop_end)(void *, unsigned long),
-		       void *loop_end_arg)
+__io_napi_do_busy_loop(struct io_ring_ctx *ctx)
 {
 	switch (READ_ONCE(ctx->napi_track_mode)) {
 	case IO_URING_NAPI_TRACKING_STATIC:
-		return static_tracking_do_busy_loop(ctx, loop_end, loop_end_arg);
+		return static_tracking_do_busy_loop(ctx);
 	case IO_URING_NAPI_TRACKING_DYNAMIC:
-		return dynamic_tracking_do_busy_loop(ctx, loop_end, loop_end_arg);
+		return dynamic_tracking_do_busy_loop(ctx);
 	default:
 		return false;
 	}
@@ -216,24 +210,12 @@ static void io_napi_blocking_busy_loop(struct io_ring_ctx *ctx,
 				       struct io_wait_queue *iowq)
 {
 	unsigned long start_time = busy_loop_current_time();
-	bool (*loop_end)(void *, unsigned long) = NULL;
-	void *loop_end_arg = NULL;
 	bool is_stale = false;
 
-	/* Singular lists use a different napi loop end check function and are
-	 * only executed once.
-	 */
-	if (list_is_singular(&ctx->napi_list)) {
-		loop_end = io_napi_busy_loop_should_end;
-		loop_end_arg = iowq;
-	}
-
 	scoped_guard(rcu) {
 		do {
-			is_stale = __io_napi_do_busy_loop(ctx, loop_end,
-							  loop_end_arg);
-		} while (!io_napi_busy_loop_should_end(iowq, start_time) &&
-			 !loop_end_arg);
+			is_stale = __io_napi_do_busy_loop(ctx);
+		} while (!io_napi_busy_loop_should_end(iowq, start_time));
 	}
 
 	io_napi_remove_stale(ctx, is_stale);
@@ -403,7 +385,7 @@ int io_napi_sqpoll_busy_poll(struct io_ring_ctx *ctx)
 		return 0;
 
 	scoped_guard(rcu) {
-		is_stale = __io_napi_do_busy_loop(ctx, NULL, NULL);
+		is_stale = __io_napi_do_busy_loop(ctx);
 	}
 
 	io_napi_remove_stale(ctx, is_stale);

base-commit: 9c207aa58e54ce14723361e4aaad16975ad64dc8
-- 
2.54.0

Re: [PATCH] io_uring/napi: drop the single NAPI busy poll special case

[Jens Axboe]

On 10/25/26 5:29 PM, Cinq Prairie wrote:
> io_napi_blocking_busy_loop() has a special case for a single registered
> NAPI: it passes io_napi_busy_loop_should_end() as loop_end to
> napi_busy_loop_rcu() and runs its own loop only once, relying on the
> inner __napi_busy_loop() to spin until that predicate says to stop.
> 
> napi_busy_loop_rcu() sets NAPI_F_END_ON_RESCHED, so __napi_busy_loop()
> also returns when need_resched() becomes true. io_uring treats that
> return as the end of the busy-poll period and falls through to
> schedule_hrtimeout() for the remainder of the wait timeout. The queue
> is then serviced by the gro_flush_timeout softirq timer instead of
> being actively polled (when napi_defer_hard_irqs/gro_flush_timeout are
> set).
> 
> With two or more NAPIs, the inner loop runs for a single iteration and
> io_uring's outer loop controls the busy-poll period, honoring
> busy_poll_to. As a result, the same registration behaves differently
> depending on whether one or multiple NAPIs are registered.
> 
> Remove the special case so a single NAPI follows the same path as
> multiple NAPIs: the inner loop runs for a single iteration, while
> io_uring continues busy polling until a completion, a signal or the
> deadline. Since commit df8599ee18c0 ("io_uring/napi: cap busy_poll_to
> 10 msec"), the deadline is at most 10 ms on every path, so this does not
> extend the busy-poll period beyond what the multi-NAPI path already
> allows.
> 
> First observed on a 5.14-based distribution kernel carrying the 6.13
> io_uring NAPI backport, reproduced on io_uring-7.4 at 9c207aa58e54,
> x86-64, HZ=1000, PREEMPT_DYNAMIC (lazy), no CPU isolation, low RX
> traffic, r8169 with one RX queue, prefer_busy_poll, busy_poll_to = wait
> timeout = 10 ms, with a SCHED_FIFO task pinned to the polling CPU
> waking every 1 ms:
> 
>   before, 1 NAPI:  busy 0.5-1.5 ms per 10 ms wait, ~0.8M polls/s, CPU ~5%
>   before, 2 NAPIs: busy 10 ms per 10 ms wait,      ~7.2M polls/s, CPU ~100%
>   after,  1 NAPI:  busy 10 ms per 10 ms wait,      ~7.4M polls/s, CPU ~100%

Claude had a few comments on this one, mostly related to now
ending up with a full start/stop cycle for every poll. Details
below:

> diff --git a/io_uring/napi.c b/io_uring/napi.c
> index 7e698720cd51..5362690a9d17 100644
> --- a/io_uring/napi.c
> +++ b/io_uring/napi.c
> @@ -166,28 +166,24 @@ static bool io_napi_busy_loop_should_end(void *data,

[ ... ]

>  	list_for_each_entry_rcu(e, &ctx->napi_list, list)
> -		napi_busy_loop_rcu(e->napi_id, loop_end, loop_end_arg,
> +		napi_busy_loop_rcu(e->napi_id, NULL, NULL,
>  				   ctx->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
>  	return false;
>  }

[ ... ]

> @@ -216,24 +210,12 @@ static void io_napi_blocking_busy_loop(struct io_ring_ctx *ctx,
>  				       struct io_wait_queue *iowq)
>  {
>  	unsigned long start_time = busy_loop_current_time();
> -	bool (*loop_end)(void *, unsigned long) = NULL;
> -	void *loop_end_arg = NULL;
>  	bool is_stale = false;
>  
> -	/* Singular lists use a different napi loop end check function and are
> -	 * only executed once.
> -	 */
> -	if (list_is_singular(&ctx->napi_list)) {
> -		loop_end = io_napi_busy_loop_should_end;
> -		loop_end_arg = iowq;
> -	}
> -
>  	scoped_guard(rcu) {
>  		do {
> -			is_stale = __io_napi_do_busy_loop(ctx, loop_end,
> -							  loop_end_arg);
> -		} while (!io_napi_busy_loop_should_end(iowq, start_time) &&
> -			 !loop_end_arg);
> +			is_stale = __io_napi_do_busy_loop(ctx);
> +		} while (!io_napi_busy_loop_should_end(iowq, start_time));
>  	}

Does dropping the single-NAPI path trade the early need_resched() exit
for a full busy poll start/stop cycle on every poll?

With loop_end set, __napi_busy_loop() acquires the NAPI once, spins in
its own loop, and only calls busy_poll_stop() when it is done. With
loop_end NULL it breaks out right after the first poll and tears the
busy poll state down each time:

net/core/dev.c:__napi_busy_loop() {
    ...
		if (!loop_end || loop_end(loop_end_arg, start_time))
			break;
    ...
	}
	if (napi_poll)
		busy_poll_stop(napi, have_poll_lock, flags, budget);
    ...
}

So after this change every iteration of the do/while above re-acquires
the NAPI with the cmpxchg on napi->state, polls once, and then runs
busy_poll_stop(). Without prefer_busy_poll that is a second
napi->poll() call just to re-enable the device interrupt:

net/core/dev.c:busy_poll_stop() {
    ...
	} else {
		/* Use driver poll to re-enable device interrupts. */
		rc = napi->poll(napi, budget);
    ...
}

so the interrupt is re-enabled after every poll, and once it fires and
schedules the NAPI the next cmpxchg in __napi_busy_loop() fails and that
iteration polls nothing. With prefer_busy_poll and gro_flush_timeout set
it is instead an hrtimer_start() and a clear of NAPI_STATE_SCHED per
poll.

The multi-NAPI path already behaves this way, but a single registered
NAPI is the case the tight loop was there to serve. The before numbers
in the message all cover the early exit case, so the cost of the fast
path when it is not cut short does not show up in them. Is losing that
path acceptable, given this also goes to stable?

The notes below the --- line say keeping the singular fast path and
falling back to the single-shot loop only after an early return "works,
but leaves two code paths for a case whose only benefit was the one it
was broken for". Avoiding the per-poll busy_poll_stop() looks like a
benefit that is independent of the need_resched() exit, so would the
fallback approach be the better fix here?

The notes also say re-entering the singular loop is not viable because
__napi_busy_loop() restarts its deadline on entry. It does take
start_time at entry:

net/core/dev.c:__napi_busy_loop() {
	unsigned long start_time = loop_end ? busy_loop_current_time() : 0;
    ...
}

but io_napi_busy_loop_should_end() is io_uring's own callback, so could
it check an absolute deadline stored in iowq instead of the start_time
that net core passes in? That would let the inner loop be re-entered for
the remaining budget after a need_resched() exit without overshooting
busy_poll_to.

This isn't a bug, but with the special case gone neither the single nor
the multi NAPI path checks need_resched() at all for the whole budget,
and the only preemption point is the preempt_enable() between polls
inside __napi_busy_loop(). On a kernel without preemption that is up to
10 ms with no cond_resched(). Should the message mention that the
single-NAPI case now stops yielding to a woken task on those
configurations?

-- 
Jens Axboe