Re: [PATCH mptcp-next 2/3] mptcp: make ADD_ADDR timeout adaptive to subflow RTT

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 29/07/2025 10:22, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch makes the ADD_ADDR retransmission timeout adaptive by using
> the maximum subflow RTT, while still capping it at the configured max
> value (add_addr_timeout_max). This improves responsiveness when
> establishing new subflows.
> 
> The change:
>  - Adds mptcp_get_add_addr_timeout() helper
>  - Uses subflow RTT when available
>  - Falls back to max timeout otherwise
> 

Can you add the Closes tag here, so GitHub will automatically close the
ticket when this patch will be applied?

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

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 40e8ebe566e5..bf4d92293e1e 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -268,6 +268,27 @@ int mptcp_pm_mp_prio_send_ack(struct mptcp_sock *msk,
>  	return -EINVAL;
>  }
>  
> +static unsigned int mptcp_get_add_addr_timeout(struct mptcp_sock *msk,
> +					       const struct net *net)
> +{
> +	unsigned int timeout = mptcp_get_add_addr_timeout_max(net);
> +	struct mptcp_subflow_context *subflow;
> +	unsigned int srtt_us = 0;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		struct tcp_sock *tp = tcp_sk(ssk);
> +
> +		if (srtt_us < tp->srtt_us)

(detail: it feels more logical do have "if (tp->srtt_us > srtt_us)" if
you look for the max, but the result is the same)

> +			srtt_us = tp->srtt_us;
> +	}
> +
> +	if (srtt_us && srtt_us < timeout)

I don't think you can do that: timeout is in jiffies, while srtt_us is
in µsec. You need to use "usecs_to_jiffies(srtt_us)"

> +		timeout = srtt_us;

Taking just srtt_us doesn't seem to be a good idea as the ADD_ADDR echo
is supposed to arrive around srtt_us: this will certainly cause too
aggressive ADD_ADDR retransmissions if timeout is around the time the
echo is supposed to arrive.

I think we should imitate TCP here: using srtt_us and rttvar_us. (Or
start with "srtt_us << 1"?). Or maybe better to use "icsk->icsk_rto"
instead, but I don't know if it will be set to a correct value when we
will look, no?

> +
> +	return timeout;
> +}
> +
>  static void mptcp_pm_add_timer(struct timer_list *timer)
>  {
>  	struct mptcp_pm_add_entry *entry = timer_container_of(entry, timer,
> @@ -302,7 +323,7 @@ static void mptcp_pm_add_timer(struct timer_list *timer)
>  
>  	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
>  		sk_reset_timer(sk, timer,
> -			       jiffies + mptcp_get_add_addr_timeout_max(sock_net(sk)));
> +			       jiffies + mptcp_get_add_addr_timeout(msk, sock_net(sk)));

I wonder if we should increase the time after each retransmission,
similar to what is done in TCP (timeout << entry->retrans_times). We
could keep it linear, but because this time now depends on the measured
srtt, it might be better to have an exponential backoff. It will still
need to be bounded to the max. In this case, it might be easier to pass
entry->retrans_times to mptcp_get_add_addr_timeout())

>  
>  	spin_unlock_bh(&msk->pm.lock);
>  
> @@ -354,7 +375,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
>  			return false;
>  
>  		sk_reset_timer(sk, &add_entry->add_timer,
> -			       jiffies + mptcp_get_add_addr_timeout_max(net));
> +			       jiffies + mptcp_get_add_addr_timeout(msk, net));
>  		return true;
>  	}
>  
> @@ -370,7 +391,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
>  
>  	timer_setup(&add_entry->add_timer, mptcp_pm_add_timer, 0);
>  	sk_reset_timer(sk, &add_entry->add_timer,
> -		       jiffies + mptcp_get_add_addr_timeout_max(net));
> +		       jiffies + mptcp_get_add_addr_timeout(msk, net));
>  
>  	return true;
>  }

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