Re: [PATCH] tcp: add new tcp_mtu_probe_floor sysctl

[Josh Hunt]

On 7/27/19 12:05 AM, Eric Dumazet wrote:
On Sat, Jul 27, 2019 at 4:23 AM Josh Hunt <joh...@akamai.com> wrote:

The current implementation of TCP MTU probing can considerably
underestimate the MTU on lossy connections allowing the MSS to get down to
48. We have found that in almost all of these cases on our networks these
paths can handle much larger MTUs meaning the connections are being
artificially limited. Even though TCP MTU probing can raise the MSS back up
we have seen this not to be the case causing connections to be "stuck" with
an MSS of 48 when heavy loss is present.

Prior to pushing out this change we could not keep TCP MTU probing enabled
b/c of the above reasons. Now with a reasonble floor set we've had it
enabled for the past 6 months.

And what reasonable value have you used ???

Reasonable for some may not be reasonable for others hence the new 
sysctl :) We're currently running with a fairly high value based off of 
the v6 min MTU minus headers and options, etc. We went conservative with 
our setting initially as it seemed a reasonable first step when 
re-enabling TCP MTU probing since with no configurable floor we saw a # 
of cases where connections were using severely reduced mss b/c of loss 
and not b/c of actual path restriction. I plan to reevaluate the setting 
at some point, but since the probing method is still the same it means 
the same clients who got stuck with mss of 48 before will land at 
whatever floor we set. Looking forward we are interested in trying to 
improve TCP MTU probing so it does not penalize clients like this.

A suggestion for a more reasonable floor default would be 512, which is 
the same as the min_pmtu. Given both mechanisms are trying to achieve 
the same goal it seems like they should have a similar min/floor.



The new sysctl will still default to TCP_MIN_SND_MSS (48), but gives
administrators the ability to control the floor of MSS probing.

Signed-off-by: Josh Hunt <joh...@akamai.com>
---
  Documentation/networking/ip-sysctl.txt | 6 ++++++
  include/net/netns/ipv4.h               | 1 +
  net/ipv4/sysctl_net_ipv4.c             | 9 +++++++++
  net/ipv4/tcp_ipv4.c                    | 1 +
  net/ipv4/tcp_timer.c                   | 2 +-
  5 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.txt 
b/Documentation/networking/ip-sysctl.txt
index df33674799b5..49e95f438ed7 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -256,6 +256,12 @@ tcp_base_mss - INTEGER
         Path MTU discovery (MTU probing).  If MTU probing is enabled,
         this is the initial MSS used by the connection.

+tcp_mtu_probe_floor - INTEGER
+       If MTU probing is enabled this caps the minimum MSS used for search_low
+       for the connection.
+
+       Default : 48
+
  tcp_min_snd_mss - INTEGER
         TCP SYN and SYNACK messages usually advertise an ADVMSS option,
         as described in RFC 1122 and RFC 6691.
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index bc24a8ec1ce5..c0c0791b1912 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -116,6 +116,7 @@ struct netns_ipv4 {
         int sysctl_tcp_l3mdev_accept;
  #endif
         int sysctl_tcp_mtu_probing;
+       int sysctl_tcp_mtu_probe_floor;
         int sysctl_tcp_base_mss;
         int sysctl_tcp_min_snd_mss;
         int sysctl_tcp_probe_threshold;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 0b980e841927..59ded25acd04 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -820,6 +820,15 @@ static struct ctl_table ipv4_net_table[] = {
                 .extra2         = &tcp_min_snd_mss_max,
         },
         {
+               .procname       = "tcp_mtu_probe_floor",
+               .data           = &init_net.ipv4.sysctl_tcp_mtu_probe_floor,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .proc_handler   = proc_dointvec_minmax,
+               .extra1         = &tcp_min_snd_mss_min,
+               .extra2         = &tcp_min_snd_mss_max,
+       },
+       {
                 .procname       = "tcp_probe_threshold",
                 .data           = &init_net.ipv4.sysctl_tcp_probe_threshold,
                 .maxlen         = sizeof(int),
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index d57641cb3477..e0a372676329 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2637,6 +2637,7 @@ static int __net_init tcp_sk_init(struct net *net)
         net->ipv4.sysctl_tcp_min_snd_mss = TCP_MIN_SND_MSS;
         net->ipv4.sysctl_tcp_probe_threshold = TCP_PROBE_THRESHOLD;
         net->ipv4.sysctl_tcp_probe_interval = TCP_PROBE_INTERVAL;
+       net->ipv4.sysctl_tcp_mtu_probe_floor = TCP_MIN_SND_MSS;

         net->ipv4.sysctl_tcp_keepalive_time = TCP_KEEPALIVE_TIME;
         net->ipv4.sysctl_tcp_keepalive_probes = TCP_KEEPALIVE_PROBES;
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index c801cd37cc2a..dbd9d2d0ee63 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -154,7 +154,7 @@ static void tcp_mtu_probing(struct inet_connection_sock 
*icsk, struct sock *sk)
         } else {
                 mss = tcp_mtu_to_mss(sk, icsk->icsk_mtup.search_low) >> 1;
                 mss = min(net->ipv4.sysctl_tcp_base_mss, mss);
-               mss = max(mss, 68 - tcp_sk(sk)->tcp_header_len);
+               mss = max(mss, net->ipv4.sysctl_tcp_mtu_probe_floor);
                 mss = max(mss, net->ipv4.sysctl_tcp_min_snd_mss);
                 icsk->icsk_mtup.search_low = tcp_mss_to_mtu(sk, mss);
         }


Existing sysctl should be enough ?

I don't think so. Changing tcp_min_snd_mss could impact clients that 
really want/need a small mss. When you added the new sysctl I tried to 
analyze the mss values we're seeing to understand what we could possibly 
raise it to. While not a huge amount, we see more clients than I 
expected announcing mss values in the 180-512 range. Given that I would 
not feel comfortable setting tcp_min_snd_mss to say 512 as I suggested 
above.


tcp_min_snd_mss  documentation could be slightly updated.

And maybe its default value could be raised a bit.


Thanks
Josh