netdev - Re: [PATCH net] tcp: remove 64 KByte limit for initial tp->rcv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 22 May 2024 10:08:54 -0400
From: Neal Cardwell <ncardwell@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jason Xing <kerneljasonxing@...il.com>, dsahern@...nel.org, kuba@...nel.org, 
	pabeni@...hat.com, davem@...emloft.net, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net] tcp: remove 64 KByte limit for initial tp->rcv_wnd value

On Wed, May 22, 2024 at 2:52 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Tue, May 21, 2024 at 3:42 PM Jason Xing <kerneljasonxing@...il.com> wrote:
> >
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > Recently, we had some servers upgraded to the latest kernel and noticed
> > the indicator from the user side showed worse results than before. It is
> > caused by the limitation of tp->rcv_wnd.
> >
> > 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 teams 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. Small rcv_wnd means slow transfer speed, to some extent. It's
> > the side effect for the latency/time-sensitive users.
> >
> > 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. The change doesn't violate
> > RFC.
> >
> > 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.
> >
> > When we apply such a patch, having a large rcv_wnd if the user tweak this
> > knob can help transfer data more rapidly and save some rtts.
> >
> > Fixes: a337531b942b ("tcp: up initial rmem to 128KB and SYN rwin to around 64KB")
> > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > ---
>
> Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Acked-by: Neal Cardwell <ncardwell@...gle.com>

Jason, thanks for the patch!

neal