[PATCH] mptcp: use icsk_rto of first subflow as ADD_ADDR timeout value

[PATCH] mptcp: use icsk_rto of first subflow as ADD_ADDR timeout value

[Yonglong Li]

ADD_ADDR is sent on first subflow, so we can use icsk_rto of first
subflow as ADD_ADDR retransmission timeout value.

Change sysctl add_addr_timeout type to millsecond and default value
to TCP_RTO_MIN.

Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
---
 net/mptcp/ctrl.c       | 13 +++++++++----
 net/mptcp/pm_netlink.c |  5 +++--
 net/mptcp/protocol.h   |  2 +-
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
index 6c2639b..40e8579 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -36,9 +36,14 @@ int mptcp_is_enabled(struct net *net)
 	return mptcp_get_pernet(net)->mptcp_enabled;
 }
 
-unsigned int mptcp_get_add_addr_timeout(struct net *net)
+unsigned int mptcp_get_add_addr_timeout(struct net *net, struct mptcp_sock *msk)
 {
-	return mptcp_get_pernet(net)->add_addr_timeout;
+	unsigned int timeout = mptcp_get_pernet(net)->add_addr_timeout;
+
+	if (msk->first && inet_csk(msk->first)->icsk_rto)
+		timeout = inet_csk(msk->first)->icsk_rto;
+
+	return timeout;
 }
 
 int mptcp_is_checksum_enabled(struct net *net)
@@ -49,7 +54,7 @@ int mptcp_is_checksum_enabled(struct net *net)
 static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 {
 	pernet->mptcp_enabled = 1;
-	pernet->add_addr_timeout = TCP_RTO_MAX;
+	pernet->add_addr_timeout = TCP_RTO_MIN;
 	pernet->checksum_enabled = 0;
 }
 
@@ -70,7 +75,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
 		.procname = "add_addr_timeout",
 		.maxlen = sizeof(unsigned int),
 		.mode = 0644,
-		.proc_handler = proc_dointvec_jiffies,
+		.proc_handler = proc_dointvec_ms_jiffies,
 	},
 	{
 		.procname = "checksum_enabled",
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d4732a4..c53e500 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -333,7 +333,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(sock_net(sk)));
+			       jiffies + mptcp_get_add_addr_timeout(sock_net(sk), msk));
 
 	spin_unlock_bh(&msk->pm.lock);
 
@@ -385,9 +385,10 @@ static bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 	add_entry->sock = msk;
 	add_entry->retrans_times = 0;
 
+	pr_debug("msk=%p, timeout:%d", msk, mptcp_get_add_addr_timeout(net, 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(net));
+		       jiffies + mptcp_get_add_addr_timeout(net, msk));
 
 	return true;
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 160c2ab..6c2322e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -538,7 +538,7 @@ static inline void mptcp_subflow_delegated_done(struct mptcp_subflow_context *su
 }
 
 int mptcp_is_enabled(struct net *net);
-unsigned int mptcp_get_add_addr_timeout(struct net *net);
+unsigned int mptcp_get_add_addr_timeout(struct net *net, struct mptcp_sock *msk);
 int mptcp_is_checksum_enabled(struct net *net);
 void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
 				     struct mptcp_options_received *mp_opt);
-- 
1.8.3.1

Re: [PATCH] mptcp: use icsk_rto of first subflow as ADD_ADDR timeout value

[Mat Martineau]

On Wed, 9 Jun 2021, Yonglong Li wrote:

> ADD_ADDR is sent on first subflow, so we can use icsk_rto of first
> subflow as ADD_ADDR retransmission timeout value.
>
> Change sysctl add_addr_timeout type to millsecond and default value
> to TCP_RTO_MIN.
>
> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> ---
> net/mptcp/ctrl.c       | 13 +++++++++----
> net/mptcp/pm_netlink.c |  5 +++--
> net/mptcp/protocol.h   |  2 +-
> 3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 6c2639b..40e8579 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c
> @@ -36,9 +36,14 @@ int mptcp_is_enabled(struct net *net)
> 	return mptcp_get_pernet(net)->mptcp_enabled;
> }
>
> -unsigned int mptcp_get_add_addr_timeout(struct net *net)
> +unsigned int mptcp_get_add_addr_timeout(struct net *net, struct mptcp_sock *msk)
> {
> -	return mptcp_get_pernet(net)->add_addr_timeout;
> +	unsigned int timeout = mptcp_get_pernet(net)->add_addr_timeout;
> +
> +	if (msk->first && inet_csk(msk->first)->icsk_rto)
> +		timeout = inet_csk(msk->first)->icsk_rto;

Since the time to echo an ADD_ADDR may have factors other then 
network-level latency involved, I don't think the timeout should be based 
on icsk_rto.

Is there a specific issue you've encountered with slow ADD_ADDR retries?


> +
> +	return timeout;
> }
>
> int mptcp_is_checksum_enabled(struct net *net)
> @@ -49,7 +54,7 @@ int mptcp_is_checksum_enabled(struct net *net)
> static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
> {
> 	pernet->mptcp_enabled = 1;
> -	pernet->add_addr_timeout = TCP_RTO_MAX;
> +	pernet->add_addr_timeout = TCP_RTO_MIN;

TCP_RTO_MAX is probably too long. But TCP_RTO_MIN is certainly too short. 
While the current upstream Linux code will echo the ADD_ADDR at a fairly 
low level in the kernel, in the future a userspace path manager may be 
involved so the timing on the ADD_ADDR echo should not be too tight.

Maybe a change to a shorter timeout would be sufficient.


> 	pernet->checksum_enabled = 0;
> }
>
> @@ -70,7 +75,7 @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet)
> 		.procname = "add_addr_timeout",
> 		.maxlen = sizeof(unsigned int),
> 		.mode = 0644,
> -		.proc_handler = proc_dointvec_jiffies,
> +		.proc_handler = proc_dointvec_ms_jiffies,

There are a few issues with this part of the patch:

1. Changes the meaning of the sysctl
2. Not sure millisecond granularity is needed
3. Like above, don't want to allow very short timeouts that could affect 
userspace path managers

Like I asked above, it would be helpful to understand the problem you're 
solving so we can discuss possible solutions. It's also very helpful 
information to explain the motivation in the commit message.

Thanks for the patch,

--
Mat Martineau
Intel

Re: [PATCH] mptcp: use icsk_rto of first subflow as ADD_ADDR timeout value

[Yonglong Li]

On 2021/6/11 6:11, Mat Martineau wrote:
>>
> 
> There are a few issues with this part of the patch:
> 
> 1. Changes the meaning of the sysctl
> 2. Not sure millisecond granularity is needed
> 3. Like above, don't want to allow very short timeouts that could affect userspace path managers
> 
> Like I asked above, it would be helpful to understand the problem you're solving so we can discuss possible solutions. It's also very helpful information to explain the motivation in the commit message.

In my test if ADD_ADDR-echo process right after ADD_ADDR, ADD_ADDR event will be flush
by ADD_ADDR-echo, so ADD_ADDR will process after add_timer timeout. And the timeout of
add_timer too long to trigger before connection closed.

The key point of this issue: timeout of add_timer is too long. As your advice we can only
change to a shorter timeout.