Message-ID: <b3kspnkcbj2p3c5q6rbujih72n7vouafpreg5mjsrgvf4fpu52@545rpheaixni>
Date: Wed, 3 Apr 2024 09:30:48 -0700
From: Hechao Li <hli@...flix.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Soheil Hassas Yeganeh <soheil@...gle.com>, netdev@...r.kernel.org,
	kernel-developers@...flix.com, Tycho Andersen <tycho@...ho.pizza>
Subject: Re: [PATCH net-next] tcp: update window_clamp together with
 scaling_ratio

On 24/04/03 04:49PM, Eric Dumazet wrote:
> On Wed, Apr 3, 2024 at 4:22 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Tue, Apr 2, 2024 at 11:56 PM Hechao Li <hli@...flix.com> wrote:
> > >
> > > After commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale"),
> > > we noticed an application-level timeout due to reduced throughput. This
> > > can be reproduced by the following minimal client and server program.
> > >
> > > server:
> > >
> > ...
> > >
> > > Before the commit, it takes around 22 seconds to transfer 10M data.
> > > After the commit, it takes 40 seconds. Because our application has a
> > > 30-second timeout, this regression broke the application.
> > >
> > > The reason that it takes longer to transfer data is that
> > > tp->scaling_ratio is initialized to a value that results in ~0.25 of
> > > rcvbuf. In our case, SO_RCVBUF is set to 65536 by the application, which
> > > translates to 2 * 65536 = 131,072 bytes in rcvbuf and hence a ~28k
> > > initial receive window.
> >
> > What driver are you using, what MTU is set ?

The driver is AWS ENA driver. This is cross-region/internet traffic, so
the MTU is 1500.

> >
> > If you get a 0.25 ratio, that is because a driver is oversizing rx skbs.
> >
> > SO_RCVBUF 65536 would map indeed to 32768 bytes of payload.
> >

The 0.25 ratio is the initial default ratio calculated using 

#define TCP_DEFAULT_SCALING_RATIO ((1200 << TCP_RMEM_TO_WIN_SCALE) / \
                                   SKB_TRUESIZE(4096))

I think this is a constant 0.25, no?

Later with skb->len/skb->truesize, we get 0.66. However, the window
can't grow to this ratio because window_clamp stays at the initial
value, which is the initial tcp_full_space(sk), which is roughly 0.25 *
rcvbuf.

> > >
> > > Later, even though the scaling_ratio is updated to a more accurate
> > > skb->len/skb->truesize, which is ~0.66 in our environment, the window
> > > stays at ~0.25 * rcvbuf. This is because tp->window_clamp does not
> > > change together with the tp->scaling_ratio update. As a result, the
> > > window size is capped at the initial window_clamp, which is also ~0.25 *
> > > rcvbuf, and never grows bigger.
> 
> Sorry I missed this part. I understand better.
> 
> I wonder if we should at least test (sk->sk_userlocks &
> SOCK_RCVBUF_LOCK) or something...

In our case, the application does set SOCK_RCVBUF_LOCK. But meanwhile,
we also want the window_clamp to grow according to the ratio, so that
the window can grow beyond the original 0.25 * rcvbuf.

> 
> For autotuned flows (majority of the cases), tp->window_clamp is
> changed from tcp_rcv_space_adjust()
> 
> I think we need to audit a bit more all tp->window_clamp changes.
> 
> > >
> > > This patch updates window_clamp along with scaling_ratio. It changes the
> > > calculation of the initial rcv_wscale as well to make sure the scale
> > > factor is also not capped by the initial window_clamp.
> >
> > This is very suspicious.
> >
> > >
> > > A comment from Tycho Andersen <tycho@...ho.pizza> is "What happens if
> > > someone has done setsockopt(sk, TCP_WINDOW_CLAMP) explicitly; will this
> > > and the above not violate userspace's desire to clamp the window size?".
> > > This comment is not addressed in this patch because the existing code
> > > also updates window_clamp at several places without checking if
> > > TCP_WINDOW_CLAMP is set by user space. Adding this check now may break
> > > certain user space assumption (similar to how the original patch broke
> > > the assumption of buffer overhead being 50%). For example, if a user
> > > space program sets TCP_WINDOW_CLAMP but the applicaiton behavior relies
> > > on window_clamp adjusted by the kernel as of today.
> >
> > Quite frankly I would prefer we increase tcp_rmem[] sysctls, instead
> > of trying to accomodate
> > with too small SO_RCVBUF values.
> >
> > This would benefit old applications that were written 20 years ago.

The application is kafka and it has a default config of 64KB SO_RCVBUF
(https://docs.confluent.io/platform/current/installation/configuration/consumer-configs.html#receive-buffer-bytes)
so in this case it's limitted by SO_RCVBUF and not tcp_rmem. It also has
a default request timeout 30 seconds
(https://docs.confluent.io/platform/current/installation/configuration/consumer-configs.html#request-timeout-ms)
The combination of these two configs requires the certain amount of app
data (in our case 10M) to be transfer within 30 seconds. But a 32k
window size can't achieve this, causing app timeout. Our goal was to
upgrade the kernel without having to update applications if possible.

Powered by blists - more mailing lists