Re: [PATCH net-next v2 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] Using a fixed 10ms sleep to synchronize with a kernel workqueue introduces a race condition that can lead to flaky tests.
- [Low] Unescaped newline in string literal causes compilation failure.
--

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

selftests/bpf: Add RX latency tests for bpf timestamping

This patch extends the test_tcp() functionality to cover the new BPF RX
timestamping path. It adds new assertions, workqueue synchronization, and mixed
timestamping modes to verify SO_TIMESTAMPING and BPF RX can function together
without 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..e3fe850e4cc70 100644
> --- a/tools/testing/selftests/bpf/prog_tests/net_timestamping.c
> +++ b/tools/testing/selftests/bpf/prog_tests/net_timestamping.c

[ ... ]

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

[ ... ]

> +	if (!ASSERT_TRUE(tss != NULL, "SCM_TIMESTAMPING cmsg present") ||
> +	    !ASSERT_TRUE(tss->ts[0].tv_sec || tss->ts[0].tv_nsec, "rx sw
> +			 timestamp non-zero"))

[Severity: Low]
Does this unescaped newline in the string literal cause a compilation failure?

The error message string is split across lines without a trailing backslash or
string concatenation, which results in invalid syntax in C.

> +		return false;
> +
> +	return true;
> +}
> +
>  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);

[Severity: Medium]
Will this hardcoded 10ms sleep cause the test to become flaky?

Using fixed delays to synchronize with asynchronous kernel workqueues can lead
to race conditions. In slow VMs or heavily loaded CI systems, the workqueue
might take longer than 10ms to run, leaving skb->tstamp unpopulated and
causing the test to fail intermittently.

Could a polling mechanism with a reasonable timeout be used here instead?

> +
>  	if (enable_socket_timestamping) {
> +		unsigned int rx_opt;

[ ... ]

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