Message-ID: <CADVnQyn1tNaAYyOA98oyV_8d0k8VA24Z4kNVcB3=QLt1Qxz6=w@mail.gmail.com>
Date: Fri, 17 May 2024 14:35:22 -0400
From: Neal Cardwell <ncardwell@...gle.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: edumazet@...gle.com, dsahern@...nel.org, kuba@...nel.org, 
	pabeni@...hat.com, davem@...emloft.net, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>, Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [RFC PATCH net-next] tcp: break the limitation of initial receive window

On Fri, May 17, 2024 at 1:49 PM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> On Sat, May 18, 2024 at 1:41 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > On Fri, May 17, 2024 at 10:42 PM Neal Cardwell <ncardwell@...gle.com> wrote:
> > >
> > > On Fri, May 17, 2024 at 4:50 AM Jason Xing <kerneljasonxing@...il.com> wrote:
> > > >
> > > > From: Jason Xing <kernelxing@...cent.com>
> > > >
> > > > Since in 2018 one commit a337531b942b ("tcp: up initial rmem to 128KB and
> > > > SYN rwin to around 64KB") limited received window within 65535, most CDN
> > > > team would not benefit from this change because they cannot have a large
> > > > window to receive a big packet one time 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."
> > > >
> > > > 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. Besides,
> > > > there are many papers conducting various interesting experiments which
> > > > have something to do with this window and show good outputs in some cases.
> > > >
> > > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > > ---
> > > >  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;
> > >
> > > Hmm, has this patch been tested? This doesn't look like it would work.
> >
> > Hello Neal,
> >
> > Thanks for the comment.
> >
> > Sure, I provided such a patch a few months ago which has been tested
> > in production for the customers.
> >
> > One example of using a much bigger initial receive window:
> > client   ---window=65535---> server
> > client   <---window=14600----  server
> > client   ---window=175616---> server
> >
> > Then the client could send more data than before in fewer rtt.
> >
> > Above is the output of tcpdump.
> >
> > Oh, I just found a similar case:
> > https://lore.kernel.org/all/20220213040545.365600-1-tilan7663@gmail.com/
> >
> > Before this, I always believed I'm not the only one who had such an issue.
> >
> > >
> > > Please note that RFC 7323 says in
> > > https://datatracker.ietf.org/doc/html/rfc7323#section-2.2 :
> > >
> > >    The window field in a segment where the SYN bit is set (i.e., a <SYN>
> > >    or <SYN,ACK>) MUST NOT be scaled.
> > >
> > > Since the receive window field in a SYN is unscaled, that means the
> > > TCP wire protocol has no way to convey a receive window in the SYN
> > > that is bigger than 64KBytes.
> > >
> > > That is why this code places a limit of U16_MAX on the value here.
> > >
> > > If you want to advertise a bigger receive window in the SYN, you'll
> >
> > No. It's not my original intention.
> >
> > For SYN packet itself is limited in the __tcp_transmit_skb() as below:
> >
> >     th->window      = htons(min(tp->rcv_wnd, 65535U));
>
> With this limitation/protection of the window in SYN packet, It would
> not break RFC with this patch applied. I try to advertise a bigger
> initRwnd of ACK in a 3-way shakehand process.

Thanks for the explanation.

I think the confusion arose because in your title ("tcp: break the
limitation of initial receive window"), I interpreted "initial receive
window" as the initial receive window advertised on the wire (which is
limited by protocol spec to 64 kbytes), when you meant the initial
value of tp->rcv_wnd. There are similar ambiguities in the commit
message body.

I would suggest resubmitting a version of the patch with a revised
commit title and commit description, to clarify at least the following
issues:

+ For the patch title, perhaps something like:
  tcp: remove 64 KByte limit for initial tp->rcv_wnd value

+ For the commit description, in the sentence 'Since in 2018 one
commit a337531b942b ("tcp: up initial rmem to 128KB and SYN rwin to
around 64KB") limited received window within 65535', please revise
this to clarify that you are talking about tp->rcv_wnd and not the
receive window on the wire. For example: '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,.'

+ For the commit description, please add a note that RFC 7323 limits
the initial receive window on the wire in a SYN or SYN+ACK to 65535
bytes, and __tcp_transmit_skb() already ensures that constraint is
respected, no matter how large tp->rcv_wnd is.

+ For the commit description, please include some version of your
example of the receive window values in a handshake, like:

One example of using a much bigger initial receive window:
client   --- SYN: rwindow=65535 ---> server
client   <--- SYN+ACK: rwindow=14600 ----  server
client   --- ACK: rwindow=175616 ---> server

+ For the commit description, please include some version of your
example of the evolution of the receive window over the first 3 round
trips, before and after your patch.

thanks,
neal

Powered by blists - more mailing lists