[PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Subash Abhinov Kasiviswanathan]

tp->scaling_ratio is not updated based on skb->len/skb->truesize once
SO_RCVBUF is set leading to the maximum window scaling to be 25% of
rcvbuf after
commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
and 50% of rcvbuf after
commit 697a6c8cec03 ("tcp: increase the default TCP scaling ratio").
50% tries to emulate the behavior of older kernels using
sysctl_tcp_adv_win_scale with default value.

Systems which were using a different values of sysctl_tcp_adv_win_scale
in older kernels ended up seeing reduced download speeds in certain
cases as covered in https://lists.openwall.net/netdev/2024/05/15/13
While the sysctl scheme is no longer acceptable, the value of 50% is
a bit conservative when the skb->len/skb->truesize ratio is later
determined to be ~0.66.

Applications not specifying SO_RCVBUF update the window scaling and
the receiver buffer every time data is copied to userspace. This
computation is now used for applications setting SO_RCVBUF to update
the maximum window scaling while ensuring that the receive buffer
is within the application specified limit.

Fixes: dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
---
v1 -> v2
  Update the condition for SO_RCVBUF window_clamp updates to always
  monitor the current rcvbuf value as suggested by Eric.

 net/ipv4/tcp_input.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 454362e359da..e2b9583ed96a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -754,8 +754,7 @@ void tcp_rcv_space_adjust(struct sock *sk)
 	 * <prev RTT . ><current RTT .. ><next RTT .... >
 	 */
 
-	if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf) &&
-	    !(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
+	if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf)) {
 		u64 rcvwin, grow;
 		int rcvbuf;
 
@@ -771,12 +770,22 @@ void tcp_rcv_space_adjust(struct sock *sk)
 
 		rcvbuf = min_t(u64, tcp_space_from_win(sk, rcvwin),
 			       READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
-		if (rcvbuf > sk->sk_rcvbuf) {
-			WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
+		if (!(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
+			if (rcvbuf > sk->sk_rcvbuf) {
+				WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
 
-			/* Make the window clamp follow along.  */
-			WRITE_ONCE(tp->window_clamp,
-				   tcp_win_from_space(sk, rcvbuf));
+				/* Make the window clamp follow along.  */
+				WRITE_ONCE(tp->window_clamp,
+					   tcp_win_from_space(sk, rcvbuf));
+			}
+		} else {
+			/* Make the window clamp follow along while being bounded
+			 * by SO_RCVBUF.
+			 */
+			int clamp = tcp_win_from_space(sk, min(rcvbuf, sk->sk_rcvbuf));
+
+			if (clamp > tp->window_clamp)
+				WRITE_ONCE(tp->window_clamp, clamp);
 		}
 	}
 	tp->rcvq_space.space = copied;
-- 
2.34.1

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Eric Dumazet]

On Fri, Jul 26, 2024 at 10:41 PM Subash Abhinov Kasiviswanathan
<quic_subashab@quicinc.com> wrote:
>
> tp->scaling_ratio is not updated based on skb->len/skb->truesize once
> SO_RCVBUF is set leading to the maximum window scaling to be 25% of
> rcvbuf after
> commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> and 50% of rcvbuf after
> commit 697a6c8cec03 ("tcp: increase the default TCP scaling ratio").
> 50% tries to emulate the behavior of older kernels using
> sysctl_tcp_adv_win_scale with default value.
>
> Systems which were using a different values of sysctl_tcp_adv_win_scale
> in older kernels ended up seeing reduced download speeds in certain
> cases as covered in https://lists.openwall.net/netdev/2024/05/15/13
> While the sysctl scheme is no longer acceptable, the value of 50% is
> a bit conservative when the skb->len/skb->truesize ratio is later
> determined to be ~0.66.
>
> Applications not specifying SO_RCVBUF update the window scaling and
> the receiver buffer every time data is copied to userspace. This
> computation is now used for applications setting SO_RCVBUF to update
> the maximum window scaling while ensuring that the receive buffer
> is within the application specified limit.
>
> Fixes: dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks.

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 26 Jul 2024 13:41:05 -0700 you wrote:
> tp->scaling_ratio is not updated based on skb->len/skb->truesize once
> SO_RCVBUF is set leading to the maximum window scaling to be 25% of
> rcvbuf after
> commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> and 50% of rcvbuf after
> commit 697a6c8cec03 ("tcp: increase the default TCP scaling ratio").
> 50% tries to emulate the behavior of older kernels using
> sysctl_tcp_adv_win_scale with default value.
> 
> [...]

Here is the summary with links:
  - [net,v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF
    https://git.kernel.org/netdev/net/c/05f76b2d634e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Neal Cardwell]

On Fri, Jul 26, 2024 at 4:41 PM Subash Abhinov Kasiviswanathan
<quic_subashab@quicinc.com> wrote:
>
> tp->scaling_ratio is not updated based on skb->len/skb->truesize once
> SO_RCVBUF is set leading to the maximum window scaling to be 25% of
> rcvbuf after
> commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> and 50% of rcvbuf after
> commit 697a6c8cec03 ("tcp: increase the default TCP scaling ratio").
> 50% tries to emulate the behavior of older kernels using
> sysctl_tcp_adv_win_scale with default value.
>
> Systems which were using a different values of sysctl_tcp_adv_win_scale
> in older kernels ended up seeing reduced download speeds in certain
> cases as covered in https://lists.openwall.net/netdev/2024/05/15/13
> While the sysctl scheme is no longer acceptable, the value of 50% is
> a bit conservative when the skb->len/skb->truesize ratio is later
> determined to be ~0.66.
>
> Applications not specifying SO_RCVBUF update the window scaling and
> the receiver buffer every time data is copied to userspace. This
> computation is now used for applications setting SO_RCVBUF to update
> the maximum window scaling while ensuring that the receive buffer
> is within the application specified limit.
>
> Fixes: dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
> ---
> v1 -> v2
>   Update the condition for SO_RCVBUF window_clamp updates to always
>   monitor the current rcvbuf value as suggested by Eric.
>
>  net/ipv4/tcp_input.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 454362e359da..e2b9583ed96a 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -754,8 +754,7 @@ void tcp_rcv_space_adjust(struct sock *sk)
>          * <prev RTT . ><current RTT .. ><next RTT .... >
>          */
>
> -       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf) &&
> -           !(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
> +       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf)) {
>                 u64 rcvwin, grow;
>                 int rcvbuf;
>
> @@ -771,12 +770,22 @@ void tcp_rcv_space_adjust(struct sock *sk)
>
>                 rcvbuf = min_t(u64, tcp_space_from_win(sk, rcvwin),
>                                READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
> -               if (rcvbuf > sk->sk_rcvbuf) {
> -                       WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
> +               if (!(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
> +                       if (rcvbuf > sk->sk_rcvbuf) {
> +                               WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
>
> -                       /* Make the window clamp follow along.  */
> -                       WRITE_ONCE(tp->window_clamp,
> -                                  tcp_win_from_space(sk, rcvbuf));
> +                               /* Make the window clamp follow along.  */
> +                               WRITE_ONCE(tp->window_clamp,
> +                                          tcp_win_from_space(sk, rcvbuf));
> +                       }
> +               } else {
> +                       /* Make the window clamp follow along while being bounded
> +                        * by SO_RCVBUF.
> +                        */
> +                       int clamp = tcp_win_from_space(sk, min(rcvbuf, sk->sk_rcvbuf));
> +
> +                       if (clamp > tp->window_clamp)
> +                               WRITE_ONCE(tp->window_clamp, clamp);
>                 }
>         }
>         tp->rcvq_space.space = copied;
> --

Is this the correct place to put this new code to update
tp->window_clamp? AFAICT it's not the correct place.

If a system administrator has disabled receive buffer autotuning by
setting `sysctl net.ipv4.tcp_moderate_rcvbuf=0`, or if (copied <=
tp->rcvq_space.space), then TCP connections will not reach this new
code, and the window_clamp will not be adjusted, and the receive
window will still be too low.

Even if a system administrator has disabled receive buffer autotuning
by setting `sysctl net.ipv4.tcp_moderate_rcvbuf=0`, or even if (copied
<= tp->rcvq_space.space), AFAICT we still want the correct receive
window value for whatever sk->sk_rcvbuf we have, based on the correct
tp->scaling_ratio.

So AFAICT the correct place to put this kind of logic is in
tcp_measure_rcv_mss(). If we compute a new scaling_ratio and it's
different than tp->scaling_ratio, then it seems we should compute a
new window_clamp value using sk->sk_rcvbuf, and if the new
window_clamp value is different then we should WRITE_ONCE that value
into tp->window_clamp.

That way we can have the correct tp->window_clamp, no matter the value
of net.ipv4.tcp_moderate_rcvbuf, and even if (copied <=
tp->rcvq_space.space).

How does that sound?

neal

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Eric Dumazet]

On Mon, Jul 29, 2024 at 4:51 PM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Fri, Jul 26, 2024 at 4:41 PM Subash Abhinov Kasiviswanathan
> <quic_subashab@quicinc.com> wrote:
> >
> > tp->scaling_ratio is not updated based on skb->len/skb->truesize once
> > SO_RCVBUF is set leading to the maximum window scaling to be 25% of
> > rcvbuf after
> > commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> > and 50% of rcvbuf after
> > commit 697a6c8cec03 ("tcp: increase the default TCP scaling ratio").
> > 50% tries to emulate the behavior of older kernels using
> > sysctl_tcp_adv_win_scale with default value.
> >
> > Systems which were using a different values of sysctl_tcp_adv_win_scale
> > in older kernels ended up seeing reduced download speeds in certain
> > cases as covered in https://lists.openwall.net/netdev/2024/05/15/13
> > While the sysctl scheme is no longer acceptable, the value of 50% is
> > a bit conservative when the skb->len/skb->truesize ratio is later
> > determined to be ~0.66.
> >
> > Applications not specifying SO_RCVBUF update the window scaling and
> > the receiver buffer every time data is copied to userspace. This
> > computation is now used for applications setting SO_RCVBUF to update
> > the maximum window scaling while ensuring that the receive buffer
> > is within the application specified limit.
> >
> > Fixes: dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> > Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
> > Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
> > ---
> > v1 -> v2
> >   Update the condition for SO_RCVBUF window_clamp updates to always
> >   monitor the current rcvbuf value as suggested by Eric.
> >
> >  net/ipv4/tcp_input.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 454362e359da..e2b9583ed96a 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -754,8 +754,7 @@ void tcp_rcv_space_adjust(struct sock *sk)
> >          * <prev RTT . ><current RTT .. ><next RTT .... >
> >          */
> >
> > -       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf) &&
> > -           !(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
> > +       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf)) {
> >                 u64 rcvwin, grow;
> >                 int rcvbuf;
> >
> > @@ -771,12 +770,22 @@ void tcp_rcv_space_adjust(struct sock *sk)
> >
> >                 rcvbuf = min_t(u64, tcp_space_from_win(sk, rcvwin),
> >                                READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
> > -               if (rcvbuf > sk->sk_rcvbuf) {
> > -                       WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
> > +               if (!(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
> > +                       if (rcvbuf > sk->sk_rcvbuf) {
> > +                               WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
> >
> > -                       /* Make the window clamp follow along.  */
> > -                       WRITE_ONCE(tp->window_clamp,
> > -                                  tcp_win_from_space(sk, rcvbuf));
> > +                               /* Make the window clamp follow along.  */
> > +                               WRITE_ONCE(tp->window_clamp,
> > +                                          tcp_win_from_space(sk, rcvbuf));
> > +                       }
> > +               } else {
> > +                       /* Make the window clamp follow along while being bounded
> > +                        * by SO_RCVBUF.
> > +                        */
> > +                       int clamp = tcp_win_from_space(sk, min(rcvbuf, sk->sk_rcvbuf));
> > +
> > +                       if (clamp > tp->window_clamp)
> > +                               WRITE_ONCE(tp->window_clamp, clamp);
> >                 }
> >         }
> >         tp->rcvq_space.space = copied;
> > --
>
> Is this the correct place to put this new code to update
> tp->window_clamp? AFAICT it's not the correct place.
>
> If a system administrator has disabled receive buffer autotuning by
> setting `sysctl net.ipv4.tcp_moderate_rcvbuf=0`, or if (copied <=
> tp->rcvq_space.space), then TCP connections will not reach this new
> code, and the window_clamp will not be adjusted, and the receive
> window will still be too low.
>
> Even if a system administrator has disabled receive buffer autotuning
> by setting `sysctl net.ipv4.tcp_moderate_rcvbuf=0`, or even if (copied
> <= tp->rcvq_space.space), AFAICT we still want the correct receive
> window value for whatever sk->sk_rcvbuf we have, based on the correct
> tp->scaling_ratio.
>
> So AFAICT the correct place to put this kind of logic is in
> tcp_measure_rcv_mss(). If we compute a new scaling_ratio and it's
> different than tp->scaling_ratio, then it seems we should compute a
> new window_clamp value using sk->sk_rcvbuf, and if the new
> window_clamp value is different then we should WRITE_ONCE that value
> into tp->window_clamp.
>
> That way we can have the correct tp->window_clamp, no matter the value
> of net.ipv4.tcp_moderate_rcvbuf, and even if (copied <=
> tp->rcvq_space.space).
>
> How does that sound?

Can this be done without adding new code in the fast path ?

Otherwise, I feel that we send a wrong signal to 'administrators' :
"We will maintain code to make sure that wrong sysctls settings were
not so wrong."

Are you aware of anyone changing net.ipv4.tcp_moderate_rcvbuf for any
valid reason ?

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Neal Cardwell]

On Mon, Jul 29, 2024 at 11:19 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Jul 29, 2024 at 4:51 PM Neal Cardwell <ncardwell@google.com> wrote:
> >
> > On Fri, Jul 26, 2024 at 4:41 PM Subash Abhinov Kasiviswanathan
> > <quic_subashab@quicinc.com> wrote:
> > >
> > > tp->scaling_ratio is not updated based on skb->len/skb->truesize once
> > > SO_RCVBUF is set leading to the maximum window scaling to be 25% of
> > > rcvbuf after
> > > commit dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> > > and 50% of rcvbuf after
> > > commit 697a6c8cec03 ("tcp: increase the default TCP scaling ratio").
> > > 50% tries to emulate the behavior of older kernels using
> > > sysctl_tcp_adv_win_scale with default value.
> > >
> > > Systems which were using a different values of sysctl_tcp_adv_win_scale
> > > in older kernels ended up seeing reduced download speeds in certain
> > > cases as covered in https://lists.openwall.net/netdev/2024/05/15/13
> > > While the sysctl scheme is no longer acceptable, the value of 50% is
> > > a bit conservative when the skb->len/skb->truesize ratio is later
> > > determined to be ~0.66.
> > >
> > > Applications not specifying SO_RCVBUF update the window scaling and
> > > the receiver buffer every time data is copied to userspace. This
> > > computation is now used for applications setting SO_RCVBUF to update
> > > the maximum window scaling while ensuring that the receive buffer
> > > is within the application specified limit.
> > >
> > > Fixes: dfa2f0483360 ("tcp: get rid of sysctl_tcp_adv_win_scale")
> > > Signed-off-by: Sean Tranchetti <quic_stranche@quicinc.com>
> > > Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.com>
> > > ---
> > > v1 -> v2
> > >   Update the condition for SO_RCVBUF window_clamp updates to always
> > >   monitor the current rcvbuf value as suggested by Eric.
> > >
> > >  net/ipv4/tcp_input.c | 23 ++++++++++++++++-------
> > >  1 file changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > index 454362e359da..e2b9583ed96a 100644
> > > --- a/net/ipv4/tcp_input.c
> > > +++ b/net/ipv4/tcp_input.c
> > > @@ -754,8 +754,7 @@ void tcp_rcv_space_adjust(struct sock *sk)
> > >          * <prev RTT . ><current RTT .. ><next RTT .... >
> > >          */
> > >
> > > -       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf) &&
> > > -           !(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
> > > +       if (READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_moderate_rcvbuf)) {
> > >                 u64 rcvwin, grow;
> > >                 int rcvbuf;
> > >
> > > @@ -771,12 +770,22 @@ void tcp_rcv_space_adjust(struct sock *sk)
> > >
> > >                 rcvbuf = min_t(u64, tcp_space_from_win(sk, rcvwin),
> > >                                READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
> > > -               if (rcvbuf > sk->sk_rcvbuf) {
> > > -                       WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
> > > +               if (!(sk->sk_userlocks & SOCK_RCVBUF_LOCK)) {
> > > +                       if (rcvbuf > sk->sk_rcvbuf) {
> > > +                               WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
> > >
> > > -                       /* Make the window clamp follow along.  */
> > > -                       WRITE_ONCE(tp->window_clamp,
> > > -                                  tcp_win_from_space(sk, rcvbuf));
> > > +                               /* Make the window clamp follow along.  */
> > > +                               WRITE_ONCE(tp->window_clamp,
> > > +                                          tcp_win_from_space(sk, rcvbuf));
> > > +                       }
> > > +               } else {
> > > +                       /* Make the window clamp follow along while being bounded
> > > +                        * by SO_RCVBUF.
> > > +                        */
> > > +                       int clamp = tcp_win_from_space(sk, min(rcvbuf, sk->sk_rcvbuf));
> > > +
> > > +                       if (clamp > tp->window_clamp)
> > > +                               WRITE_ONCE(tp->window_clamp, clamp);
> > >                 }
> > >         }
> > >         tp->rcvq_space.space = copied;
> > > --
> >
> > Is this the correct place to put this new code to update
> > tp->window_clamp? AFAICT it's not the correct place.
> >
> > If a system administrator has disabled receive buffer autotuning by
> > setting `sysctl net.ipv4.tcp_moderate_rcvbuf=0`, or if (copied <=
> > tp->rcvq_space.space), then TCP connections will not reach this new
> > code, and the window_clamp will not be adjusted, and the receive
> > window will still be too low.
> >
> > Even if a system administrator has disabled receive buffer autotuning
> > by setting `sysctl net.ipv4.tcp_moderate_rcvbuf=0`, or even if (copied
> > <= tp->rcvq_space.space), AFAICT we still want the correct receive
> > window value for whatever sk->sk_rcvbuf we have, based on the correct
> > tp->scaling_ratio.
> >
> > So AFAICT the correct place to put this kind of logic is in
> > tcp_measure_rcv_mss(). If we compute a new scaling_ratio and it's
> > different than tp->scaling_ratio, then it seems we should compute a
> > new window_clamp value using sk->sk_rcvbuf, and if the new
> > window_clamp value is different then we should WRITE_ONCE that value
> > into tp->window_clamp.
> >
> > That way we can have the correct tp->window_clamp, no matter the value
> > of net.ipv4.tcp_moderate_rcvbuf, and even if (copied <=
> > tp->rcvq_space.space).
> >
> > How does that sound?
>
> Can this be done without adding new code in the fast path ?

I was imagining that the code would not really be in the fast path,
because it would move to the spot in tcp_measure_rcv_mss() where the
segment length "len" is greater than icsk->icsk_ack.rcv_mss. I imagine
that should be rare, and we are already doing a somewhat expensive
do_div() call there, so I was imagining that the additional cost would
be relatively rare and small?

> Otherwise, I feel that we send a wrong signal to 'administrators' :
> "We will maintain code to make sure that wrong sysctls settings were
> not so wrong."
>
> Are you aware of anyone changing net.ipv4.tcp_moderate_rcvbuf for any
> valid reason ?

No, I'm not aware of any valid reason to disable
net.ipv4.tcp_moderate_rcvbuf. :-)

Even if tcp_moderate_rcvbuf is enabled, AFAICT there is still the
issue that if (copied <= tp->rcvq_space.space) we will not get to this
code...

neal

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Eric Dumazet]

On Mon, Jul 29, 2024 at 5:33 PM Neal Cardwell <ncardwell@google.com> wrote:

> I was imagining that the code would not really be in the fast path,
> because it would move to the spot in tcp_measure_rcv_mss() where the
> segment length "len" is greater than icsk->icsk_ack.rcv_mss. I imagine
> that should be rare, and we are already doing a somewhat expensive
> do_div() call there, so I was imagining that the additional cost would
> be relatively rare and small?

Let's see your patch :)

Thanks !

Re: [PATCH net v2] tcp: Adjust clamping window for applications specifying SO_RCVBUF

[Jason Xing]

Hello Neal,

> > Otherwise, I feel that we send a wrong signal to 'administrators' :
> > "We will maintain code to make sure that wrong sysctls settings were
> > not so wrong."
> >
> > Are you aware of anyone changing net.ipv4.tcp_moderate_rcvbuf for any
> > valid reason ?
>
> No, I'm not aware of any valid reason to disable
> net.ipv4.tcp_moderate_rcvbuf. :-)

I was also curious about why this sysctl knob was useful a long time
ago? I don't see any good in it (for many years, we haven't touched
it, setting it to 1 as default). Since you maintainers don't think
it's a good one, could we mark it as deprecated and remove this
sysctl?

Thanks,
Jason