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

["Björn Töpel" <bjorn@kernel.org>]

Josef Bacik <josef@toxicpanda.com> writes:

> With threaded NAPI an mlx5e channel can stop receiving when a burst of
> traffic ends. It stays dead for minutes on an idle machine, while the
> connections hashed to it time out.
>
> When mlx5e_napi_poll() has more to do, it checks that it runs on a CPU of
> its interrupt's affinity mask. If it does not, it takes the affinity to
> have been changed: it completes the NAPI after at most budget - 1 packets
> and arms the CQs, so that the next interrupt moves the poll to the new CPU.
>
> A NAPI thread runs wherever the scheduler puts it, so with threaded NAPI a
> busy queue takes this path at nearly every poll. That costs an interrupt
> for every 64 packets. Worse, it counts on an interrupt for the CQEs that
> are still in the CQ when it is armed, and none comes: the next event is
> raised by the next new completion. While packets keep arriving this goes
> unnoticed. When a flood stops, what is in the CQ stays there, and is worked
> off one poll of 64 packets per new arrival. If the RQ has run dry as well,
> arrivals are dropped (rx_out_of_buffer) and bring no completion either.
>
> Seen on a 50 Gbit/s port (firmware 22.47.1088) with 63 channels, threaded
> NAPI and rx-usecs 64: a CPU-bound task bound to each of the 128 CPUs and
> 32 Gbit/s of UDP in 8 flows for 7 seconds, then both stopped. After the
> second round one channel had 0 polls and 0 events for 150 seconds on an
> idle machine, while TCP connections hashed to it timed out and
> rx_out_of_buffer counted their SYNs. Later arrivals took it 16 polls and
> 1018 packets further, it stood still again for 14 seconds, and another 93
> polls and 5917 packets emptied it. In a longer test 22 of 48 iperf3 runs
> failed with "unable to connect stream: Connection timed out", with no ping
> to the same address lost.
>
> i40e and iavf force an interrupt at this point, and mlx5e does for XSK.
> Doing that here would cure the stall and keep the interrupt per 64 packets.
>
> Fix by going on polling when the NAPI is threaded. With that, 8 of 8
> rounds leave every channel alive, ch_aff_change stays 0, rx_out_of_buffer
> does not move once the flood is over, none of the 48 runs fails, and the
> interrupts taken during the flood halve.
>
> Fixes: 29863d41bb6e ("net: implement threaded-able napi poll loop support")
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> I hit this while comparing threaded NAPI with softirq on two bare metal
> machines.  The runs in the changelog are from a 7.3-rc5 based kernel, with
> and without this change.  The patch applies to net unchanged, there I've
> only compiled it.
>
> I don't have the PRM, so "arming the CQ raises no event for the CQEs that
> are already there" is what the counters showed and not something I checked
> against the documentation.
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_txrx.c
> index 185105606469..3142e90e68ce 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)) {

Interesting. I wonder if more drivers hit this -- I think I've seen
similar affinity checks elsewhere.

I'd move this into a helper to hide these NAPI details. Regardless,

Reviewed-by: Björn Töpel <bjorn@kernel.org>