Message-ID: <CANn89iKRuxON3pWjivs0kU-XopBiqTZn4Mx+wOKHVmQ97zAU5A@mail.gmail.com>
Date: Wed, 15 May 2024 09:10:28 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@...cinc.com>
Cc: soheil@...gle.com, ncardwell@...gle.com, yyd@...gle.com, ycheng@...gle.com, 
	quic_stranche@...cinc.com, davem@...emloft.net, kuba@...nel.org, 
	netdev@...r.kernel.org
Subject: Re: Potential impact of commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")

On Wed, May 15, 2024 at 6:47 AM Subash Abhinov Kasiviswanathan (KS)
<quic_subashab@...cinc.com> wrote:
>
> We recently noticed that a device running a 6.6.17 kernel (A) was having
> a slower single stream download speed compared to a device running
> 6.1.57 kernel (B). The test here is over mobile radio with iperf3 with
> window size 4M from a third party server.
>
> The radio conditions are the same between the devices so that maximum
> achievable download speed will be the same. Both devices have the same
> tcp_rmem values.
>
> We captured tcpdump at the device and the main difference was that the
> receiver window advertised in the ACKs was going upto a maximum of ~2M
> in A vs ~6M in B. By explicitly increasing the window size in iperf3 in
> A, the download speed of A was able to match that of B which suggests
> that the RTT was high and needed a larger window size to achieve the
> download speed. We do not have tcp_shrink_window enabled.
>
> We noticed that there were some changes to window size done in commit
> dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale"). After
> reverting this commit in A, we observed that the receiver window
> advertised in ACKs matches B.
>
> We logged free_space, allowed_space and full_space in
> __tcp_select_window() in both devices and observed that the
> allowed_space was 6MB+ on both devices, however the full_space was
> smaller in A as the tp->window_clamp was smaller. The free_space was
> smaller in A compared to B which I believe is an expected consequence of
> the commit.
>
> Could you please advise on how we need to handle this. We are open to
> trying out any debug patches.

Hi Subash

I think you gave many details, but please give us more of them :

1) What driver is used on the receiver side.
2) MTU
3) cat /proc/sys/net/ipv4/tcp_rmem

Ideally, you could snapshot "ss -temoi dst <otherpeer>" on receive
side while the transfer is ongoing,
and possibly while stopping the receiver thread (kill -STOP `pidof iperf`)

TCP is sensitive to the skb->len/skb->truesize ratio.
Some drivers are known to provide 'bad skbs' in this regard.

Commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale") is
simply a step for dynamic
probing of skb->len/skb->truesize ratio, and give incentive for better
memory use.

Ultimately, TCP RWIN derives from effective memory usage.

Sending a too big RWIN can cause excessive memory usage or packet drops.
If you say RWIN was 6MB+ before the patch, this looks like a bug to me,
because tcp_rmem[2] = 6MB by default. There is no way a driver can
pack 6MB of TCP payload in 6MB of memory (no skb/headers overhead ???)
This would only work well in lossless networks, and if receiving
application drains TCP receive queue fast enough.

Please take a look at these relevant patches.
Note they are not perfect patches, because usbnet can still provide
'bad skbs', forcing TCP to send small RWIN.

d50729f1d60bca822ef6d9c1a5fb28d486bd7593 net: usb: smsc95xx: stop
lying about skb->truesize
05417aa9c0c038da2464a0c504b9d4f99814a23b net: usb: sr9700: stop lying
about skb->truesize
1b3b2d9e772b99ea3d0f1f2252bf7a1c94b88be6 net: usb: smsc75xx: stop
lying about skb->truesize
9aad6e45c4e7d16b2bb7c3794154b828fb4384b4 usb: aqc111: stop lying about
skb->truesize
4ce62d5b2f7aecd4900e7d6115588ad7f9acccca net: usb: ax88179_178a: stop
lying about skb->truesize

Powered by blists - more mailing lists