diff mbox series

[net] tcp: make TCP_USER_TIMEOUT accurate for zero window probes

Message ID 20210122191306.GA99540@localhost.localdomain (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series [net] tcp: make TCP_USER_TIMEOUT accurate for zero window probes | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 1498 this patch: 1498
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 49 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 1505 this patch: 1505
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Enke Chen Jan. 22, 2021, 7:13 p.m. UTC 
From: Enke Chen <enchen@paloaltonetworks.com>

The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
timer has backoff with a max interval of about two minutes, the
actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.

In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
into account when computing the timer value for the 0-window probes.

This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
helper to improve accuracy").

Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
Reviewed-by: Neal Cardwell <ncardwell@google.com>
---
 include/net/tcp.h     |  1 +
 net/ipv4/tcp_input.c  |  4 ++--
 net/ipv4/tcp_output.c |  2 ++
 net/ipv4/tcp_timer.c  | 18 ++++++++++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

Comments

Eric Dumazet Jan. 22, 2021, 8:41 p.m. UTC | #1 
On Fri, Jan 22, 2021 at 8:13 PM Enke Chen <enkechen2020@gmail.com> wrote:
>
> From: Enke Chen <enchen@paloaltonetworks.com>
>
> The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
> timer has backoff with a max interval of about two minutes, the
> actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.
>
> In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
> into account when computing the timer value for the 0-window probes.
>
> This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
> RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
> helper to improve accuracy").
>
> Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
> Reviewed-by: Neal Cardwell <ncardwell@google.com>
> ---

SGTM, thanks !

Signed-off-by: Eric Dumazet <edumazet@google.com>
Jakub Kicinski Jan. 23, 2021, 1:43 a.m. UTC | #2 
On Fri, 22 Jan 2021 11:13:06 -0800 Enke Chen wrote:
> From: Enke Chen <enchen@paloaltonetworks.com>
> 
> The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
> timer has backoff with a max interval of about two minutes, the
> actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.
> 
> In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
> into account when computing the timer value for the 0-window probes.
> 
> This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
> RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
> helper to improve accuracy").
> 
> Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
> Reviewed-by: Neal Cardwell <ncardwell@google.com>

This is targeting net, any guidance on Fixes / backporting?
Enke Chen Jan. 23, 2021, 2:28 a.m. UTC | #3 
Hi, Jakub:

In terms of backporting, this patch should go together with:

    9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

Thanks.  -- Enke

On Fri, Jan 22, 2021 at 05:43:25PM -0800, Jakub Kicinski wrote:
> On Fri, 22 Jan 2021 11:13:06 -0800 Enke Chen wrote:
> > From: Enke Chen <enchen@paloaltonetworks.com>
> > 
> > The TCP_USER_TIMEOUT is checked by the 0-window probe timer. As the
> > timer has backoff with a max interval of about two minutes, the
> > actual timeout for TCP_USER_TIMEOUT can be off by up to two minutes.
> > 
> > In this patch the TCP_USER_TIMEOUT is made more accurate by taking it
> > into account when computing the timer value for the 0-window probes.
> > 
> > This patch is similar to the one that made TCP_USER_TIMEOUT accurate for
> > RTOs in commit b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout()
> > helper to improve accuracy").
> > 
> > Signed-off-by: Enke Chen <enchen@paloaltonetworks.com>
> > Reviewed-by: Neal Cardwell <ncardwell@google.com>
> 
> This is targeting net, any guidance on Fixes / backporting?
Jakub Kicinski Jan. 23, 2021, 2:34 a.m. UTC | #4 
On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:
> Hi, Jakub:
> 
> In terms of backporting, this patch should go together with:
> 
>     9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

As in it:

Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

or does it further fix the same issue, so:

Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

?
Enke Chen Jan. 23, 2021, 2:45 a.m. UTC | #5 
Hi, Jakub:

On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:
> On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:
> > Hi, Jakub:
> > 
> > In terms of backporting, this patch should go together with:
> > 
> >     9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> 
> As in it:
> 
> Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> 
> or does it further fix the same issue, so:
> 
> Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")
>
> ?

Let me clarify:

1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window

   fixes the bug and makes it work.

2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
   It's independent.

With 1) and 2), the known issues with TCP_USER_TIMEOUT for 0-window probes
would be resolved.

Thanks.   -- Enke
Neal Cardwell Jan. 24, 2021, 12:19 a.m. UTC | #6 
On Fri, Jan 22, 2021 at 9:45 PM Enke Chen <enkechen2020@gmail.com> wrote:
>
> Hi, Jakub:
>
> On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:
> > On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:
> > > Hi, Jakub:
> > >
> > > In terms of backporting, this patch should go together with:
> > >
> > >     9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> >
> > As in it:
> >
> > Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> >
> > or does it further fix the same issue, so:
> >
> > Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")
> >
> > ?
>
> Let me clarify:
>
> 1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
>
>    fixes the bug and makes it work.
>
> 2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
>    It's independent.

Patch (2) ("tcp: make TCP_USER_TIMEOUT accurate for zero window
probes") is indeed conceptually independent of (1) but its
implementation depends on the icsk_probes_tstamp field defined in (1),
so AFAICT (2) cannot be backported further back than (1).

Patch (1) fixes a bug in 5.1:
    Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")

So probably (1) and (2) should be backported as a pair, and only back
as far as 5.1. (That covers 2 LTS kernels, 5.4 and 5.10, so hopefully
that is good enough.)

neal
Enke Chen Jan. 24, 2021, 12:56 a.m. UTC | #7 
Hi, Neal:

What you described is more accurate, and is correct.

Thanks.  -- Enke

On Sat, Jan 23, 2021 at 07:19:13PM -0500, Neal Cardwell wrote:
> On Fri, Jan 22, 2021 at 9:45 PM Enke Chen <enkechen2020@gmail.com> wrote:
> >
> > Hi, Jakub:
> >
> > On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:
> > > On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:
> > > > Hi, Jakub:
> > > >
> > > > In terms of backporting, this patch should go together with:
> > > >
> > > >     9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> > >
> > > As in it:
> > >
> > > Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> > >
> > > or does it further fix the same issue, so:
> > >
> > > Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")
> > >
> > > ?
> >
> > Let me clarify:
> >
> > 1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> >
> >    fixes the bug and makes it work.
> >
> > 2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
> >    It's independent.
> 
> Patch (2) ("tcp: make TCP_USER_TIMEOUT accurate for zero window
> probes") is indeed conceptually independent of (1) but its
> implementation depends on the icsk_probes_tstamp field defined in (1),
> so AFAICT (2) cannot be backported further back than (1).
> 
> Patch (1) fixes a bug in 5.1:
>     Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")
> 
> So probably (1) and (2) should be backported as a pair, and only back
> as far as 5.1. (That covers 2 LTS kernels, 5.4 and 5.10, so hopefully
> that is good enough.)
> 
> neal
Jakub Kicinski Jan. 24, 2021, 3:36 a.m. UTC | #8 
On Sat, 23 Jan 2021 16:56:43 -0800 Enke Chen wrote:
> On Sat, Jan 23, 2021 at 07:19:13PM -0500, Neal Cardwell wrote:
> > On Fri, Jan 22, 2021 at 9:45 PM Enke Chen <enkechen2020@gmail.com> wrote:  
> > > On Fri, Jan 22, 2021 at 06:34:24PM -0800, Jakub Kicinski wrote:  
> > > > On Fri, 22 Jan 2021 18:28:23 -0800 Enke Chen wrote:  
> > > > > In terms of backporting, this patch should go together with:
> > > > >
> > > > >     9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window  
> > > >
> > > > As in it:
> > > >
> > > > Fixes: 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> > > >
> > > > or does it further fix the same issue, so:
> > > >
> > > > Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")
> > > >
> > > > ?  
> > >
> > > Let me clarify:
> > >
> > > 1) 9d9b1ee0b2d1 tcp: fix TCP_USER_TIMEOUT with zero window
> > >
> > >    fixes the bug and makes it work.
> > >
> > > 2) The current patch makes the TCP_USER_TIMEOUT accurate for 0-window probes.
> > >    It's independent.  
> > 
> > Patch (2) ("tcp: make TCP_USER_TIMEOUT accurate for zero window
> > probes") is indeed conceptually independent of (1) but its
> > implementation depends on the icsk_probes_tstamp field defined in (1),
> > so AFAICT (2) cannot be backported further back than (1).
> > 
> > Patch (1) fixes a bug in 5.1:
> >     Fixes: 9721e709fa68 ("tcp: simplify window probe aborting on USER_TIMEOUT")
> > 
> > So probably (1) and (2) should be backported as a pair, and only back
> > as far as 5.1. (That covers 2 LTS kernels, 5.4 and 5.10, so hopefully
> > that is good enough.)
> 
> What you described is more accurate, and is correct.

That makes it clear.

I added a Fixes tag, reworded the message slightly and applied, thanks!
diff mbox series

Patch

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 78d13c88720f..ca7e2c6cc663 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -630,6 +630,7 @@  static inline void tcp_clear_xmit_timers(struct sock *sk)
 
 unsigned int tcp_sync_mss(struct sock *sk, u32 pmtu);
 unsigned int tcp_current_mss(struct sock *sk);
+u32 tcp_clamp_probe0_to_user_timeout(const struct sock *sk, u32 when);
 
 /* Bound MSS / TSO packet size with the half of the window */
 static inline int tcp_bound_to_half_wnd(struct tcp_sock *tp, int pktsize)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bafcab75f425..4923cdbea95a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3392,8 +3392,8 @@  static void tcp_ack_probe(struct sock *sk)
 	} else {
 		unsigned long when = tcp_probe0_when(sk, TCP_RTO_MAX);
 
-		tcp_reset_xmit_timer(sk, ICSK_TIME_PROBE0,
-				     when, TCP_RTO_MAX);
+		when = tcp_clamp_probe0_to_user_timeout(sk, when);
+		tcp_reset_xmit_timer(sk, ICSK_TIME_PROBE0, when, TCP_RTO_MAX);
 	}
 }
 
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index ab458697881e..8478cf749821 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -4099,6 +4099,8 @@  void tcp_send_probe0(struct sock *sk)
 		 */
 		timeout = TCP_RESOURCE_PROBE_INTERVAL;
 	}
+
+	timeout = tcp_clamp_probe0_to_user_timeout(sk, timeout);
 	tcp_reset_xmit_timer(sk, ICSK_TIME_PROBE0, timeout, TCP_RTO_MAX);
 }
 
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 454732ecc8f3..90722e30ad90 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -40,6 +40,24 @@  static u32 tcp_clamp_rto_to_user_timeout(const struct sock *sk)
 	return min_t(u32, icsk->icsk_rto, msecs_to_jiffies(remaining));
 }
 
+u32 tcp_clamp_probe0_to_user_timeout(const struct sock *sk, u32 when)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	u32 remaining;
+	s32 elapsed;
+
+	if (!icsk->icsk_user_timeout || !icsk->icsk_probes_tstamp)
+		return when;
+
+	elapsed = tcp_jiffies32 - icsk->icsk_probes_tstamp;
+	if (unlikely(elapsed < 0))
+		elapsed = 0;
+	remaining = msecs_to_jiffies(icsk->icsk_user_timeout) - elapsed;
+	remaining = max_t(u32, remaining, TCP_TIMEOUT_MIN);
+
+	return min_t(u32, remaining, when);
+}
+
 /**
  *  tcp_write_err() - close socket and save error info
  *  @sk:  The socket the error has appeared on.