[PATCH bpf v3 0/2] selftest/bpf: make test_tc_tunnel and test_tc_edt more robust to CI load

[PATCH bpf v3 0/2] selftest/bpf: make test_tc_tunnel and test_tc_edt more robust to CI load

[Alexis Lothoré (eBPF Foundation)]

Hello,
this is the v3 of the series aiming to reduce the flakyness of
tc_tunnel/tc_edt tests in CI. This revision handles Ihor's feedback
about the need to conform to the timeout value passed in opts->timeout.
The first commit has been reorganized a bit more than what was needed
for this change, to make sure that the code can be followed easily.
There's also now a second commit to make sure that test_tc_edt benefits
from this new connect() handling.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
Changes in v3:
- set errno before logging errors
- respect time budget set by
- respect opts->timeout_ms when polling: only poll for the remaining
  time not already consume by connect()
- keep polling if poll returns with EINTR
- reorder early returns and add intermediate variables to clarify code
  flow
- Link to v2: https://patch.msgid.link/20260803-tc_tunnel_flaky-v2-1-657b287dfa75@bootlin.com

Changes in v2:
- drop unneeded initialization
- add back error message for immediate connection failure, and slightly
  reword the async connection failure error message
- Link to v1: https://patch.msgid.link/20260710-tc_tunnel_flaky-v1-1-42aab5399a49@bootlin.com

To: Alexei Starovoitov <ast@kernel.org>
To: Daniel Borkmann <daniel@iogearbox.net>
To: Andrii Nakryiko <andrii@kernel.org>
To: Eduard Zingerman <eddyz87@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
To: Song Liu <song@kernel.org>
To: Yonghong Song <yonghong.song@linux.dev>
To: Jiri Olsa <jolsa@kernel.org>
To: Emil Tsalapatis <emil@etsalapatis.com>
To: Ihor Solodrai <ihor.solodrai@linux.dev>
To: Shuah Khan <shuah@kernel.org>
Cc: ebpf@linuxfoundation.org
Cc: Bastien Curutchet <bastien.curutchet@bootlin.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: bpf@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Alexis Lothoré (eBPF Foundation) (2):
      selftests/bpf: keep polling connection that is still in progress
      selftests/bpf: add connect timeout to test_tc_edt

 tools/testing/selftests/bpf/network_helpers.c      | 70 ++++++++++++++++++++--
 .../testing/selftests/bpf/prog_tests/test_tc_edt.c |  2 +-
 tools/testing/selftests/bpf/testing_helpers.h      |  9 +++
 3 files changed, 75 insertions(+), 6 deletions(-)
---
base-commit: 53cc65185a51fa9dd288cd1137a21467eced2df4
change-id: 20260710-tc_tunnel_flaky-27e9a191bd03

Best regards,
--  
Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

[PATCH bpf v3 1/2] selftests/bpf: keep polling connection that is still in progress

[Alexis Lothoré (eBPF Foundation)]

Some tests, like tc_tunnel or tc_edt, sporadically fail in CI with the
following logs:

  (network_helpers.c:309: errno: Operation now in progress) \
    Failed to connect to server
  send_and_test_data:FAIL:connect to server unexpected error: -115

This is due to SO_RCVTIMEO and SO_SNDTIMEO being set on the client
socket (see settimeo() in client_socket()), allowing connect() to return
an error and to set errno to EINPROGRESS instead of blocking until
connection result is known. Increasing the timeout value for those tests
is likely not a good solution (and it has already been done by commit
2790db208b44 ("selftests/bpf: Improve tc_tunnel test reliability")):
they involve subtests that expect the connection to fail, and so
increasing the timeout value would increase overall test execution
duration again (not only the connection, but any socket operation).

Another solution, as documented in man 2 connect, is to poll the socket
for POLLOUT once connect has returned EINPROGRESS, and to get the actual
connection result through getsockopt: this allows to keep the overall
timeout values low for the general traffic, while letting a chance to
the connection to succeed even if CI runners are loaded.

When connect() returns EINPROGRESS, poll the socket for POLLOUT and
check the connection result via getsockopt(SO_ERROR). This new handling
conforms to the configured timeout: the polling loop will only run for
the amount of time still available, accounting for the time used by the
initial connect() call.

Fixes: 99126abec5e5 ("bpf: selftests: A few improvements to network_helpers.c")
Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
 tools/testing/selftests/bpf/network_helpers.c | 70 +++++++++++++++++++++++++--
 tools/testing/selftests/bpf/testing_helpers.h |  9 ++++
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
index db935a9d9fc1..e3157724adec 100644
--- a/tools/testing/selftests/bpf/network_helpers.c
+++ b/tools/testing/selftests/bpf/network_helpers.c
@@ -14,6 +14,7 @@
 #include <sys/types.h>
 #include <sys/un.h>
 #include <sys/eventfd.h>
+#include <sys/poll.h>
 
 #include <linux/err.h>
 #include <linux/in.h>
@@ -294,7 +295,10 @@ int client_socket(int family, int type,
 int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t addrlen,
 		    const struct network_helper_opts *opts)
 {
-	int fd;
+	__u64 start_ms, duration_ms;
+	__u64 remaining_ms;
+	socklen_t errlen;
+	int fd, err, ret;
 
 	if (!opts)
 		opts = &default_opts;
@@ -305,13 +309,69 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
 		return -1;
 	}
 
-	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
+	start_ms = get_time_ms();
+	err = connect(fd, (const struct sockaddr *)addr, addrlen);
+
+	if (!err)
+		return fd;
+
+	if (errno != EINPROGRESS) {
 		log_err("Failed to connect to server");
-		save_errno_close(fd);
-		return -1;
+		goto close;
 	}
 
-	return fd;
+	duration_ms = get_time_ms() - start_ms;
+	remaining_ms = duration_ms < opts->timeout_ms ?
+			       opts->timeout_ms - duration_ms :
+			       0;
+	if (!remaining_ms) {
+		errno = ETIMEDOUT;
+		log_err("Can not poll connection, already in timeout");
+		goto close;
+	}
+
+	while (remaining_ms) {
+		struct pollfd pfd = { .fd = fd, .events = POLLOUT };
+
+		start_ms = get_time_ms();
+		ret = poll(&pfd, 1, remaining_ms);
+
+		if (ret == 0) {
+			errno = ETIMEDOUT;
+			log_err("Connection timeout while polling");
+			goto close;
+		} else if (ret < 0 && errno == EINTR) {
+			duration_ms = get_time_ms() - start_ms;
+			remaining_ms = duration_ms < remaining_ms ?
+					       remaining_ms - duration_ms :
+					       0;
+			if (!remaining_ms) {
+				errno = ETIMEDOUT;
+				log_err("Connection timeout after signal");
+				goto close;
+			}
+		} else if (ret < 0) {
+			log_err("Failed to poll connect status");
+			goto close;
+		}
+
+		errlen = sizeof(err);
+		if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &errlen) < 0) {
+			log_err("Failed to getsockopt");
+			goto close;
+		}
+
+		if (err) {
+			errno = err;
+			log_err("Eventually failed to connect to server");
+			goto close;
+		}
+		return fd;
+	}
+
+close:
+	save_errno_close(fd);
+	return -1;
 }
 
 int connect_to_addr_str(int family, int type, const char *addr_str, __u16 port,
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index 2edc6fb7fc52..7440cfcd1025 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -52,6 +52,15 @@ static inline __u64 get_time_ns(void)
 	return (u64)t.tv_sec * 1000000000 + t.tv_nsec;
 }
 
+static inline __u64 get_time_ms(void)
+{
+	struct timespec t;
+
+	clock_gettime(CLOCK_MONOTONIC, &t);
+
+	return (u64)t.tv_sec * 1000 + t.tv_nsec / 1000000;
+}
+
 struct bpf_insn;
 /* Request BPF program instructions after all rewrites are applied,
  * e.g. verifier.c:convert_ctx_access() is done.

-- 
2.55.0

[PATCH bpf v3 2/2] selftests/bpf: add connect timeout to test_tc_edt

[Alexis Lothoré (eBPF Foundation)]

Now that the connect helpers from network_helpers.c poll the connect
status when connect() returns EINPROGRESS, make sure that test_tc_edt
benefits from this polling by setting a timeout value for connection.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
 tools/testing/selftests/bpf/prog_tests/test_tc_edt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_edt.c b/tools/testing/selftests/bpf/prog_tests/test_tc_edt.c
index 462512fb191f..8ac3caa3d091 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_tc_edt.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_tc_edt.c
@@ -109,7 +109,7 @@ static void run_test(void)
 	nstoken = open_netns(CLIENT_NS);
 	if (!ASSERT_OK_PTR(nstoken, "open client ns"))
 		return;
-	client_fd = connect_to_fd(server_fd, 0);
+	client_fd = connect_to_fd(server_fd, TIMEOUT_MS);
 	if (!ASSERT_OK_FD(client_fd, "connect client"))
 		return;
 

-- 
2.55.0

Re: [PATCH bpf v3 1/2] selftests/bpf: keep polling connection that is still in progress

[Jiayuan Chen]

On 8/11/26 10:26 PM, Alexis Lothoré (eBPF Foundation) wrote:
> Some tests, like tc_tunnel or tc_edt, sporadically fail in CI with the
> following logs:
>
>    (network_helpers.c:309: errno: Operation now in progress) \
>      Failed to connect to server
>    send_and_test_data:FAIL:connect to server unexpected error: -115
>
> This is due to SO_RCVTIMEO and SO_SNDTIMEO being set on the client
> socket (see settimeo() in client_socket()), allowing connect() to return
> an error and to set errno to EINPROGRESS instead of blocking until
> connection result is known. Increasing the timeout value for those tests
> is likely not a good solution (and it has already been done by commit
> 2790db208b44 ("selftests/bpf: Improve tc_tunnel test reliability")):
> they involve subtests that expect the connection to fail, and so
> increasing the timeout value would increase overall test execution
> duration again (not only the connection, but any socket operation).
>
> Another solution, as documented in man 2 connect, is to poll the socket
> for POLLOUT once connect has returned EINPROGRESS, and to get the actual
> connection result through getsockopt: this allows to keep the overall
> timeout values low for the general traffic, while letting a chance to
> the connection to succeed even if CI runners are loaded.
>
> When connect() returns EINPROGRESS, poll the socket for POLLOUT and
> check the connection result via getsockopt(SO_ERROR). This new handling
> conforms to the configured timeout: the polling loop will only run for
> the amount of time still available, accounting for the time used by the
> initial connect() call.


So IIUC this patch doesn't actually fix the flakiness: connect() on a 
blocking socket only
returns EINPROGRESS after SO_SNDTIMEO is fully consumed, so remaining_ms 
is always ~0 and the
overall time budget is still 1s, same as before. Am I missing something?

Re: [PATCH bpf v3 1/2] selftests/bpf: keep polling connection that is still in progress

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3201 bytes --]

> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index b82f572641b7..25e4d2fa6f6f 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c

[ ... ]

> @@ -305,13 +309,69 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
>  		return -1;
>  	}
>
> -	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
> +	start_ms = get_time_ms();
> +	err = connect(fd, (const struct sockaddr *)addr, addrlen);
> +
> +	if (!err)
> +		return fd;
> +
> +	if (errno != EINPROGRESS) {
>  		log_err("Failed to connect to server");
> -		save_errno_close(fd);
> -		return -1;
> +		goto close;
>  	}
>
> -	return fd;
> +	duration_ms = get_time_ms() - start_ms;
> +	remaining_ms = duration_ms < opts->timeout_ms ?
> +			       opts->timeout_ms - duration_ms :
> +			       0;
> +	if (!remaining_ms) {
> +		errno = ETIMEDOUT;
> +		log_err("Can not poll connection, already in timeout");
> +		goto close;
> +	}
> +
> +	while (remaining_ms) {
> +		struct pollfd pfd = { .fd = fd, .events = POLLOUT };
> +
> +		start_ms = get_time_ms();
> +		ret = poll(&pfd, 1, remaining_ms);
> +
> +		if (ret == 0) {
> +			errno = ETIMEDOUT;
> +			log_err("Connection timeout while polling");
> +			goto close;
> +		} else if (ret < 0 && errno == EINTR) {
> +			duration_ms = get_time_ms() - start_ms;
> +			remaining_ms = duration_ms < remaining_ms ?
> +					       remaining_ms - duration_ms :
> +					       0;
> +			if (!remaining_ms) {
> +				errno = ETIMEDOUT;
> +				log_err("Connection timeout after signal");
> +				goto close;
> +			}
> +		} else if (ret < 0) {
> +			log_err("Failed to poll connect status");
> +			goto close;
> +		}
> +
> +		errlen = sizeof(err);
> +		if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &errlen) < 0) {
> +			log_err("Failed to getsockopt");
> +			goto close;
> +		}
> +
> +		if (err) {
> +			errno = err;
> +			log_err("Eventually failed to connect to server");
> +			goto close;
> +		}
> +		return fd;
> +	}

Does the EINTR branch need to continue the loop? When poll() returns
EINTR, the code recomputes remaining_ms but then falls through to
getsockopt(SO_ERROR). If the connection is still in progress, SO_ERROR
will be 0 and connect_to_addr() returns the fd even though the socket
is not yet connected.

Also, no path inside the while loop reaches the loop condition again -
every branch either does 'goto close' or 'return fd'. So after the EINTR
budget recalculation runs, the code never re-enters poll() to use that
budget.

Would adding 'continue;' at the end of the EINTR branch fix this? That
way poll() gets called again with the updated remaining_ms, and the
connection completes properly after a signal interruption.

Test_progs installs a SIGINT handler without SA_RESTART in
test_progs.c, so EINTR is reachable here.

> +
> +close:
> +	save_errno_close(fd);
> +	return -1;
>  }

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31503700726

Re: [PATCH bpf v3 1/2] selftests/bpf: keep polling connection that is still in progress

[Alexis Lothoré]

On Tue Aug 11, 2026 at 5:30 PM CEST, bot+bpf-ci wrote:
>> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
>> index b82f572641b7..25e4d2fa6f6f 100644
>> --- a/tools/testing/selftests/bpf/network_helpers.c
>> +++ b/tools/testing/selftests/bpf/network_helpers.c
>
> [ ... ]
>
>> @@ -305,13 +309,69 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
>>  		return -1;
>>  	}
>>
>> -	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
>> +	start_ms = get_time_ms();
>> +	err = connect(fd, (const struct sockaddr *)addr, addrlen);
>> +
>> +	if (!err)
>> +		return fd;
>> +
>> +	if (errno != EINPROGRESS) {
>>  		log_err("Failed to connect to server");
>> -		save_errno_close(fd);
>> -		return -1;
>> +		goto close;
>>  	}
>>
>> -	return fd;
>> +	duration_ms = get_time_ms() - start_ms;
>> +	remaining_ms = duration_ms < opts->timeout_ms ?
>> +			       opts->timeout_ms - duration_ms :
>> +			       0;
>> +	if (!remaining_ms) {
>> +		errno = ETIMEDOUT;
>> +		log_err("Can not poll connection, already in timeout");
>> +		goto close;
>> +	}
>> +
>> +	while (remaining_ms) {
>> +		struct pollfd pfd = { .fd = fd, .events = POLLOUT };
>> +
>> +		start_ms = get_time_ms();
>> +		ret = poll(&pfd, 1, remaining_ms);
>> +
>> +		if (ret == 0) {
>> +			errno = ETIMEDOUT;
>> +			log_err("Connection timeout while polling");
>> +			goto close;
>> +		} else if (ret < 0 && errno == EINTR) {
>> +			duration_ms = get_time_ms() - start_ms;
>> +			remaining_ms = duration_ms < remaining_ms ?
>> +					       remaining_ms - duration_ms :
>> +					       0;
>> +			if (!remaining_ms) {
>> +				errno = ETIMEDOUT;
>> +				log_err("Connection timeout after signal");
>> +				goto close;
>> +			}
>> +		} else if (ret < 0) {
>> +			log_err("Failed to poll connect status");
>> +			goto close;
>> +		}
>> +
>> +		errlen = sizeof(err);
>> +		if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &errlen) < 0) {
>> +			log_err("Failed to getsockopt");
>> +			goto close;
>> +		}
>> +
>> +		if (err) {
>> +			errno = err;
>> +			log_err("Eventually failed to connect to server");
>> +			goto close;
>> +		}
>> +		return fd;
>> +	}
>
> Does the EINTR branch need to continue the loop? When poll() returns
> EINTR, the code recomputes remaining_ms but then falls through to
> getsockopt(SO_ERROR). If the connection is still in progress, SO_ERROR
> will be 0 and connect_to_addr() returns the fd even though the socket
> is not yet connected.

Argh, I accidentally got rid of the continue that was in the EINTR
branch while reorganizing a bit all the branches. And it then defeat the
whole purpose of the EINTR branch... Will be updated.
>
> Also, no path inside the while loop reaches the loop condition again -
> every branch either does 'goto close' or 'return fd'. So after the EINTR
> budget recalculation runs, the code never re-enters poll() to use that
> budget.
>
> Would adding 'continue;' at the end of the EINTR branch fix this? That
> way poll() gets called again with the updated remaining_ms, and the
> connection completes properly after a signal interruption.
>
> Test_progs installs a SIGINT handler without SA_RESTART in
> test_progs.c, so EINTR is reachable here.
>
>> +
>> +close:
>> +	save_errno_close(fd);
>> +	return -1;
>>  }
>
> [ ... ]
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31503700726




-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH bpf v3 1/2] selftests/bpf: keep polling connection that is still in progress

[Alexis Lothoré]

On Tue Aug 11, 2026 at 5:05 PM CEST, Jiayuan Chen wrote:
>
> On 8/11/26 10:26 PM, Alexis Lothoré (eBPF Foundation) wrote:
>> Some tests, like tc_tunnel or tc_edt, sporadically fail in CI with the
>> following logs:
>>
>>    (network_helpers.c:309: errno: Operation now in progress) \
>>      Failed to connect to server
>>    send_and_test_data:FAIL:connect to server unexpected error: -115
>>
>> This is due to SO_RCVTIMEO and SO_SNDTIMEO being set on the client
>> socket (see settimeo() in client_socket()), allowing connect() to return
>> an error and to set errno to EINPROGRESS instead of blocking until
>> connection result is known. Increasing the timeout value for those tests
>> is likely not a good solution (and it has already been done by commit
>> 2790db208b44 ("selftests/bpf: Improve tc_tunnel test reliability")):
>> they involve subtests that expect the connection to fail, and so
>> increasing the timeout value would increase overall test execution
>> duration again (not only the connection, but any socket operation).
>>
>> Another solution, as documented in man 2 connect, is to poll the socket
>> for POLLOUT once connect has returned EINPROGRESS, and to get the actual
>> connection result through getsockopt: this allows to keep the overall
>> timeout values low for the general traffic, while letting a chance to
>> the connection to succeed even if CI runners are loaded.
>>
>> When connect() returns EINPROGRESS, poll the socket for POLLOUT and
>> check the connection result via getsockopt(SO_ERROR). This new handling
>> conforms to the configured timeout: the polling loop will only run for
>> the amount of time still available, accounting for the time used by the
>> initial connect() call.
>
>
> So IIUC this patch doesn't actually fix the flakiness: connect() on a 
> blocking socket only
> returns EINPROGRESS after SO_SNDTIMEO is fully consumed, so remaining_ms 
> is always ~0 and the
> overall time budget is still 1s, same as before. Am I missing something?

Hmmm, I have been assuming that this EINPROGRESS could be returned
_before_ the configured timeout depletion, but I may have been mistaken,
it indeed happens only  the socket is O_NONBLOCK, which is not the case
here. So indeed, it does not fix anything for the blocking case, as the
budget is already depleted when getting EINPROGRESS...

I added this budget mechanism to follow up on Ihor's suggestion, so I
either got it wrong, or it can not work. An intermediate solution could
be to exceptionally raise the budget by 1s when getting EINPROGRESS.
That potentially brings back part of the issues he has been mentioning
with selftests duration possibly increasing by a non negligeable amount,
but maybe 1s is a better compromise, compared to my initial 3s proposal
?

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH bpf v3 1/2] selftests/bpf: keep polling connection that is still in progress

[Ihor Solodrai]

On 8/11/26 9:25 AM, Alexis Lothoré wrote:
> On Tue Aug 11, 2026 at 5:05 PM CEST, Jiayuan Chen wrote:
>>
>> On 8/11/26 10:26 PM, Alexis Lothoré (eBPF Foundation) wrote:
>>> Some tests, like tc_tunnel or tc_edt, sporadically fail in CI with the
>>> following logs:
>>>
>>>    (network_helpers.c:309: errno: Operation now in progress) \
>>>      Failed to connect to server
>>>    send_and_test_data:FAIL:connect to server unexpected error: -115
>>>
>>> This is due to SO_RCVTIMEO and SO_SNDTIMEO being set on the client
>>> socket (see settimeo() in client_socket()), allowing connect() to return
>>> an error and to set errno to EINPROGRESS instead of blocking until
>>> connection result is known. Increasing the timeout value for those tests
>>> is likely not a good solution (and it has already been done by commit
>>> 2790db208b44 ("selftests/bpf: Improve tc_tunnel test reliability")):
>>> they involve subtests that expect the connection to fail, and so
>>> increasing the timeout value would increase overall test execution
>>> duration again (not only the connection, but any socket operation).
>>>
>>> Another solution, as documented in man 2 connect, is to poll the socket
>>> for POLLOUT once connect has returned EINPROGRESS, and to get the actual
>>> connection result through getsockopt: this allows to keep the overall
>>> timeout values low for the general traffic, while letting a chance to
>>> the connection to succeed even if CI runners are loaded.
>>>
>>> When connect() returns EINPROGRESS, poll the socket for POLLOUT and
>>> check the connection result via getsockopt(SO_ERROR). This new handling
>>> conforms to the configured timeout: the polling loop will only run for
>>> the amount of time still available, accounting for the time used by the
>>> initial connect() call.
>>
>>
>> So IIUC this patch doesn't actually fix the flakiness: connect() on a 
>> blocking socket only
>> returns EINPROGRESS after SO_SNDTIMEO is fully consumed, so remaining_ms 
>> is always ~0 and the
>> overall time budget is still 1s, same as before. Am I missing something?
> 
> Hmmm, I have been assuming that this EINPROGRESS could be returned
> _before_ the configured timeout depletion, but I may have been mistaken,
> it indeed happens only  the socket is O_NONBLOCK, which is not the case
> here. So indeed, it does not fix anything for the blocking case, as the
> budget is already depleted when getting EINPROGRESS...
> 
> I added this budget mechanism to follow up on Ihor's suggestion, so I
> either got it wrong, or it can not work.

Hi Alexis,

Looks like my suggestion was bad, sorry. Jiayuan is right. I didn't
realize what -EINPROGRESS actually means here.

See __inet_stream_connect() in net/ipv4/af_inet.c:697

	err = -EINPROGRESS;
	...
	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
	...
	if (!timeo || !inet_wait_for_connect(sk, timeo, writebias))
		goto out;

-EINPROGRESS is a default that reaches userspace only when the wait
expires. Every other exit sets a different error code. So on a
blocking socket -EINPROGRESS does not mean "connection started", it
means "waited the whole budget, gave up in SYN_SENT". On a nonblocking
socket it means the opposite, and man 2 connect seems to only document
the nonblocking case.

> An intermediate solution could
> be to exceptionally raise the budget by 1s when getting EINPROGRESS.

I believe you're right that the flakiness can be addressed with bigger
timeout, which is why v2 fixed it for you. But +1s may not be enough.

IIUC the way the helpers are written cause -EINPROGRESS instead of
-ETIMEDOUT. If SO_SNDTIMEO is not set before connect(), the kernel
reports plain ETIMEDOUT, but it is set in settimeo().

I vibe-coded a userspace repro: loopback listener with a full accept
queue drained at t=1500ms, so the handshake completes at ~2040ms.
Caller asks for timeout_ms=1000, like tc_tunnel:

  mainline (blocking)                     budget 1000   FAIL at 1016ms
  v3 (blocking + leftover poll)           budget 1000   FAIL at 1015ms # my suggestion
  O_NONBLOCK + poll(timeout_ms)           budget 1000   FAIL at 1001ms # Jiayuan's suggestion
  O_NONBLOCK + poll(3500)                 budget 3500   OK   at 2031ms
  blocking, SO_SNDTIMEO raised to 3500    budget 3500   OK   at 2040ms
  v2 (blocking 1000 + fixed 3000 grace)   budget 4046   OK   at 2067ms

Everything that gave the handshake more than ~2040ms passed. Blocking
vs nonblocking makes no difference, poll() vs no poll() makes no
difference. We just need to increase the budget for connect().

I don't think swtiching to O_NONBLOCK makes sense. Apparently, it was
nonblocking in the past, see 99126abec5e5 ("bpf: selftests: A few
improvements to network_helpers.c")

So I think we can introduce a separate connect() budget, similar to
your v2. And then use opts->timeout_ms for everything after. Something
like this:

	#define CONNECT_TIMEOUT_MS	5000

	static int connect_timeout_ms(const struct network_helper_opts *opts)
	{
		/* don't shorten what the caller asked for: tc_redirect
		 * and xdp_synproxy pass 10000
		 */
		return MAX(opts->timeout_ms, CONNECT_TIMEOUT_MS);
	}

and in connect_to_addr():

	fd = client_socket(addr->ss_family, type, opts);
	if (fd < 0) {
		log_err("Failed to create client socket");
		return -1;
	}

	/* SO_SNDTIMEO is a ceiling, not a sleep: a wider handshake
	 * budget costs nothing on connections that succeed.
	 */
	if (settimeo(fd, connect_timeout_ms(opts)))
		goto close;

	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
		log_err("Failed to connect to server");
		goto close;
	}

	if (settimeo(fd, opts->timeout_ms))	/* data I/O budget */
		goto close;

	return fd;

close:
	save_errno_close(fd);
	return -1;

This is simpler than poll() and retry loops.


An entirely different alternative that Eduard brought up in an
off-list discussion, is to drop all the timeout machinery from
network_helpers.c altogether, and rely solely on the test_progs
watchdog to kill the subtest processes: d9d4d127e813 ("selftests/bpf:
watchdog timer for test_progs")

I am not convinced it's a good idea, because I don't know what will
happen with all the tc_* tests if there is no connection timeouts. If
you're interested, you could try it and see.


> That potentially brings back part of the issues he has been mentioning
> with selftests duration possibly increasing by a non negligeable amount,
> but maybe 1s is a better compromise, compared to my initial 3s proposal
> ?
>