Re: [PATCH mptcp-next v3 2/3] mptcp: make ADD_ADDR retransmission timeout adaptive

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 10/08/2025 03:13, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, 2025-08-07 at 13:56 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 07/08/2025 05:25, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Wed, 2025-08-06 at 17:34 +0200, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> On 06/08/2025 05:17, Geliang Tang wrote:
>>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>>
>>>>> Replace the fixed ADD_ADDR retransmission timeout with an
>>>>> adaptive
>>>>> mechanism that uses the maximum RTO among existing subflows.
>>>>> This
>>>>> improves responsiveness when establishing new subflows while
>>>>> maintaining
>>>>> an upper bound through the configured add_addr_timeout value.
>>>>>
>>>>> Key changes:
>>>>> 1. Compute timeout as min(max_subflow_RTO,
>>>>> configured_max_timeout)
>>>>> 2. Apply exponential backoff based on retransmission count
>>>>> 3. Maintain fallback to configured max timeout when no subflows
>>>>> exist
>>>>> 4. Update documentation to clarify add_addr_timeout is now the
>>>>> maximum
>>>>> value
>>>>>
>>>>> Closes:
>>>>> https://github.com/multipath-tcp/mptcp_net-next/issues/576
>>>>> Reviewed-by: Christoph Paasch <cpaasch@openai.com>
>>>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>>>> ---
>>>>>  Documentation/networking/mptcp-sysctl.rst |  4 +--
>>>>>  net/mptcp/pm.c                            | 30
>>>>> ++++++++++++++++++++---
>>>>>  2 files changed, 29 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/networking/mptcp-sysctl.rst
>>>>> b/Documentation/networking/mptcp-sysctl.rst
>>>>> index 1683c139821e..2b8e8fb99ee2 100644
>>>>> --- a/Documentation/networking/mptcp-sysctl.rst
>>>>> +++ b/Documentation/networking/mptcp-sysctl.rst
>>>>> @@ -8,8 +8,8 @@ MPTCP Sysfs variables
>>>>>  ===============================
>>>>>  
>>>>>  add_addr_timeout - INTEGER (seconds)
>>>>> -	Set the timeout after which an ADD_ADDR control
>>>>> message
>>>>> will be
>>>>> -	resent to an MPTCP peer that has not acknowledged a
>>>>> previous
>>>>> +	Set the maximum value of timeout after which an
>>>>> ADD_ADDR
>>>>> control message
>>>>> +	will be resent to an MPTCP peer that has not
>>>>> acknowledged
>>>>> a previous
>>>>>  	ADD_ADDR message.
>>>>>  
>>>>>  	Do not retransmit if set to 0.
>>>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>>>> index 1330af6a2154..34cc4f45bc7d 100644
>>>>> --- a/net/mptcp/pm.c
>>>>> +++ b/net/mptcp/pm.c
>>>>> @@ -268,6 +268,30 @@ int mptcp_pm_mp_prio_send_ack(struct
>>>>> mptcp_sock *msk,
>>>>>  	return -EINVAL;
>>>>>  }
>>>>>  
>>>>> +static unsigned int mptcp_adjust_add_addr_timeout(struct
>>>>> mptcp_sock *msk)
>>>>> +{
>>>>> +	struct mptcp_subflow_context *subflow;
>>>>> +	struct sock *sk = (struct sock *)msk;
>>>>> +	unsigned int max_rto = 0;
>>>>> +	unsigned int timeout;
>>>>> +
>>>>> +	timeout = mptcp_get_add_addr_timeout(sock_net(sk));
>>>>> +
>>>>> +	mptcp_for_each_subflow(msk, subflow) {
>>>>> +		struct sock *ssk =
>>>>> mptcp_subflow_tcp_sock(subflow);
>>>>> +		unsigned int subflow_rto;
>>>>> +
>>>>> +		subflow_rto = __tcp_set_rto(tcp_sk(ssk)) * 2;
>>>>
>>>> Why "* 2"? Is it what TCP is doing?
>>>>
>>>> (Probably best to add a comment here above, and in the commit
>>>> message.)
>>>>
>>>> (Also, I guess the compiler will do the optimisation, but you
>>>> could
>>>> write: ">> 1" instead of "* 2".
>>>
>>> Yes, "<< 1" is much better. I added a comment here:
>>>
>>> +               /*
>>> +                * icsk->icsk_rto is not used directly since
>>> +                * it's not updated frequently enough.
>>> +                *
>>> +                * subflow_rto is twice of rto, and allow
>>> +                * some tolerance for add_addr echo.
>>> +                */
>>> +               subflow_rto = __tcp_set_rto(tcp_sk(ssk)) << 1;
>>>
>>> Is this okay?
>>
>> No :)
>>
>> I don't think the comment explain why it is twice the default TCP
>> RTO.
>> Why having an exception here for the ADD_ADDR? I mean: I'm OK with
>> the
>> idea of doubling the RTO, but only if there is a reason behind that
>> :)
> 
> How about this one (answered by DeepSeek, an AI from China):
> 
>         /*   
>          * icsk->icsk_rto is not used directly since it's not
>          * updated frequently enough.

What about: (...) since it might not have been updated recently.

>          * Setting subflow_rto to twice of RTO provides a safety
>          * buffer for RTT estimation variance, kernel RTO update
>          * delays, and a full transmission retry cycle while
>          * minimizing false failures.

A bit long and not clear (typical AI, making commit messages too
verbose, it almost force me to use one to give me the summary :) )

What about:

  Setting this RTO to twice of TCP one, not to be too agressive for this
  notification that might take longer to process than "simple" data.

Still, is icsk_rto not already "long" enough by default? I mean: when I
see your description in the related packetdrill PR [1] where the delay
is supposed to be 0.01 sec, but "subflow_rto" (or was it isck_rto?) is
0.65 sec, that's quite different, and half of it would be more than
enough :)

But maybe that's different when it happens later during the connection?

[1] https://github.com/multipath-tcp/packetdrill/pull/166

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.