Date:   Sun, 28 Jul 2019 14:32:04 -0700
From:   Josh Hunt <johunt@...mai.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] tcp: add new tcp_mtu_probe_floor sysctl

On 7/28/19 2:14 PM, Josh Hunt wrote:
> On 7/28/19 6:54 AM, Eric Dumazet wrote:
>> On Sun, Jul 28, 2019 at 1:21 AM Josh Hunt <johunt@...mai.com> wrote:
>>>
>>> On 7/27/19 12:05 AM, Eric Dumazet wrote:
>>>> On Sat, Jul 27, 2019 at 4:23 AM Josh Hunt <johunt@...mai.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 <johunt@...mai.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.
>>
>> If these clients need mss values in 180-512 ranges, how MTU probing
>> would work for them,
>> if you set a floor to 512 ?
> 
> First, we already seem to be fine with ignoring these paths with ICMP 
> based PMTU discovery b/c of our min_pmtu default of 512 and that is 
> configurable. Second by adding this sysctl we're giving administrators 
> the choice to decide if they'd like to attempt to support these very 
> very small # of paths which may be below 512 (MSS <= 512 does not mean 
> MTU <= 512) or cover themselves by being able to raise the floor to not 
> penalize clients who may be on very lossy networks.
> 
>>
>> Are we sure the intent of tcp_base_mss was not to act as a floor ?
> 
> My understanding is that tcp_base_mss is meant to be the initial value 
> of search_low (as per Docs). Then in RFC 4821 [1] Sections 7.2, shows 
> search_low should be configurable, and 7.7 we see that in response to 
> successive black hole detection search_low should be halved. So I don't 
> think it was meant to be a floor, but just the initial search_low param. 
> Also note that in that same section they suggest a floor of 68 for v4, 
> but a floor of 1280 for v6 which we do not adhere to currently.
> 

Clarification. We == Akamai in regards to setting tcp_base_mss to 
1400-overheads. Upstream default is 1024.

> We actually set tcp_base_mss to something close to the value suggested 
> towards the end of section 7.2 of the RFC of 1400 bytes minus IP and 
> Transport overheads and options. This way we have more realistic 
> searching based on the majority of clients that we see. The kernel winds 
> up using initial search_low/tcp_base_mss as initial eff_pmtu, so we see 
> something like:
> 
> 21:03:41.314612 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq 
> 1:1461, ack 1, win 229, length 1460: HTTP
> 21:03:41.670307 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq 
> 1:1461, ack 1, win 229, length 1460: HTTP
> 21:03:42.030308 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq 
> 1:1461, ack 1, win 229, length 1460: HTTP
> 21:03:42.534307 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq 
> 1:1461, ack 1, win 229, length 1460: HTTP
> 21:03:43.198308 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq 
> 1:1461, ack 1, win 229, length 1460: HTTP
> 21:03:44.478307 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [P.], seq 
> 1:1461, ack 1, win 229, length 1460: HTTP
> 21:03:47.742310 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [.], seq 
> 1:1349, ack 1, win 229, length 1348: HTTP
> 21:03:56.702310 IP 192.168.0.1.8080 > 192.0.2.1.41523: Flags [.], seq 
> 1:675, ack 1, win 229, length 674: HTTP
> 
> For further evidence this is a real problem here's a sample of mss 
> values I found when originally investigating this problem for us:
> 
> I dug up some #s I found when originally investigating this problem:
> 
> # ss -emoitn | grep mss | sed "s/.*mss:\([0-9]*\).*/\1/" | sort -u | 
> sort -g | head -5
> 
> 36:11
> 64:7
> 72:1
> 128:13
> 144:4
> 
>  From what I could tell these connections were on paths much larger than 
> the mss they were being forced to use. I determined this by looking at 
> the mss used for other objects fetched from the same IPs.
> 
> Josh
> 
> [1] - https://www.ietf.org/rfc/rfc4821.txt
> 
>>
>> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
>> index 
>> c801cd37cc2a9c11f2dd4b9681137755e501a538..6d15895e9dcfb2eff51bbcf3608c7e68c1970a9e 
>>
>> 100644
>> --- a/net/ipv4/tcp_timer.c
>> +++ b/net/ipv4/tcp_timer.c
>> @@ -153,7 +153,7 @@ static void tcp_mtu_probing(struct
>> inet_connection_sock *icsk, struct sock *sk)
>>                  icsk->icsk_mtup.probe_timestamp = tcp_jiffies32;
>>          } else {
>>                  mss = tcp_mtu_to_mss(sk, icsk->icsk_mtup.search_low) 
>> >> 1;
>> -               mss = min(net->ipv4.sysctl_tcp_base_mss, mss);
>> +               mss = max(net->ipv4.sysctl_tcp_base_mss, mss);
>>                  mss = max(mss, 68 - tcp_sk(sk)->tcp_header_len);
>>                  mss = max(mss, net->ipv4.sysctl_tcp_min_snd_mss);
>>                  icsk->icsk_mtup.search_low = tcp_mss_to_mtu(sk, mss);
>>
>>
>>
>>>
>>>>
>>>> tcp_min_snd_mss  documentation could be slightly updated.
>>>>
>>>> And maybe its default value could be raised a bit.
>>>>
>>>
>>> Thanks
>>> Josh

Powered by blists - more mailing lists