From: Enke Chen <[email protected]>
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 <[email protected]>
Reviewed-by: Neal Cardwell <[email protected]>
---
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(-)
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.
--
2.29.2
On Fri, Jan 22, 2021 at 8:13 PM Enke Chen <[email protected]> wrote:
>
> From: Enke Chen <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Neal Cardwell <[email protected]>
> ---
SGTM, thanks !
Signed-off-by: Eric Dumazet <[email protected]>
On Fri, 22 Jan 2021 11:13:06 -0800 Enke Chen wrote:
> From: Enke Chen <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Neal Cardwell <[email protected]>
This is targeting net, any guidance on Fixes / backporting?
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Reviewed-by: Neal Cardwell <[email protected]>
>
> This is targeting net, any guidance on Fixes / backporting?
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")
?
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
On Fri, Jan 22, 2021 at 9:45 PM Enke Chen <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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!