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

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

Hi Ihor,
thanks for the extensive investigation

On Wed Aug 12, 2026 at 8:46 PM CEST, Ihor Solodrai wrote:
> 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:

[...]

>>> 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().

Ok, so indeed the poll() loop complexifies the connection helpers
without much gain.

> 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.

Ok, I'll give it a try with this simpler connect-only-extended-budget,
without the poll loop

> 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.

Hmmm, I'm not very familiar with this mechanism, but aside from the
potential issues you are mentioning, wouldn't it make us loose a bit of
info here, when the watchdog kicks the runner out ? We may know from the
stacktrace that the subtest was in connect(), but we would loose any
formal error/errno on connection timeout I guess, and I am not sure how
confident we can get about any watchdog kick being a connect timeout.

>> 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