Message-ID: <CAL+tcoBzijiKSwJdy6AhLH2Po+8b3sKUScvEAzKLFJea_KGvsQ@mail.gmail.com>
Date: Tue, 21 May 2024 21:11:53 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com, 
	davem@...emloft.net, ncardwell@...gle.com, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [RFC PATCH v2 net-next] tcp: remove 64 KByte limit for initial
 tp->rcv_wnd value

On Tue, May 21, 2024 at 5:43 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, May 21, 2024 at 8:56 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > Hello Eric,
> >
> > On Tue, May 21, 2024 at 8:36 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > >
> > > Hello Eric,
> > >
> > > On Tue, May 21, 2024 at 12:51 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > > >
> > > > On Sat, May 18, 2024 at 4:50 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > > > >
> > > > > From: Jason Xing <kernelxing@...cent.com>
> > > > >
> > > > > In 2018 commit a337531b942b ("tcp: up initial rmem to 128KB and SYN rwin
> > > > > to around 64KB") limited the initial value of tp->rcv_wnd to 65535, most
> > > > > CDN team would not benefit from this change because they cannot have a
> > > > > large window to receive a big packet, which will be slowed down especially
> > > > > in long RTT.
> > > > >
> > > > > According to RFC 7323, it says:
> > > > >   "The maximum receive window, and therefore the scale factor, is
> > > > >    determined by the maximum receive buffer space."
> > > >
> > > > This seems not relevant ?  wscale factor is not changed in this patch ?
> > > > tp->rcv_wnd is also not the maximum receive window.
> > >
> > > Thanks for your review.
> > >
> > > I can remove this part. I was trying to claim I do not break RFC.
> > >
> > > >
> > > > >
> > > > > So we can get rid of this 64k limitation and let the window be tunable if
> > > > > the user wants to do it within the control of buffer space. Then many
> > > > > companies, I believe, can have the same behaviour as old days.
> > > >
> > > > Not sure this has ever worked, see below.
> > > >
> > > > Also, the "many companies ..." mention has nothing to do in a changelog.
> > >
> > > Oh, I just copied/translated from my initial studies of this rcv_wnd
> > > by reading many papers something like this.
> > >
> > > I can also remove this sentence.
> > >
> > > >
> > > >
> > > > > Besides,
> > > > > there are many papers conducting various interesting experiments which
> > > > > have something to do with this window and show good outputs in some cases,
> > > > > say, paper [1] in Yahoo! CDN.
> > > >
> > > > I think this changelog is trying hard to sell something, but in
> > > > reality TCP 3WHS nature
> > > > makes your claims wrong.
> > > >
> > > > Instead, you should clearly document that this problem can _not_ be
> > > > solved for both
> > > > active _and_ passive connections.
> > > >
> > > > In the first RTT, a client (active connection) can not send more than
> > > > 64KB, if TCP specs
> > > > are properly applied.
> > >
> > > Having a large rcv_wnd if the user can tweak this knob can help
> > > transfer data more rapidly. I'm not referring to the first RTT.
> > >
> > > >
> > > > >
> > > > > To avoid future confusion, current change doesn't affect the initial
> > > > > receive window on the wire in a SYN or SYN+ACK packet which are set within
> > > > > 65535 bytes according to RFC 7323 also due to the limit in
> > > > > __tcp_transmit_skb():
> > > > >
> > > > >     th->window      = htons(min(tp->rcv_wnd, 65535U));
> > > > >
> > > > > In one word, __tcp_transmit_skb() already ensures that constraint is
> > > > > respected, no matter how large tp->rcv_wnd is.
> > > > >
> > > > > Let me provide one example if with or without the patch:
> > > > > Before:
> > > > > client   --- SYN: rwindow=65535 ---> server
> > > > > client   <--- SYN+ACK: rwindow=65535 ----  server
> > > > > client   --- ACK: rwindow=65536 ---> server
> > > > > Note: for the last ACK, the calculation is 512 << 7.
> > > > >
> > > > > After:
> > > > > client   --- SYN: rwindow=65535 ---> server
> > > > > client   <--- SYN+ACK: rwindow=65535 ----  server
> > > > > client   --- ACK: rwindow=175232 ---> server
> > > > > Note: I use the following command to make it work:
> > > > > ip route change default via [ip] dev eth0 metric 100 initrwnd 120
> > > > > For the last ACK, the calculation is 1369 << 7.
> > > > >
> > > > > We can pay attention to the last ACK in 3-way shakehand and notice that
> > > > > with the patch applied the window can reach more than 64 KByte.
> > > >
> > > > You carefully avoided mentioning the asymmetry.
> > > > I do not think this is needed in the changelog, because this is adding
> > > > confusion.
> > >
> > > What kind of case I've met in production is only about whether we're
> > > capable of sending more data at the same time at the very beginning,
> > > so I care much more about the sending process right now.
> > >
> > > >
> > > > >
> > > > > [1]: https://conferences.sigcomm.org/imc/2011/docs/p569.pdf
> > > > >
> > > > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > > > ---
> > > > > v2
> > > > > Link: https://lore.kernel.org/all/20240517085031.18896-1-kerneljasonxing@gmail.com/
> > > > > 1. revise the title and body messages (Neal)
> > > > > ---
> > > > >  net/ipv4/tcp_output.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > > > > index 95caf8aaa8be..95618d0e78e4 100644
> > > > > --- a/net/ipv4/tcp_output.c
> > > > > +++ b/net/ipv4/tcp_output.c
> > > > > @@ -232,7 +232,7 @@ void tcp_select_initial_window(const struct sock *sk, int __space, __u32 mss,
> > > > >         if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_workaround_signed_windows))
> > > > >                 (*rcv_wnd) = min(space, MAX_TCP_WINDOW);
> > > > >         else
> > > > > -               (*rcv_wnd) = min_t(u32, space, U16_MAX);
> > > > > +               (*rcv_wnd) = space;
> > > >
> > > > This is probably breaking some  packetdrill tests, but your change
> > > > might [1] be good,
> > >
> > > I'll do some packetdrill tests and get back some information soon.
> >
> > I'm done with the packetdrill tests[1]. Here are two tests failed
> > after comparing with/without this patch:
> > 1) ./packetdrill/run_all.py -S -v -L -l tcp/ioctl/ioctl-siocinq-fin.pkt
> > 2) ./packetdrill/run_all.py -S -v -L -l
> > tcp/fastopen/server/client-ack-dropped-then-recovery-ms-timestamps.pkt
> >
> > For the first one, it shows:
> > "FAIL [/data/home/kernelxing/source_code/packetdrill/gtests/net/tcp/ioctl/ioctl-siocinq-fin.pkt
> > (ipv6)]
> > stdout:
> > stderr:
> > ioctl-siocinq-fin.pkt:28: error handling packet: timing error:
> > expected outbound packet at 0.302321 sec but happened at 0.342759 sec;
> > tolerance 0.004000 sec
> > script packet:  0.302321 . 1:1(0) ack 2002
> > actual packet:  0.342759 . 1:1(0) ack 2002 win 65535"
> >
> > For the second one, it shows:
> > "client-ack-dropped-then-recovery-ms-timestamps.pkt:33: error handling
> > packet: live packet field tcp_window: expected: 256 (0x100) vs actual:
> > 532 (0x214)
> > script packet:  0.012251 P. 1:5001(5000) ack 1001 win 256 <nop,nop,TS
> > val 2010 ecr 1000>
> > actual packet:  0.012242 P. 1:5001(5000) ack 1001 win 532 <nop,nop,TS
> > val 2010 ecr 1000>
> > Ran    3 tests:    0 passing,    3 failing,    0 timed out (0.91 sec):
> > tcp/fastopen/server/client-ack-dropped-then-recovery-ms-timestamps.pkt"
> >
> > The reason is unexpected window size. Since I removed the limit of
> > 64KB, It is expected from my view.
>
> I think you misunderstood what I was saying.
>
> Basically, this change will break some packetdrill tests, and this is fine,
> because those packetdrill tests were relying on a prior kernel behavior that
> was not set in stone (certainly not documented)
>
> >
> > [1]: https://github.com/google/packetdrill
> > Running: ./packetdrill/run_all.py -S -v -L -l tcp/
> >
> > I wonder if you mind this change which might be unpredictable, how
> > about this one:
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 95caf8aaa8be..3bf7d9fd2d6b 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -231,11 +231,13 @@ void tcp_select_initial_window(const struct sock
> > *sk, int __space, __u32 mss,
> >          */
> >         if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_workaround_signed_windows))
> >                 (*rcv_wnd) = min(space, MAX_TCP_WINDOW);
> > -       else
> > -               (*rcv_wnd) = min_t(u32, space, U16_MAX);
> >
> > -       if (init_rcv_wnd)
> > +       if (init_rcv_wnd) {
> > +               *rcv_wnd = space;
> >                 *rcv_wnd = min(*rcv_wnd, init_rcv_wnd * mss);
> > +       } else {
> > +               *rcv_wnd = min_t(u32, space, U16_MAX);
> > +       }
> >
> >         *rcv_wscale = 0;
> >         if (wscale_ok) {
> > ?
> >
> > It affects/changes the TCP stack only when the user tries to use 'ip
> > route' to set initrwnd.
>
> I much prefer the prior and simpler version.
>
> Only the changelog was not very good IMO.
>
> Also, I think this is fixing a bug and should target the net tree.
>
> If it took 6 years to discover the unwanted side effects, we should
> make sure the fix
> is backported by stable teams, thanks to an appropriate Fixes: tag.

Oh, I see. I'll submit a new one soon.

Thanks,
Jason

Powered by blists - more mailing lists