Message-ID: <CANn89iLyb70E+0NcYUQ7qBJ1N3UH64D4Q8EoigXw287NNQv2sg@mail.gmail.com>
Date: Wed, 3 Apr 2024 16:49:03 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Hechao Li <hli@...flix.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 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 ?
>
> 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.
>
> >
> > 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...

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.

Powered by blists - more mailing lists