[PATCH net-next v2] tcp: enable mid stream window clamp

[PATCH net-next v2] tcp: enable mid stream window clamp

[Neil Spring]

The TCP_WINDOW_CLAMP socket option is defined in tcp(7) to "Bound the size of
the advertised window to this value."  Window clamping is distributed across two
variables, window_clamp ("Maximal window to advertise" in tcp.h) and rcv_ssthresh
("Current window clamp").

This patch updates the function where the window clamp is set to also reduce the current
window clamp, rcv_sshthresh, if needed.  With this, setting the TCP_WINDOW_CLAMP option
has the documented effect of limiting the window.

Signed-off-by: Neil Spring <ntspring@fb.com>
---
v2: - fix email formatting


 net/ipv4/tcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f931def6302e..2dc6212d5888 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3338,6 +3338,8 @@ int tcp_set_window_clamp(struct sock *sk, int val)
 	} else {
 		tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
 			SOCK_MIN_RCVBUF / 2 : val;
+		tp->rcv_ssthresh = min(tp->rcv_ssthresh,
+				       tp->window_clamp);
 	}
 	return 0;
 }
-- 
2.30.2

Re: [PATCH net-next v2] tcp: enable mid stream window clamp

[Eric Dumazet]

On 8/19/21 12:54 PM, Neil Spring wrote:
> The TCP_WINDOW_CLAMP socket option is defined in tcp(7) to "Bound the size of
> the advertised window to this value."  Window clamping is distributed across two
> variables, window_clamp ("Maximal window to advertise" in tcp.h) and rcv_ssthresh
> ("Current window clamp").
> 
> This patch updates the function where the window clamp is set to also reduce the current
> window clamp, rcv_sshthresh, if needed.  With this, setting the TCP_WINDOW_CLAMP option
> has the documented effect of limiting the window.
> 
> Signed-off-by: Neil Spring <ntspring@fb.com>
> ---
> v2: - fix email formatting
> 
> 
>  net/ipv4/tcp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index f931def6302e..2dc6212d5888 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3338,6 +3338,8 @@ int tcp_set_window_clamp(struct sock *sk, int val)
>  	} else {
>  		tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
>  			SOCK_MIN_RCVBUF / 2 : val;
> +		tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> +				       tp->window_clamp);

This fits in a single line I think.
		tp->rcv_ssthresh = min(tp->rcv_ssthresh, tp->window_clamp);

>  	}
>  	return 0;
>  }
> 

Hi Neil

Can you provide a packetdrill test showing the what the new expected behavior is ?

It is not really clear why you need this.

Also if we are unable to increase tp->rcv_ssthresh, this means the following sequence
will not work as we would expect :

+0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [10000], 4) = 0
+0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [100000], 4) = 0


Thanks.

Re: [PATCH net-next v2] tcp: enable mid stream window clamp

[Neil Spring]

Eric Dumazet wrote:
> On 8/19/21 12:54 PM, Neil Spring wrote:
> > The TCP_WINDOW_CLAMP socket option is defined in tcp(7) to "Bound the size of
> > the advertised window to this value."  Window clamping is distributed across two
> > variables, window_clamp ("Maximal window to advertise" in tcp.h) and rcv_ssthresh
> > ("Current window clamp").
> >
> > This patch updates the function where the window clamp is set to also reduce the current
> > window clamp, rcv_sshthresh, if needed.  With this, setting the TCP_WINDOW_CLAMP option
> > has the documented effect of limiting the window.
> >
> > Signed-off-by: Neil Spring <ntspring@fb.com>
> > ---
> > v2: - fix email formatting
> >
> >
> >  net/ipv4/tcp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index f931def6302e..2dc6212d5888 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -3338,6 +3338,8 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> >        } else {
> >                tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> >                        SOCK_MIN_RCVBUF / 2 : val;
> > +             tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> > +                                    tp->window_clamp);

> This fits in a single line I think.
>                 tp->rcv_ssthresh = min(tp->rcv_ssthresh, tp->window_clamp);

I'll fix in v3 in a moment, thanks!

> >        }
> >        return 0;
> >  }
> >

> Hi Neil
> 
> Can you provide a packetdrill test showing the what the new expected behavior is ?

Sure.  I submitted a pull request on packetdrill -
https://github.com/google/packetdrill/pull/56 - to document the intended behavior.

> It is not really clear why you need this.

The observation is that this option is currently ineffective at limiting once the
connection is exchanging data. I interpret this as a result of only looking at 
the window clamp when growing rcv_ssthresh, not as a key to reduce this 
limit.  

The packetdrill example will fail at the point where an ack should have a reduced
window due to the clamp.

> Also if we are unable to increase tp->rcv_ssthresh, this means the following sequence
> will not work as we would expect :

> +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [10000], 4) = 0
> +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [100000], 4) = 0

The packetdrill shows that raising the window clamp works:
tcp_grow_window takes over and raises the window quickly, but I'll add
a specific test for this sequence (with no intervening data) to confirm.

> Thanks.

Thanks Eric!

-neil

Re: [PATCH net-next v2] tcp: enable mid stream window clamp

[Eric Dumazet]

On Wed, Aug 25, 2021 at 9:57 AM Neil Spring <ntspring@fb.com> wrote:
>
>
> Eric Dumazet wrote:
> > On 8/19/21 12:54 PM, Neil Spring wrote:
> > > The TCP_WINDOW_CLAMP socket option is defined in tcp(7) to "Bound the size of
> > > the advertised window to this value."  Window clamping is distributed across two
> > > variables, window_clamp ("Maximal window to advertise" in tcp.h) and rcv_ssthresh
> > > ("Current window clamp").
> > >
> > > This patch updates the function where the window clamp is set to also reduce the current
> > > window clamp, rcv_sshthresh, if needed.  With this, setting the TCP_WINDOW_CLAMP option
> > > has the documented effect of limiting the window.
> > >
> > > Signed-off-by: Neil Spring <ntspring@fb.com>
> > > ---
> > > v2: - fix email formatting
> > >
> > >
> > >  net/ipv4/tcp.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > index f931def6302e..2dc6212d5888 100644
> > > --- a/net/ipv4/tcp.c
> > > +++ b/net/ipv4/tcp.c
> > > @@ -3338,6 +3338,8 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> > >        } else {
> > >                tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> > >                        SOCK_MIN_RCVBUF / 2 : val;
> > > +             tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> > > +                                    tp->window_clamp);
>
> > This fits in a single line I think.
> >                 tp->rcv_ssthresh = min(tp->rcv_ssthresh, tp->window_clamp);
>
> I'll fix in v3 in a moment, thanks!
>
> > >        }
> > >        return 0;
> > >  }
> > >
>
> > Hi Neil
> >
> > Can you provide a packetdrill test showing the what the new expected behavior is ?
>
> Sure.  I submitted a pull request on packetdrill -
> https://github.com/google/packetdrill/pull/56 - to document the intended behavior.
>
> > It is not really clear why you need this.
>
> The observation is that this option is currently ineffective at limiting once the
> connection is exchanging data. I interpret this as a result of only looking at
> the window clamp when growing rcv_ssthresh, not as a key to reduce this
> limit.
>
> The packetdrill example will fail at the point where an ack should have a reduced
> window due to the clamp.
>
> > Also if we are unable to increase tp->rcv_ssthresh, this means the following sequence
> > will not work as we would expect :
>
> > +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [10000], 4) = 0
> > +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [100000], 4) = 0
>
> The packetdrill shows that raising the window clamp works:
> tcp_grow_window takes over and raises the window quickly, but I'll add
> a specific test for this sequence (with no intervening data) to confirm.

Sure, raising the window clamping is working (even before your patch)

But after your patch, rcv_ssthresh will still be 10000, instead of
something maybe bigger ?

>
> > Thanks.
>
> Thanks Eric!
>
> -neil

Re: [PATCH net-next v2] tcp: enable mid stream window clamp

[Neil Spring]

Eric Dumazet wrote:
> On Wed, Aug 25, 2021 at 9:57 AM Neil Spring <ntspring@fb.com> wrote:
> >
> >
> > Eric Dumazet wrote:
> > > On 8/19/21 12:54 PM, Neil Spring wrote:
> > > > The TCP_WINDOW_CLAMP socket option is defined in tcp(7) to "Bound the size of
> > > > the advertised window to this value."  Window clamping is distributed across two
> > > > variables, window_clamp ("Maximal window to advertise" in tcp.h) and rcv_ssthresh
> > > > ("Current window clamp").
> > > >
> > > > This patch updates the function where the window clamp is set to also reduce the current
> > > > window clamp, rcv_sshthresh, if needed.  With this, setting the TCP_WINDOW_CLAMP option
> > > > has the documented effect of limiting the window.
> > > >
> > > > Signed-off-by: Neil Spring <ntspring@fb.com>
> > > > ---
> > > > v2: - fix email formatting
> > > >
> > > >
> > > >  net/ipv4/tcp.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > > > index f931def6302e..2dc6212d5888 100644
> > > > --- a/net/ipv4/tcp.c
> > > > +++ b/net/ipv4/tcp.c
> > > > @@ -3338,6 +3338,8 @@ int tcp_set_window_clamp(struct sock *sk, int val)
> > > >        } else {
> > > >                tp->window_clamp = val < SOCK_MIN_RCVBUF / 2 ?
> > > >                        SOCK_MIN_RCVBUF / 2 : val;
> > > > +             tp->rcv_ssthresh = min(tp->rcv_ssthresh,
> > > > +                                    tp->window_clamp);
> >
> > > This fits in a single line I think.
> > >                 tp->rcv_ssthresh = min(tp->rcv_ssthresh, tp->window_clamp);
> >
> > I'll fix in v3 in a moment, thanks!
> >
> > > >        }
> > > >        return 0;
> > > >  }
> > > >
> >
> > > Hi Neil
> > >
> > > Can you provide a packetdrill test showing the what the new expected behavior is ?
> >
> > Sure.  I submitted a pull request on packetdrill -
> > https://github.com/google/packetdrill/pull/56 - to document the intended behavior.
> >
> > > It is not really clear why you need this.
> >
> > The observation is that this option is currently ineffective at limiting once the
> > connection is exchanging data. I interpret this as a result of only looking at
> > the window clamp when growing rcv_ssthresh, not as a key to reduce this
> > limit.
> >
> > The packetdrill example will fail at the point where an ack should have a reduced
> > window due to the clamp.
> >
> > > Also if we are unable to increase tp->rcv_ssthresh, this means the following sequence
> > > will not work as we would expect :
> >
> > > +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [10000], 4) = 0
> > > +0 setsockopt(5, IPPROTO_TCP, TCP_WINDOW_CLAMP, [100000], 4) = 0
> >
> > The packetdrill shows that raising the window clamp works:
> > tcp_grow_window takes over and raises the window quickly, but I'll add
> > a specific test for this sequence (with no intervening data) to confirm.
> 
> Sure, raising the window clamping is working (even before your patch)
> 
> But after your patch, rcv_ssthresh will still be 10000, instead of
> something maybe bigger ?

Ahh, I think I see, thanks for your patience in repeating the idea.
I'll alter the patch to set rcv_ssthresh to:

  rcv_ssthresh = min(rcv_wnd, window_clamp)

Walking through your example, assuming the host just advertised 50000 bytes,
it will raise rcv_ssthresh immediately to 50000 when raising the window_clamp field
from 10000 to 100000.  

I think this is consistent with what rcv_ssthresh is meant to do, but
I could see a case for eagerly increasing to anywhere between rcv_wnd
+ rcv_mss to rcv_wnd * 2.  Happy to update again if that seems useful.

I've updated the packetdrill pull request with the new behavior.

> >
> > > Thanks.
> >
> > Thanks Eric!
> >
> > -neil