Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] In tester_run(), the new comment `/* Sample before teardown can
produce additional echo traffic.
Pre-existing issues:
- [Medium] In this commit, dut_run() spins forever if the tester closes
the control connection without sending CMD_STOP.
--
selftests/bpf: Fix XDP feature test setup and teardown
This patch fixes setup and teardown in the XDP features test tool. It
initializes the DUT state and socket address lengths, returns the echo
socket fd to the caller, and joins the echo worker only if it was
started. It also reads the tester counters before sending CMD_STOP, so
the teardown echo does not cause a false XDP_DROP failure.
> diff --git a/tools/testing/selftests/bpf/xdp_features.c
> b/tools/testing/selftests/bpf/xdp_features.c
> index a27ed663967c4..5b802b14d8589 100644
> --- a/tools/testing/selftests/bpf/xdp_features.c
> +++ b/tools/testing/selftests/bpf/xdp_features.c
[ ... ]
> @@ -484,7 +488,9 @@ static int dut_run(struct xdp_features *skel)
> }
>
> end_thread:
> - pthread_join(dut_thread, NULL);
> + exiting = true;
> + if (thread_started)
> + pthread_join(dut_thread, NULL);
> out:
> bpf_xdp_detach(env.ifindex, flags, NULL);
> close(ctrl_sockfd);
[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The
commit message says "Stop the worker on error paths". Can the new
exiting = true and pthread_join() be reached if the tester closes the
control connection without sending CMD_STOP?
At EOF, recv() in recv_msg() returns 0. That gets the same -EINVAL as a
malformed message:
len = recv(sockfd, buf, bufsize, 0);
if (len != ntohs(tlv->len) || len < sizeof(*tlv))
return -EINVAL;
dut_run() then just loops again:
err = recv_msg(ctrl_sockfd, buf, BUFSIZE, NULL, 0);
if (err)
continue;
exiting is still false, so the next recv() returns 0 at once. The DUT
spins at full CPU and never reaches end_thread or out. Meanwhile the
echo worker keeps running and the XDP program stays attached until
SIGINT or SIGTERM.
After CMD_START, tester_run() can close the socket without sending
CMD_STOP in several ways:
- send_echo_msg() fails.
- send_and_recv_msg(CMD_GET_STATS) fails or times out.
- The tester is killed.
The later patch in this series, "selftests/bpf: Send XDP feature probes
with AF_PACKET", appears to fix this. In that patch, recv_msg() returns
-ECONNRESET on a short or zero-length read. dut_run() then jumps to
end_thread for any error other than -EAGAIN or -EINTR.
[ ... ]
> @@ -636,12 +642,14 @@ static int tester_run(struct xdp_features *skel)
> if (err)
> goto out;
>
> + /* Sample before teardown can produce additional echo traffic. */
[Severity: Low]
This isn't a bug, but the comment reads as if sampling causes the extra
echo traffic, when it is the teardown that does. Would something like
"Sample before teardown, which can produce additional echo traffic" be
clearer?
Also, moving the detected_cap assignment leaves two blank lines in a row
after send_echo_msg() below. checkpatch warns about this.
Both go away when the later patch "selftests/bpf: Send XDP feature
probes with AF_PACKET" rewrites tester_run().
> + detected_cap = tester_collect_detected_cap(skel, ntohl(stats));
> +
> /* stop the test */
> err = send_and_recv_msg(sockfd, CMD_STOP, NULL, 0);
> /* send a new echo message to wake echo thread of the dut */
> send_echo_msg();
>
> - detected_cap = tester_collect_detected_cap(skel, ntohl(stats));
>
> fprintf(stdout, "Feature %s: [%s][%s]\n", get_xdp_feature_str(),
> detected_cap ? GREEN("DETECTED") : RED("NOT DETECTED"),
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010-xdp-v5-send-20261008-v5-0-de687cb67c40%40gmail.com
