| Message ID | 20230811023747.12065-1-kerneljasonxing@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e4dd0d3a2f64b8bd8029ec70f52bdbebd0644408 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: fix the RTO timer retransmitting skb every 1ms if linear option is enabled | expand |
On Fri, Aug 11, 2023 at 10:38 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > From: Jason Xing <kernelxing@tencent.com> > > In the real workload, I encountered an issue which could cause the RTO > timer to retransmit the skb per 1ms with linear option enabled. The amount > of lost-retransmitted skbs can go up to 1000+ instantly. > > The root cause is that if the icsk_rto happens to be zero in the 6th round > (which is the TCP_THIN_LINEAR_RETRIES value), then it will always be zero > due to the changed calculation method in tcp_retransmit_timer() as follows: > > icsk->icsk_rto = min(icsk->icsk_rto << 1, TCP_RTO_MAX); > > Above line could be converted to > icsk->icsk_rto = min(0 << 1, TCP_RTO_MAX) = 0 > > Therefore, the timer expires so quickly without any doubt. > > I read through the RFC 6298 and found that the RTO value can be rounded > up to a certain value, in Linux, say TCP_RTO_MIN as default, which is > regarded as the lower bound in this patch as suggested by Eric. > > Fixes: 36e31b0af587 ("net: TCP thin linear timeouts") > Suggested-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Jason Xing <kernelxing@tencent.com> Hello maintainers, I wonder why someone in the patchwork[1] changed this v2 patch into Superseded status without comments? Should I convert it to a new status or something else? [1]: https://patchwork.kernel.org/project/netdevbpf/patch/20230811023747.12065-1-kerneljasonxing@gmail.com/ Thanks, Jason > --- > v2: > 1) nit: alway->always and the indentation style suggested by Simon. > --- > net/ipv4/tcp_timer.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c > index d45c96c7f5a4..69795b273419 100644 > --- a/net/ipv4/tcp_timer.c > +++ b/net/ipv4/tcp_timer.c > @@ -599,7 +599,9 @@ void tcp_retransmit_timer(struct sock *sk) > tcp_stream_is_thin(tp) && > icsk->icsk_retransmits <= TCP_THIN_LINEAR_RETRIES) { > icsk->icsk_backoff = 0; > - icsk->icsk_rto = min(__tcp_set_rto(tp), TCP_RTO_MAX); > + icsk->icsk_rto = clamp(__tcp_set_rto(tp), > + tcp_rto_min(sk), > + TCP_RTO_MAX); > } else if (sk->sk_state != TCP_SYN_SENT || > icsk->icsk_backoff > > READ_ONCE(net->ipv4.sysctl_tcp_syn_linear_timeouts)) { > -- > 2.37.3 >
On Tue, 15 Aug 2023 10:08:13 +0800 Jason Xing wrote: > I wonder why someone in the patchwork[1] changed this v2 patch into > Superseded status without comments? Should I convert it to a new > status or something else? Ack, looks like a mistake, let me bring it back.
On Tue, Aug 15, 2023 at 10:24 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 15 Aug 2023 10:08:13 +0800 Jason Xing wrote: > > I wonder why someone in the patchwork[1] changed this v2 patch into > > Superseded status without comments? Should I convert it to a new > > status or something else? > > Ack, looks like a mistake, let me bring it back. Thanks. Jason > -- > pw-bot: under-review
On Fri, Aug 11, 2023 at 4:38 AM Jason Xing <kerneljasonxing@gmail.com> wrote: > > From: Jason Xing <kernelxing@tencent.com> > > In the real workload, I encountered an issue which could cause the RTO > timer to retransmit the skb per 1ms with linear option enabled. The amount > of lost-retransmitted skbs can go up to 1000+ instantly. > > The root cause is that if the icsk_rto happens to be zero in the 6th round > (which is the TCP_THIN_LINEAR_RETRIES value), then it will always be zero > due to the changed calculation method in tcp_retransmit_timer() as follows: > > icsk->icsk_rto = min(icsk->icsk_rto << 1, TCP_RTO_MAX); > > Above line could be converted to > icsk->icsk_rto = min(0 << 1, TCP_RTO_MAX) = 0 > > Therefore, the timer expires so quickly without any doubt. > > I read through the RFC 6298 and found that the RTO value can be rounded > up to a certain value, in Linux, say TCP_RTO_MIN as default, which is > regarded as the lower bound in this patch as suggested by Eric. > > Fixes: 36e31b0af587 ("net: TCP thin linear timeouts") > Suggested-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Jason Xing <kernelxing@tencent.com> Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Fri, 11 Aug 2023 10:37:47 +0800 you wrote: > From: Jason Xing <kernelxing@tencent.com> > > In the real workload, I encountered an issue which could cause the RTO > timer to retransmit the skb per 1ms with linear option enabled. The amount > of lost-retransmitted skbs can go up to 1000+ instantly. > > The root cause is that if the icsk_rto happens to be zero in the 6th round > (which is the TCP_THIN_LINEAR_RETRIES value), then it will always be zero > due to the changed calculation method in tcp_retransmit_timer() as follows: > > [...] Here is the summary with links: - [v2,net] net: fix the RTO timer retransmitting skb every 1ms if linear option is enabled https://git.kernel.org/netdev/net/c/e4dd0d3a2f64 You are awesome, thank you!
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index d45c96c7f5a4..69795b273419 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -599,7 +599,9 @@ void tcp_retransmit_timer(struct sock *sk) tcp_stream_is_thin(tp) && icsk->icsk_retransmits <= TCP_THIN_LINEAR_RETRIES) { icsk->icsk_backoff = 0; - icsk->icsk_rto = min(__tcp_set_rto(tp), TCP_RTO_MAX); + icsk->icsk_rto = clamp(__tcp_set_rto(tp), + tcp_rto_min(sk), + TCP_RTO_MAX); } else if (sk->sk_state != TCP_SYN_SENT || icsk->icsk_backoff > READ_ONCE(net->ipv4.sysctl_tcp_syn_linear_timeouts)) {