Re: [PATCH net v2 5/5] selftests/xsk: fix multi-buffer invalid desc tests for drain_cont

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Premature exit in the newly added `wait_for_tx_completion` drain loop causes the test to silently strand valid packets.
- [High] Clamping `outstanding_tx` to 0 in `complete_pkts` degrades test strictness by silently discarding unexpected completions.
- [High] Unhandled `EOVERFLOW` error in `kick_tx()` causes spurious test failures when the kernel synchronously processes invalid packets.
--

commit 51fe3cee29ff3a261580a926424f8e6171690b0d
Author: Jason Xing <kerneljasonxing@gmail.com>

selftests/xsk: fix multi-buffer invalid desc tests for drain_cont

This commit updates the selftests to account for the kernel returning CQ
entries for dropped and drained multi-buffer descriptors. It clamps the
outstanding TX count, adds a drain loop to consume extra CQ entries, and
modifies test descriptors to stop drains at packet boundaries.

>     - wait_for_tx_completion: add a drain loop that consumes CQ entries
>       after outstanding_tx reaches zero. Ensure remaining valid packets
>       are transmitted. This change is made because of patch 3 in the
>       series adds a logic in __xsk_generic_xmit(): return -EOVERFLOW
>       after detecting and handling the remaining part of the skb.

Since the kernel now returns -EOVERFLOW after handling invalid packets,
will kick_tx() abort the test when it encounters this new error code?

kick_tx() is called by complete_pkts(), which checks its return value:

test_xsk.c:complete_pkts() {
    if (xsk_ring_prod__needs_wakeup(&xsk->tx)) {
        ret = kick_tx(xsk);
        if (ret)
            return TEST_FAILURE;
    }
    ...
}

If kick_tx() does not tolerate -EOVERFLOW, this might cause spurious test
failures when the kernel synchronously processes invalid packets.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index 7950c504ed289..488ace93ff663 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c

[ ... ]

> @@ -950,17 +950,11 @@ static int complete_pkts(struct xsk_socket_info *xsk, int batch_size)
>  
>  	rcvd = xsk_ring_cons__peek(&xsk->umem->cq, batch_size, &idx);
>  	if (rcvd) {
> -		if (rcvd > xsk->outstanding_tx) {
> -			u64 addr = *xsk_ring_cons__comp_addr(&xsk->umem->cq, idx + rcvd - 1);
> -
> -			ksft_print_msg("[%s] Too many packets completed\n", __func__);
> -			ksft_print_msg("Last completion address: %llx\n",
> -				       (unsigned long long)addr);
> -			return TEST_FAILURE;
> -		}
> -
>  		xsk_ring_cons__release(&xsk->umem->cq, rcvd);
> -		xsk->outstanding_tx -= rcvd;
> +		if (rcvd > xsk->outstanding_tx)
> +			xsk->outstanding_tx = 0;
> +		else
> +			xsk->outstanding_tx -= rcvd;

Does clamping xsk->outstanding_tx to 0 mask underlying regressions by
silently discarding unexpected completions?

If the kernel correctly generates CQ entries for invalid descriptors now,
could outstanding_tx be incremented by the total number of submitted
descriptors instead of only the valid ones?

Tracking total descriptors would allow the test to use the existing
timeout-backed wait loop and eliminate the need for this clamping.

>  	}
>  
>  	return TEST_PASS;

[ ... ]

> @@ -1293,6 +1289,14 @@ static int wait_for_tx_completion(struct xsk_socket_info *xsk)
>  		complete_pkts(xsk, xsk->batch_size);
>  	}
>  
> +	do {
> +		if (xsk_ring_prod__needs_wakeup(&xsk->tx))
> +			kick_tx(xsk);
> +		rcvd = xsk_ring_cons__peek(&xsk->umem->cq, xsk->batch_size, &idx);
> +		if (rcvd)
> +			xsk_ring_cons__release(&xsk->umem->cq, rcvd);
> +	} while (rcvd);

Can this loop exit prematurely and strand valid packets?

In asynchronous modes, kick_tx() wakes the NAPI thread and returns
immediately. Because packet processing happens asynchronously, the hardware
may not have populated the completion queue before the peek() occurs.

If the queue is not yet populated, xsk_ring_cons__peek() will return 0,
causing the loop to break instantly before the valid packets have actually
completed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515123018.80147-1-kerneljasonxing@gmail.com?part=5