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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

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

--

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

This patch changes copyfd_io_poll() in mptcp_connect so that a write()
failing with EAGAIN no longer ends the transfer. The tool sleeps 1ms and
goes back to poll(). When a poll timeout is set, the number of retries is
capped by the poll_timeout value.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> index 178d98d91fea2..b9fd0bdab7486 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c

[ ... ]

> @@ -711,6 +712,21 @@ 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 &&
> +					    (poll_timeout < 0 ||
> +					     ++eagain_cnt <= poll_timeout)) {
> +						usleep(1000);
> +						continue;
> +					}

[Severity: Low]
This isn't a bug, but is the "but not forever" part of the comment
accurate in every mode?

main() turns -t 0, or any negative -t value, into poll_timeout = -1:

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

When poll_timeout is -1, the condition above never looks at eagain_cnt.
If write() keeps returning EAGAIN while poll() keeps reporting POLLOUT,
copyfd_io_poll() loops forever with a 1ms sleep on each pass. That is
the case the comment says it guards against.

The commit message ("If a poll timeout is set, still fail after as many
retries") and the v4 note ("no limit when there is no poll timeout,
-t 0") suggest this is intended. The in-tree callers all pass
-t ${timeout_poll}, so the selftests never take this branch.

Could the comment say that the limit only applies when a poll timeout
is set?

> +
>  					/* expected reset, continue to read */
>  					if (cfg_rcv_trunc &&
>  					    (errno == ECONNRESET ||

[ ... ]

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