Re: [PATCH net] net/mlx5e: Fix stalled channels with threaded NAPI

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new predicate in mlx5e_napi_poll() (en_txrx.c:216-217)
  uses NAPI_STATE_THREADED, which records the configured mode.
- [Low] The opposite transition has a smaller problem.

--

net/mlx5e: Fix stalled channels with threaded NAPI

With this patch, mlx5e_napi_poll() keeps polling when the NAPI is in
threaded mode, instead of taking the affinity-change path. The changelog
says that path completes the NAPI and arms the CQs while CQEs are still
pending, and this can leave a channel stalled after a burst of traffic
ends.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> index 1851056064698..3142e90e68cee 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> @@ -207,7 +207,14 @@ int mlx5e_napi_poll(struct napi_struct *napi, int budget)
>  				rq);
>  
>  	if (busy) {
> -		if (likely(mlx5e_channel_no_affinity_change(c))) {
> +		/* A NAPI thread runs where the scheduler puts it, which says
> +		 * nothing about the affinity of the interrupt. Completing here
> +		 * would leave the CQEs that are pending to the next interrupt,
> +		 * and arming the CQs raises none for those, only for a new
> +		 * completion.
> +		 */
> +		if (likely(mlx5e_channel_no_affinity_change(c)) ||
> +		    test_bit(NAPI_STATE_THREADED, &napi->state)) {

[Severity: Medium]

NAPI_STATE_THREADED records the configured mode. It does not say whether
the current poll is running in the NAPI kthread. Can a kthread poll still
see this bit clear?

When threaded mode is disabled, napi_set_threaded() calls
napi_stop_kthread(). That function clears THREADED while the kthread
still owns the NAPI, and only waits for SCHED_THREADED afterwards:

net/core/dev.c:napi_stop_kthread() {
    ...
		if ((val & NAPIF_STATE_SCHED_THREADED) ||
		    !(val & NAPIF_STATE_SCHED)) {
			new = val & (~(NAPIF_STATE_THREADED |
				       NAPIF_STATE_THREADED_BUSY_POLL));
    ...
	/* Once STATE_THREADED is unset, wait for SCHED_THREADED to be unset by
	 * the kthread.
	 */
	while (true) {
		if (!test_bit(NAPI_STATE_SCHED_THREADED, &napi->state))
			break;

		msleep(20);
	}
    ...
}

During that window, napi_threaded_poll_loop() keeps calling __napi_poll()
as long as repoll is set, and it never looks at THREADED.

Suppose a busy mlx5e poll runs in the kthread on a CPU outside
c->aff_mask, which the changelog describes as the usual case. It would
then fall through to the affinity-change branch:

	ch_stats->aff_change++;
	aff_change = true;
	if (work_done == budget)
		work_done--;

napi_complete_done() then clears SCHED and SCHED_THREADED. The CQs are
armed with CQEs still pending, and no interrupt is forced.

Isn't that the same state the changelog blames for the stall? If traffic
stops right then and the RQ is dry, the channel could stall the same way.

Would NAPI_STATE_SCHED_THREADED be a better test here? It marks that the
kthread owns the current poll.

[Severity: Low]

The opposite transition seems to have a smaller problem. When threaded
mode is enabled, napi_set_threaded() sets THREADED right away, even if
the NAPI is being polled in softirq at that moment:

net/core/dev.c:napi_set_threaded() {
    ...
	/* Setting/unsetting threaded mode on a napi might not immediately
	 * take effect, if the current napi instance is actively being
	 * polled. In this case, the switch between threaded mode and
	 * softirq mode will happen in the next round of napi_schedule().
    ...
		napi_set_threaded_state(napi, threaded);
    ...
}

____napi_schedule() checks THREADED only at schedule time. A NAPI that is
already on a softirq poll_list therefore keeps being repolled by
net_rx_action() with THREADED set.

Suppose the IRQ affinity changes around the same time, so the softirq
poll runs on a CPU outside c->aff_mask. The new test is then true, so
mlx5e_napi_poll() returns budget and the NAPI is repolled on the same
CPU.

That skips the complete-and-arm step. That step would move the poll to
the IRQ's new CPU, and the next schedule would hand the NAPI to the
kthread.

Does NAPI processing then stay on the old CPU, with the switch to
threaded mode put off until the queue goes idle? Testing
NAPI_STATE_SCHED_THREADED instead of NAPI_STATE_THREADED looks like it
would cover this case as well.

>  			work_done = budget;
>  			goto out;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021-b4-mlx5e-threaded-napi-stall-v1-1-d719c17b3ec4%40toxicpanda.com