| Message ID | 20210819195443.1191973-1-ntspring@fb.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v2] tcp: enable mid stream window clamp | expand |
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.
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
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
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
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; }
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(+)