| Message ID | 20210419213836.59887-1-mathew.j.martineau@linux.intel.com |
|---|---|
| State | Superseded, archived |
| Commit | 7137c5d18e294428b8258e703625c24cb5b93df3 |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | [mptcp-next] mptcp: Retransmit DATA_FIN | expand |
Hello, On Mon, 2021-04-19 at 14:38 -0700, Mat Martineau wrote: > With this change, the MPTCP-level retransmission timer is used to resend > DATA_FIN. The retranmit timer is not stopped while waiting for a > MPTCP-level ACK of DATA_FIN, and retransmitted DATA_FINs are sent on all > subflows. The retry interval starts with a couple of attempts at > TCP_RTO_MIN and then doubles on each attempt, up to TCP_RTO_MAX. > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/146 > Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > --- > net/mptcp/protocol.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index a8180a917649..1fddaeeb8051 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -11,6 +11,7 @@ > #include <linux/netdevice.h> > #include <linux/sched/signal.h> > #include <linux/atomic.h> > +#include <linux/log2.h> > #include <net/sock.h> > #include <net/inet_common.h> > #include <net/inet_hashtables.h> > @@ -402,6 +403,15 @@ static void mptcp_set_timeout(const struct sock *sk, const struct sock *ssk) > mptcp_sk(sk)->timer_ival = tout > 0 ? tout : TCP_RTO_MIN; > } > > +static void mptcp_set_datafin_timeout(const struct sock *sk) > +{ > + struct inet_connection_sock *icsk = inet_csk(sk); > + int retransmits = clamp_val(icsk->icsk_retransmits, 1, > + ilog2(TCP_RTO_MAX / TCP_RTO_MIN)); > + > + mptcp_sk(sk)->timer_ival = TCP_RTO_MIN << retransmits; The patch LGTM! I have only a couple of minor comment WRT to the above. To me something alike: mptcp_sk(sk)->timer_ival = min(TCP_RTO_MAX, TCP_RTO_MIN << icsk->icsk_retransmits); would be more readable. Also I'm wondering if we could use max subflows 'icsk->icsk_rto' instead of TCP_RTO_MIN - so that e.g. we are not too "aggressive" on slow links The latter could be really a follow-up, if needed at all. Thanks! Paolo
On Wed, 21 Apr 2021, Paolo Abeni wrote: > Hello, > > On Mon, 2021-04-19 at 14:38 -0700, Mat Martineau wrote: >> With this change, the MPTCP-level retransmission timer is used to resend >> DATA_FIN. The retranmit timer is not stopped while waiting for a >> MPTCP-level ACK of DATA_FIN, and retransmitted DATA_FINs are sent on all >> subflows. The retry interval starts with a couple of attempts at >> TCP_RTO_MIN and then doubles on each attempt, up to TCP_RTO_MAX. >> >> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/146 >> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> >> --- >> net/mptcp/protocol.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index a8180a917649..1fddaeeb8051 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -11,6 +11,7 @@ >> #include <linux/netdevice.h> >> #include <linux/sched/signal.h> >> #include <linux/atomic.h> >> +#include <linux/log2.h> >> #include <net/sock.h> >> #include <net/inet_common.h> >> #include <net/inet_hashtables.h> >> @@ -402,6 +403,15 @@ static void mptcp_set_timeout(const struct sock *sk, const struct sock *ssk) >> mptcp_sk(sk)->timer_ival = tout > 0 ? tout : TCP_RTO_MIN; >> } >> >> +static void mptcp_set_datafin_timeout(const struct sock *sk) >> +{ >> + struct inet_connection_sock *icsk = inet_csk(sk); >> + int retransmits = clamp_val(icsk->icsk_retransmits, 1, >> + ilog2(TCP_RTO_MAX / TCP_RTO_MIN)); >> + >> + mptcp_sk(sk)->timer_ival = TCP_RTO_MIN << retransmits; > > The patch LGTM! I have only a couple of minor comment WRT to the above. > To me something alike: > > mptcp_sk(sk)->timer_ival = min(TCP_RTO_MAX, > TCP_RTO_MIN << icsk->icsk_retransmits); > > would be more readable. Yeah, good point. I didn't want to start overflowing the result of TCP_RTO_MIN << x, but that would take an hour or two without any subflow catching up to the DATA_FIN sequence number. Probably too paranoid there. > > Also I'm wondering if we could use max subflows 'icsk->icsk_rto' > instead of TCP_RTO_MIN - so that e.g. we are not too "aggressive" on > slow links > > The latter could be really a follow-up, if needed at all. By "max subflows icsk_rto", you mean look through all the subflows and use the max icsk_rto? Follow-up sounds right to me. Thanks! -- Mat Martineau Intel
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index a8180a917649..1fddaeeb8051 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -11,6 +11,7 @@ #include <linux/netdevice.h> #include <linux/sched/signal.h> #include <linux/atomic.h> +#include <linux/log2.h> #include <net/sock.h> #include <net/inet_common.h> #include <net/inet_hashtables.h> @@ -402,6 +403,15 @@ static void mptcp_set_timeout(const struct sock *sk, const struct sock *ssk) mptcp_sk(sk)->timer_ival = tout > 0 ? tout : TCP_RTO_MIN; } +static void mptcp_set_datafin_timeout(const struct sock *sk) +{ + struct inet_connection_sock *icsk = inet_csk(sk); + int retransmits = clamp_val(icsk->icsk_retransmits, 1, + ilog2(TCP_RTO_MAX / TCP_RTO_MIN)); + + mptcp_sk(sk)->timer_ival = TCP_RTO_MIN << retransmits; +} + static bool tcp_can_send_ack(const struct sock *ssk) { return !((1 << inet_sk_state_load(ssk)) & @@ -1061,7 +1071,8 @@ static void __mptcp_clean_una(struct sock *sk) } } - if (snd_una == READ_ONCE(msk->snd_nxt)) { + if (snd_una == READ_ONCE(msk->snd_nxt) && + !mptcp_data_fin_enabled(msk)) { if (msk->timer_ival) mptcp_stop_timer(sk); } else { @@ -2287,8 +2298,19 @@ static void __mptcp_retrans(struct sock *sk) __mptcp_clean_una_wakeup(sk); dfrag = mptcp_rtx_head(sk); - if (!dfrag) + if (!dfrag) { + if (mptcp_data_fin_enabled(msk)) { + struct inet_connection_sock *icsk = inet_csk(sk); + + icsk->icsk_retransmits++; + mptcp_set_datafin_timeout(sk); + mptcp_send_ack(msk); + + goto reset_timer; + } + return; + } ssk = mptcp_subflow_get_retrans(msk); if (!ssk)
With this change, the MPTCP-level retransmission timer is used to resend DATA_FIN. The retranmit timer is not stopped while waiting for a MPTCP-level ACK of DATA_FIN, and retransmitted DATA_FINs are sent on all subflows. The retry interval starts with a couple of attempts at TCP_RTO_MIN and then doubles on each attempt, up to TCP_RTO_MAX. Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/146 Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> --- net/mptcp/protocol.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)