Re: [PATCH mptcp-next v7 4/6] mptcp: pm: in-kernel: retry subflows closed after an error

[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] The new paragraph in Documentation/networking/mptcp.rst (lines
  76-85) does not match what the code does in three ways.

--

mptcp: pm: in-kernel: retry subflows closed after an error

With the in-kernel PM, a subflow that TCP closes after a retransmission
or keepalive timeout is now re-created with the same addresses. The new
subflow's SYN retries are set from the subflow-retries limit. Subflows
reset by the peer or the path, never established, aborted locally, or
using a removed address are not retried.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/440

> diff --git a/Documentation/networking/mptcp.rst b/Documentation/networking/mptcp.rst
> index b6753ffb9c9a6..0f07d4a29b6b7 100644
> --- a/Documentation/networking/mptcp.rst
> +++ b/Documentation/networking/mptcp.rst
> @@ -73,6 +73,17 @@ To be able to use multiple IP addresses on a host to create multiple *subflows*
>  addresses can be used. This can be configured with ``ip mptcp endpoint`` for
>  example.
>  
> +When TCP gives up on an established subflow initiated by the in-kernel path
> +manager, after a retransmission or keepalive timeout, the path manager creates
> +it again, with the ``subflow-retries`` limit as its number of SYN retries, like
> +``TCP_SYNCNT``. A subflow reset by the peer, or by anything on the path, is not
> +created again. TCP only gives up on a subflow after ``tcp_retries2``

[Severity: Low]
Does this paragraph match what mptcp_pm_kernel_retry_check() and
mptcp_pm_kernel_retry() do? I see three differences.

First, the text says only subflows "initiated by the in-kernel path
manager" are re-created. The scope check in mptcp_pm_kernel_retry_check()
is:

net/mptcp/pm_kernel.c:mptcp_pm_kernel_retry_check() {
    ...
	/* Only re-create subflows initiated by this side: MP_JOIN ones, or the
	 * initial one on the client side.
	 */
	if (!subflow->request_join &&
	    (subflow->mp_join || READ_ONCE(msk->pm.server_side)))
		return false;
    ...
}

On the client side, the initial MP_CAPABLE subflow has request_join,
mp_join and server_side all at 0, so it passes this check and gets
re-created. That subflow came from the application's connect(), not
from the PM.

Second, re-creating the initial subflow takes one of the "subflows"
limit slots. mptcp_pm_kernel_retry() always does:

	if (allowed)
		msk->pm.extra_subflows++;

The original initial subflow never decremented that counter when it
closed, because mptcp_pm_subflow_check_next() only updates it when
request_join or mp_join is set.

So once the initial subflow is re-created, later endpoints or announced
addresses have one less slot. With a "subflows" limit of 0, the initial
subflow is never re-created at all.

Third, the re-creation only happens once. A re-created subflow that does
not connect within its SYN retries never becomes fully_established, so
this check stops it from being tried again:

	if (!READ_ONCE(subflow->fully_established))
		return false;

The commit message covers the second and third points ("Re-creating the
initial subflow uses one of the additional subflows allowed by the
limits" and "If this new subflow cannot connect, it is not retried
again"). Could mptcp.rst describe the scope correctly and mention these
two points as well?

> +retransmissions, about 15 minutes by default: shorter outages are handled by
> +TCP itself. A subflow that only receives data does not notice that its path
> +stopped working: the application should then enable TCP keepalive on the
> +MPTCP socket (``SO_KEEPALIVE``, ``TCP_KEEPIDLE``, ``TCP_KEEPINTVL`` and
> +``TCP_KEEPCNT``), which MPTCP applies to all its subflows.
> +

[ ... ]

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