diff mbox series

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

Message ID 20210122191306.GA99540@localhost.localdomain
State New
Headers show
Series [net] tcp: make TCP_USER_TIMEOUT accurate for zero window probes | expand

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.