| Message ID | 1623219886-69610-1-git-send-email-liyonglong@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | mptcp: use icsk_rto of first subflow as ADD_ADDR timeout value | expand |
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
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.
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);
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(-)