Re: [RFC PATCH mptcp-next v3 1/3] selftests: mptcp: connect: retry write() on EAGAIN

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In copyfd_io_poll()
  (tools/testing/selftests/net/mptcp/mptcp_connect.c), the new EAGAIN…
- [Low] In copyfd_io_poll()
  (tools/testing/selftests/net/mptcp/mptcp_connect.c), `eagain_ms`…

--

selftests: mptcp: connect: retry write() on EAGAIN

In copyfd_io_poll(), this patch stops treating an EAGAIN from write()
after POLLOUT as fatal. The loop sleeps 1ms and goes back to poll().
It still gives up once the EAGAIN count exceeds poll_timeout.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea..03fff859e4d1 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> @@ -638,6 +638,7 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
>  		.events = POLLIN | POLLOUT,
>  	};
>  	unsigned int total_wlen = 0, total_rlen = 0;
> +	int eagain_ms = 0;
>  
>  	set_nonblock(peerfd, true);
>  
> @@ -711,6 +712,20 @@ static int copyfd_io_poll(int infd, int peerfd, int outfd,
>  
>  				bw = do_rnd_write(peerfd, winfo->buf + winfo->off, winfo->len);
>  				if (bw < 0) {
> +					/* POLLOUT does not guarantee that
> +					 * the next write will not block:
> +					 * the MPTCP send buffer can shrink
> +					 * in between, e.g. when a subflow
> +					 * is closed. Wait for more space,
> +					 * but not forever: poll() does not
> +					 * check every cause of EAGAIN.
> +					 */
> +					if (errno == EAGAIN &&
> +					    ++eagain_ms <= poll_timeout) {

[Severity: Medium]
What happens here when poll_timeout is -1? The -t option parser in
main() turns any non-positive value into the "no timeout" value:

	case 't':
		poll_timeout = atoi(optarg) * 1000;
		if (poll_timeout <= 0)
			poll_timeout = -1;

With -t 0 or a negative value, the poll() call at the top of the loop
waits forever. But the first EAGAIN from do_rnd_write() sets eagain_ms
to 1, and 1 <= -1 is false.

Control then falls through to perror("write") and return 111. That is
the same fatal exit this patch is meant to remove.

Would a condition like (poll_timeout < 0 || ++eagain_ms <= poll_timeout)
fit the intent better? A separate finite cap would also work.

The in-tree scripts all pass -t ${timeout_poll}, so only an explicit
non-positive -t hits this.

> +						usleep(1000);
> +						continue;
> +					}

[Severity: Low]
Does eagain_ms actually measure milliseconds? It goes up by one per
EAGAIN and is compared with poll_timeout, which is in milliseconds. But
each retry goes back to the top of the loop, and this call gets the full
timeout again:

	switch (poll(&fds, 1, poll_timeout)) {

That iteration can also handle POLLIN with a blocking do_write() to
outfd before the next write attempt.

So each counted retry costs at least 1ms plus timer slack and syscall
overhead. If poll() blocks before reporting POLLOUT, a retry can cost up
to a full poll_timeout.

The counter is reset only after a successful write. Iterations that only
see POLLIN neither increment it nor reset it.

The in-tree scripts pass -t 30, which allows 30000 retries. That is
about 31-35 seconds when POLLOUT is reported right away.

If poll() blocks between EAGAINs, the total can go past the harness kill
timeout in mptcp_connect.sh:

	timeout_test=$((timeout_poll * 2 + 1))

The tool is then killed from outside and never prints its own write()
error.

The commit message says "If EAGAIN lasts longer than the poll timeout,
still fail". Could the bound track elapsed time, for example with
clock_gettime()? Alternatively, the variable could be renamed to show
that it is a retry count.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014143349.768138-1-anilkaushikwireless%40gmail.com