Re: [PATCH net-next v3 4/4] selftests/xsk: add shared-UMEM callback framework and initial test cases

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index 814b8325493f..5d4eb47cbfee 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> @@ -224,6 +224,12 @@ int hw_ring_size_reset(struct ifobject *ifobj)
>  static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  			     struct ifobject *ifobj_rx)
>  {
> +	/*
> +	 * Keep the same default as xskxceiver startup: when TX and RX share the same netdev,
> +	 * shared UMEM is the baseline mode for this test harness. Individual tests can still
> +	 * override this as needed.
> +	 */
> +	bool shared_default = ifobj_tx->ifindex == ifobj_rx->ifindex;
>  	u32 i, j;
>
>  	for (i = 0; i < MAX_INTERFACES; i++) {
> @@ -235,6 +241,7 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  		ifobj->use_fill_ring = true;
>  		ifobj->release_rx = true;
>  		ifobj->validation_func = NULL;
> +		ifobj->shared_umem = shared_default;
>  		ifobj->use_metadata = false;

The shared_default derivation here can never be true in this harness,
can it?

prog_tests/xsk.c uses distinct veth pair ends (VETH_RX "veth0" /
VETH_TX "veth1"), and configure_ifobj() resolves two different
ifindexes. Nothing in test_xsk.c ever assigns ifobj->shared_umem =
true. The only other writer is testapp_poll_txq_tmout(), which merely
saves and restores the current value.

Consequently the umem->base_addr offsetting in xsk_configure_umem(),
the 'shared = (ifobject->shared_umem && tx)' branch in
xsk_configure(), and thread_common_ops_tx() remain unreachable.

The comment asserts the opposite: "when TX and RX share the same
netdev, shared UMEM is the baseline mode for this test harness." In
xskxceiver.c the equivalent derivation can be true because that
binary accepts '-i' twice with the same interface; the ported
harness has no such option, so the copied logic plus its three-line
justification describe behavior that cannot occur.

Should this assignment and comment be removed, or is there a plan to
add same-netdev configuration so the mode is actually exercised?

[ ... ]

> @@ -593,28 +600,95 @@ static int pkt_stream_receive_half(struct test_spec *test)
>  	return 0;
>  }
>
> +static int pkt_stream_replace_seq(struct test_spec *test, pkt_stream_dims_fn dims,
> +				  const void *ctx)
> +{
> +	struct pkt_stream *tx_streams[MAX_SOCKETS] = {};
> +	struct pkt_stream *rx_streams[MAX_SOCKETS] = {};
> +	u32 i, nb_pkts, pkt_len;
> +
> +	for (i = 0; i < test->nb_sockets; i++) {
> +		dims(test->ifobj_tx->xsk_arr[i].pkt_stream, i, ctx, &nb_pkts, &pkt_len);
> +		tx_streams[i] = __pkt_stream_generate(nb_pkts, pkt_len, i, 2);
> +		if (!tx_streams[i])
> +			goto err;
> +
> +		dims(test->ifobj_rx->xsk_arr[i].pkt_stream, i, ctx, &nb_pkts, &pkt_len);
> +		rx_streams[i] = __pkt_stream_generate(nb_pkts, pkt_len, i, 2);
> +		if (!rx_streams[i])
> +			goto err;
> +	}

The hardcoded stride of 2 in __pkt_stream_generate() is correct for the
pre-existing two-socket XDP_SHARED_UMEM test (it produces the even/odd
split), but is it also correct for the new SHARED_UMEM_4_SOCKETS case?

With four sockets the four generated sequences are not disjoint: socket 0
gets pkt_nb = {0,2,4,...,4094} and socket 2 gets pkt_nb = {2,4,...,4096};
socket 1 gets {1,3,...,4095} and socket 3 gets {3,5,...,4097}.

Because the payload written by pkt_generate()/write_payload() and
validated by is_frag_valid() is a function of pkt_nb only (the seqnum
word is '(pkt_nb << 16) | word_index'), a packet that leaks from TX
socket 0 to RX socket 2 (or 1 to 3, and vice versa) is indistinguishable
from the packet that socket really expected, and it is also received in
the expected order.

The only thing that would still catch cross-delivery between those pairs
is the aggregate per-socket count check in receive_pkts(), so the test
largely cannot detect the very misrouting it is meant to cover. The
changelog's '4-socket even/odd split' does not describe the generated
streams (there is no 4-way split).

Using test->nb_sockets as the stride argument would give each socket a
disjoint pkt_nb class while preserving current 2-socket behavior. Does
that make sense here?

[ ... ]

> +static void uneven_dist_dims(struct pkt_stream *orig, u32 sock_id, const void *ctx,
> +			     u32 *nb_pkts, u32 *pkt_len)
> +{
> +	const struct shared_umem_uneven_dist_ctx *cfg = ctx;
> +	u32 pkts_sock0 = cfg->total_pkts / 4;
> +
> +	*nb_pkts = sock_id ? cfg->total_pkts - pkts_sock0 : pkts_sock0;
> +	*pkt_len = cfg->pkt_len;
> +}

[ ... ]

> +static int pkt_stream_uneven_dist_seq(struct test_spec *test,
> +				      const struct shared_umem_uneven_dist_ctx *cfg)
> +{
> +	if (test->nb_sockets < 2 || cfg->total_pkts < 4)
> +		return -EINVAL;
> +
> +	return pkt_stream_replace_seq(test, uneven_dist_dims, cfg);
> +}

The guard validates nb_sockets < 2, but uneven_dist_dims() only
distinguishes socket 0 from "everything else":

	u32 pkts_sock0 = cfg->total_pkts / 4;
	*nb_pkts = sock_id ? cfg->total_pkts - pkts_sock0 : pkts_sock0;

So the 1:3 split it implements is only meaningful for exactly two
sockets. With nb_sockets == 3 or 4 every non-zero slot gets 3/4 *
total_pkts and the aggregate becomes (n - 1) * 3/4 + 1/4 of total_pkts
instead of total_pkts, silently contradicting the ctx the caller
supplied.

The check accepts precisely the configurations the callback cannot
express while rejecting nothing that matters (the only caller,
testapp_shared_umem_uneven_dist(), passes 2).

Should the condition be 'test->nb_sockets != 2', or should
uneven_dist_dims() distribute the remaining 3/4 across the remaining
sockets?

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32400920730