Re: [PATCH v6 2/2] selftests/bpf: add sockmap recvfrom EAGAIN selftest

[Emil Tsalapatis]

On Mon Jul 20, 2026 at 6:07 PM EDT, Nnamdi Onyeyiri wrote:
> On Mon, Jul 20, 2026 at 05:47:09PM -0400, Emil Tsalapatis wrote:
>> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
>> > These selftests exercise the tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser()
>> > functions, to ensure that they are properly handling spurious wakeups in
>> > tcp_msg_wait_data().
>> >
>> > The expected behaviour is that recvfrom() does not return an EAGAIN
>> > error.  If the spurious wakeups are incorrectly handled, this assertion
>> > will fail.
>> >
>> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@xxxxxxxxx>
>> 
>> The test looks fine, even if slightly flaky. Running with only patch 2/2
>> still passes sometimes on my box. Can we handle this somehow, e.g., do
>> more attempts?
>> 
>> There's also a couple magic numbers in the tests that may need some
>> explanation (noted below).
>>
>
> Thanks for the review!  I'll attach an explaination to the numbers.
> Essentially the larger the payload the fewer iterations seemed to be
> required to reproduce (on my machine).
>
> With the issue fixed though, larger payloads and more iterations make
> the test run longer.  Is there is a rule of thumb I should follow for
> tuning the runtime?

Unfortunately there's no fixed rule, I'd say the two requirements are:
a) The test should not be flakey, esp. false positives are a no-go.
b) The test shouldn't add noticeable latency to the testbench.

As it stands the test is fine, since the main issue is a false negative
(the test spuriously passes without the fix). That's still an issue, but
as long as the test without the fix properly fails the vast majority of
the time I think think it's fine.

>
>> > ---
>> >  .../selftests/bpf/prog_tests/sockmap_basic.c  | 124 ++++++++++++++++++
>> >  1 file changed, 124 insertions(+)
>> >
>> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>> > index cb3229711f93..d18faf46fac0 100644
>> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>> > @@ -1373,6 +1373,126 @@ static void test_sockmap_multi_channels(int sotype)
>> >  	test_sockmap_pass_prog__destroy(skel);
>> >  }
>> >  
>> > +static void *test_sockmap_recvfrom_eagain_thread(void *arg)
>> > +{
>> > +	int fd = *(int *)arg;
>> > +	char buf[1024];
>> > +	void *result = NULL;
>> > +
>> > +	while (true) {
>> > +		ssize_t len = recvfrom(fd, buf, sizeof(buf), 0, NULL, NULL);
>> > +
>> > +		if (len == -1) {
>> > +			if (errno == EINTR)
>> > +				continue;
>> > +			result = (void *)1;
>> > +			break;
>> > +		}
>> > +
>> > +		if (!len || buf[len - 1] == 'e')
>> > +			break;
>> > +	}
>> > +
>> > +	send(fd, "test", 4, MSG_NOSIGNAL);
>> > +
>> > +	close(fd);
>> > +
>> > +	return result;
>> > +}
>> > +
>> > +static void test_sockmap_recvfrom_eagain(bool with_verdict)
>> > +{
>> > +	struct test_sockmap_pass_prog *skel = NULL;
>> > +	struct bpf_program *prog = NULL;
>> > +	size_t buflen = 1024 * 1024 * 25;
>> 
>> Here
>> 
>> > +	char *buf = NULL;
>> > +	int map, err;
>> > +
>> > +	skel = test_sockmap_pass_prog__open_and_load();
>> > +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
>> > +		return;
>> > +
>> > +	map = bpf_map__fd(skel->maps.sock_map_msg);
>> > +
>> > +	if (with_verdict) {
>> > +		prog = skel->progs.prog_skb_verdict;
>> > +		err = bpf_prog_attach(bpf_program__fd(prog), map, BPF_SK_SKB_STREAM_VERDICT, 0);
>> > +		if (!ASSERT_OK(err, "bpf_prog_attach verdict"))
>> > +			goto cleanup;
>> > +	}
>> > +
>> > +	buf = malloc(buflen);
>> > +	if (!ASSERT_OK_PTR(buf, "malloc buf"))
>> > +		goto cleanup;
>> > +	memset(buf, 0, buflen);
>> > +	buf[buflen - 1] = 'e';
>> > +
>> > +	for (int i = 0; i < 200; ++i) {
>> 
>> Also here. Why 200 iterations specifically? Can we at least name the
>> defaults to make it clearer that we've chosen those numbers because
>> that's how we trigger the bug?
>> 
>> > +		ssize_t sent;
>> > +		char ignored[128];
>> > +		pthread_t thread;
>> > +		bool thread_created = false;
>> > +		size_t rem = buflen;
>> > +		int c = -1, p = -1, zero = 0;
>> > +		bool success = false;
>> > +
>> > +		err = create_pair(AF_INET, SOCK_STREAM, &c, &p);
>> > +		if (!ASSERT_OK(err, "create_pair"))
>> > +			goto end_attempt;
>> > +
>> > +		err = pthread_create(&thread, NULL, &test_sockmap_recvfrom_eagain_thread, &p);
>> > +		if (!ASSERT_OK(err, "pthread_create"))
>> > +			goto end_attempt;
>> > +		thread_created = true;
>> > +
>> > +		err = bpf_map_update_elem(map, &zero, &c, BPF_ANY);
>> > +		if (!ASSERT_OK(err, "bpf_map_update_elem"))
>> > +			goto end_attempt;
>> > +
>> > +		while (rem) {
>> > +			sent = xsend(c, buf + (buflen - rem), rem, 0);
>> > +			if (sent == -1)
>> > +				goto end_attempt;
>> > +			rem -= sent;
>> > +		}
>> > +
>> > +		/* we cannot use recv_timeout(), otherwise EAGAIN would be an expected errno. */
>> > +		err = recvfrom(c, ignored, sizeof(ignored), 0, NULL, NULL);
>> > +
>> > +		/*
>> > +		 * we are checking for the invalid return of EAGAIN, any other return is considered
>> > +		 * successful for the purposes of this test.
>> > +		 */
>> > +		if (err < 0 && !ASSERT_NEQ(errno, EAGAIN, "recvfrom eagain"))
>> > +			goto end_attempt;
>> > +
>> > +		success = true;
>> > +
>> > +end_attempt:
>> > +		if (c >= 0)
>> > +			close(c);
>> > +
>> > +		if (thread_created) {
>> > +			void *retval = NULL;
>> > +
>> > +			pthread_join(thread, &retval);
>> > +			if (!ASSERT_NULL(retval, "retval"))
>> > +				success = false;
>> > +		}
>> > +
>> > +		if (!thread_created && p >= 0)
>> > +			close(p);
>> > +		if (!success)
>> > +			break;
>> > +	}
>> > +
>> > +cleanup:
>> > +	if (buf)
>> > +		free(buf);
>> > +
>> > +	test_sockmap_pass_prog__destroy(skel);
>> > +}
>> > +
>> >  void test_sockmap_basic(void)
>> >  {
>> >  	if (test__start_subtest("sockmap create_update_free"))
>> > @@ -1451,4 +1571,8 @@ void test_sockmap_basic(void)
>> >  		test_sockmap_multi_channels(SOCK_STREAM);
>> >  	if (test__start_subtest("sockmap udp multi channels"))
>> >  		test_sockmap_multi_channels(SOCK_DGRAM);
>> > +	if (test__start_subtest("sockmap recvfrom eagain"))
>> > +		test_sockmap_recvfrom_eagain(false);
>> > +	if (test__start_subtest("sockmap recvfrom eagain with verdict"))
>> > +		test_sockmap_recvfrom_eagain(true);
>> >  }
>>