Re: [PATCH net-next 6/6] selftests/bpf: Add RX latency tests for bpf timestamping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The return values of `ASSERT_TRUE` are ignored in `recv_verify_rx_timestamp`, causing it to unconditionally return `true` on successful `recvmsg`.
- [Medium] Using a fixed `usleep(10000)` to wait for a workqueue to run is prone to test flakiness on heavily loaded CI systems.
--

commit 3f1fdeab36943dce908082eb563628a5f8ca353b
Author: Jason Xing <kerneljasonxing@gmail.com>

selftests/bpf: Add RX latency tests for bpf timestamping

This commit extends the test_tcp() selftest to cover the new BPF RX
timestamping path. It tests a mixed mode where SO TX/RX and BPF TX/RX
timestamping operate simultaneously, verifying they do not conflict.

> diff --git a/tools/testing/selftests/bpf/prog_tests/net_timestamping.c b/tools/testing/selftests/bpf/prog_tests/net_timestamping.c
> index dbfd87499b6b5..3cc52b670e74b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/net_timestamping.c
> +++ b/tools/testing/selftests/bpf/prog_tests/net_timestamping.c

[ ... ]

> @@ -143,11 +143,45 @@ static void test_socket_timestamping(int fd)
>  	SK_TS_ACK = 0;
>  }
>  
> +static bool recv_verify_rx_timestamp(int fd, char *buf, int len)
> +{

[ ... ]

> +	for (cm = CMSG_FIRSTHDR(&msg); cm; cm = CMSG_NXTHDR(&msg, cm)) {
> +		if (cm->cmsg_level == SOL_SOCKET &&
> +		    cm->cmsg_type == SCM_TIMESTAMPING) {
> +			tss = (void *)CMSG_DATA(cm);
> +			break;
> +		}
> +	}
> +
> +	ASSERT_TRUE(tss != NULL, "SCM_TIMESTAMPING cmsg present");
> +	ASSERT_TRUE(tss && (tss->ts[0].tv_sec || tss->ts[0].tv_nsec),
> +		    "rx sw timestamp non-zero");
> +	return true;
> +}

Since the return values of ASSERT_TRUE() are ignored here, will this
function unconditionally return true even if the assertions fail?

If the caller expects a false return to abort early, this could cause the
test to proceed regardless of the assertion results.

[ ... ]

> @@ -187,7 +221,27 @@ static void test_tcp(int family, bool enable_socket_timestamping)

[ ... ]

> +	afd = accept(sfd, NULL, NULL);
> +	if (!ASSERT_OK_FD(afd, "accept"))
> +		goto out;
> +
> +	/* net_enable_timestamp() defers the static key update via
> +	 * schedule_work() when CONFIG_JUMP_LABEL is set. Give the
> +	 * workqueue a chance to run so that netstamp_needed_key is
> +	 * active and skb->tstamp gets populated in the receive path.
> +	 */
> +	usleep(10000);

Could this fixed 10ms sleep cause test flakiness on heavily loaded CI
systems?

If the workqueue is delayed for longer than 10ms, the static key might not
be active in time, which would leave skb->tstamp unpopulated and cause the
test to fail intermittently.

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