Messages in this thread | | | From | Neal Cardwell <> | Date | Thu, 27 Jul 2023 21:44:26 -0700 | Subject | Re: [PATCH net-next 3/3] net: tcp: check timeout by icsk->icsk_timeout in tcp_retransmit_timer() |
| |
On Thu, Jul 27, 2023 at 7:57 PM Menglong Dong <menglong8.dong@gmail.com> wrote: > > On Fri, Jul 28, 2023 at 3:31 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Thu, Jul 27, 2023 at 2:52 PM <menglong8.dong@gmail.com> wrote: > > > > > > From: Menglong Dong <imagedong@tencent.com> > > > > > > In tcp_retransmit_timer(), a window shrunk connection will be regarded > > > as timeout if 'tcp_jiffies32 - tp->rcv_tstamp > TCP_RTO_MAX'. This is not > > > right all the time. > > > > > > The retransmits will become zero-window probes in tcp_retransmit_timer() > > > if the 'snd_wnd==0'. Therefore, the icsk->icsk_rto will come up to > > > TCP_RTO_MAX sooner or later. > > > > > > However, the timer is not precise enough, as it base on timer wheel. > > > Sorry that I am not good at timer, but I know the concept of time-wheel. > > > The longer of the timer, the rougher it will be. So the timeout is not > > > triggered after TCP_RTO_MAX, but 122877ms as I tested. > > > > > > Therefore, 'tcp_jiffies32 - tp->rcv_tstamp > TCP_RTO_MAX' is always true > > > once the RTO come up to TCP_RTO_MAX. > > > > > > Fix this by replacing the 'tcp_jiffies32' with '(u32)icsk->icsk_timeout', > > > which is exact the timestamp of the timeout. > > > > > > Signed-off-by: Menglong Dong <imagedong@tencent.com> > > > --- > > > net/ipv4/tcp_timer.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c > > > index 470f581eedd4..3a20db15a186 100644 > > > --- a/net/ipv4/tcp_timer.c > > > +++ b/net/ipv4/tcp_timer.c > > > @@ -511,7 +511,11 @@ void tcp_retransmit_timer(struct sock *sk) > > > tp->snd_una, tp->snd_nxt); > > > } > > > #endif > > > - if (tcp_jiffies32 - tp->rcv_tstamp > TCP_RTO_MAX) { > > > + /* It's a little rough here, we regard any valid packet that > > > + * update tp->rcv_tstamp as the reply of the retransmitted > > > + * packet. > > > + */ > > > + if ((u32)icsk->icsk_timeout - tp->rcv_tstamp > TCP_RTO_MAX) { > > > tcp_write_err(sk); > > > goto out; > > > } > > > > > > Hmm, this looks like a net candidate, since this is unrelated to the > > other patches ? > > Yeah, this patch can be standalone. However, considering the > purpose of this series, it is necessary. Without this patch, the > OOM probe will always timeout after a few minutes. > > I'm not sure if I express the problem clearly in the commit log. > Let's explain it more. > > Let's mark the timestamp of the 10th timeout of the rtx timer > as TS1. Now, the retransmission happens and the ACK of > the retransmitted packet will update the tp->rcv_tstamp to > TS1+rtt. > > The RTO now is TCP_RTO_MAX. So let's see what will > happen in the 11th timeout. As we timeout after 122877ms, > so tcp_jiffies32 now is "TS1+122877ms", and > "tcp_jiffies32 - tp->rcv_tstamp" is > "TS1+122877ms - (TS1+rtt)" -> "122877ms - rtt", > which is always bigger than TCP_RTO_MAX, which is 120000ms. > > > > > Neal, what do you think ?
Sorry, I am probably missing something here, but: what would ever make this new proposed condition ((u32)icsk->icsk_timeout - tp->rcv_tstamp > TCP_RTO_MAX) true? :-)
In your nicely explained scenario, your new expression, icsk->icsk_timeout - tp->rcv_tstamp, will be:
icsk->icsk_timeout - tp->rcv_tstamp = TS1 + 120 sec - (TS1+rtt) = 120 sec - RTT
AFAICT there is no way for that expression to be bigger than TCP_RTO_MAX = 120 sec unless somehow RTT is negative. :-)
So AFAICT your expression ((u32)icsk->icsk_timeout - tp->rcv_tstamp > TCP_RTO_MAX) will always be false, so rather than this patch we may as well remove the if check and the body of the if block?
To me such a change does not seem like a safe and clear bug fix for the "net" branch but rather a riskier design change (appropriate for "net-next" branch) that has connections retry forever when the receiver retracts the window to zero, under the estimation that this is preferable to having the connections die in such a case.
There might be apps that depend on the old behavior of having connections die in such cases, so we might want to have this new fail-faster behavior guarded by a sysctl in case some sites need to revert to the older behavior? Not sure...
neal
| |