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
