Re: [PATCH bpf-next] selftests/bpf: Handle EAGAIN in bpf_tcp_ca

[Martin KaFai Lau <martin.lau@linux.dev>]

On 3/28/24 10:57 PM, Geliang Tang wrote:
> Hi Martin,
> 
> On Thu, 2024-03-28 at 09:55 -0700, Martin KaFai Lau wrote:
>> On 3/28/24 3:23 AM, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> bpf_tcp_ca tests may emit EAGAIN sometimes. In that case, tests
>>> fail with
>>> "bytes != total_bytes" errors. Sending should continue, not break
>>> when
>>> errno is EAGAIN. This patch can make bpf_tcp_ca tests stable.
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>    tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>>> b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>>> index 077b107130f6..fbc219c2d53b 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
>>> @@ -56,7 +56,7 @@ static void *server(void *arg)
>>>    	while (bytes < total_bytes && !READ_ONCE(stop)) {
>>>    		nr_sent = send(fd, &batch,
>>>    			       MIN(total_bytes - bytes,
>>> sizeof(batch)), 0);
>>> -		if (nr_sent == -1 && errno == EINTR)
>>> +		if (nr_sent == -1 && (errno == EINTR || errno ==
>>> EAGAIN))
>>
>> This is a non blocking socket. EAGAIN is hitting the timeout
>> situation?
>>
>> The default timeout is 3s and it has not been changed after the
>> recent
>> connect_fd_to_fd and start_server simplifications. I don't find bpf
>> CI failing
>> in this test in the last month also.
>>
>> I would prefer to fail after timeout instead of keep retrying. Do you
>> really hit
>> that in your environment for this specific bpf_tcp_ca test? There are
>> many tests
>> using this timeout value also.
> 
> This is the 2nd patch of "refactor mptcp bpf tests" series:
> 
> https://patchwork.kernel.org/project/mptcp/cover/cover.1711688054.git.tanggeliang@kylinos.cn/ >
> I didn't get the mentioned EAGAIN errors in bpf_tcp_ca tests, but got
> them in MPTCP BPF sched tests (see patch 1). MPTCP BPF sched tests (not
> upstream yet) use the same sending and receiving functions as
> bpf_tcp_ca tests (patch 15). So it makes sense to add this fix for
> bpf_tcp_ca tests too.

It sounds like the EAGAIN is specific to mptcp sched test and is not due to a 
timeout? Did you try to increase the timeout and see if it resolves the issue?

afaik, there is no fix needed for the bpf_tcp_ca test. bpf CI expects the test 
to finish. If the default 3s turns out to be too flaky for most tests, it could 
be increased instead of loop testing EAGAIN because the bpf CI expects the test 
to finish. However, so far I don't see this (i.e. 3s is too short) to be the 
case from looking at one month old data in bpf CI.


> And here's another reason. I want to move these functions from
> bpf_tcp_ca into network_helpers as public ones (patch 4), which can be
> used by both bpf_tcp_ca and MPTCP BPF sched tests. So we must add this
> fix to the public ones too.

Lets understand why mptcp sched test hits EAGAIN first.

> 
> Maybe the commit log of this patch needs to be updated. Or I should
> send patches 2, 3 and 4 together to bpf-next?

All selftests/bpf changes have to pass bpf CI. It is always a good idea to 
target bpf-next to kick off the bpf CI to bar any surprise later when it got 
merged into bpf-next.

Unrelated, since we are in the mptcp bpf test, one thing needs to fix in 
test_mptcpify(). The mptcpify test is upgrading a IPPROTO_TCP socket to 
IPPROTO_MPTCP socket. This could break other tests when the test_progs is run in 
parallel (test_progs "-j" which fork() to run prog_tests/* in parallel). It 
could unexpectedly upgrade the tcp socket of another test. One option is to 
limit the upgrade by checking the pid in the mptcpify.c bpf prog.