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

Eric Dumazet wrote:

On Wed, Aug 25, 2021 at 9:57 AM Neil Spring [off-list ref] wrote:

quoted

Eric Dumazet wrote:

quoted

On 8/19/21 12:54 PM, Neil Spring wrote:

quoted

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 <redacted>
---
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);

quoted

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!

quoted

quoted

       }
       return 0;
 }

quoted

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.

quoted

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.

quoted

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

quoted

+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.

quoted

quoted

Thanks.

Thanks Eric!

-neil