Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Ahern]

On 3/5/18 5:29 AM, Stefano Brivio wrote:
> On Sun, 4 Mar 2018 18:11:41 -0700
> David Ahern  wrote:
> 
>> On 3/4/18 4:12 PM, Stefano Brivio wrote:
>>> On Sat, 3 Mar 2018 12:22:36 +0100
>>> Stefano Brivio  wrote:
>>>   
> And please codify the above expectation as a test under
> tools/testing/selftests/net

 And this, along with v2.  
>>>
>>> On a second thought: I start thinking it doesn't make much sense,
>>> especially given the current context of self-tests, to explicitly test
>>> this, because it's a rather particular corner case.
>>>
>>> I think it would make more sense to introduce generic tests first.
>>> About, say, PMTU, or route exceptions, but not "tunnel causes route
>>> exception and administrative change doesn't affect PMTU".
>>>   
>>
>> I would argue corner cases in particular should be documented.
> 
> Sure, but self-tests are not meant for documentation. I think commit
> messages are.
> 
> And about corner cases, from Documentation/dev-tools/kselftest.rst:
> 
>   These are intended to be small tests to exercise individual code
>   paths in the kernel. Tests are intended to be run after building, 
> installing
>   and booting a kernel.
> 
> and:
> 
>   In general, the rules for selftests are
>   [...]
>* Don't take too long;
> 
> if you plan to request a self-test for every fix in the networking area,
> you need to substantially change the scope of these self-tests. This stuff
> would instead fit in a comprehensive networking test suite.

The Linux Networking stack is long over due for a comprehensive
functional test. There is very little about Layer 3 that can not be
tested with network namespaces, vrf, veth and a recent iproute2 package.

No one company is going to pay someone to write this test suite. It
takes commitment from contributors to submit tests as we go, and test
cases for bug fixes is one of the easiest and best ways to get this moving.

> 
>> From the commit message it seems like you took the time to create a test
>> setup using network namespaces. Throw those commands into a shell script
>> -- tools/testing/selftests/net/mtu.sh. It can evolve from there.
> 
> My script sets up namespaces, veth and vti6 interfaces, xfrm states and
> policies (could be replaced by vxlan, but that's what I have now). Then
> it pings, waits, prints exception routes, changes MTU, etc. In the
> commit message, I reported only the relevant parts that are enough to
> clearly show the issue.
> 
> This script is some ugly monster I don't want to have on my conscience,
> or wish for anybody to run as "small test to exercise individual code
> paths".

Understood. I have a lot of those for MPLS, for example. Each time I
write one it evolves into something cleaner and now I have a few worth
submitting to selftests (and that will happen in it - e.g, when I come
back to MPLS).

> 
> I don't think sensible self-tests can evolve from it. They could
> instead evolve from some generic, basic PMTU (or route exceptions) test,
> rather than from my very particular fix that needs to involve so many
> steps to be checked.
> 

sure it can. I have a basic pmtu script that I wrote to test IPv6 for my
FIB change patch set. I will be submitting it in time as well.

We have to start somewhere, and it takes a commitment from multiple
people to make this happen.

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Miller]

From: Stefano Brivio 
Date: Mon, 5 Mar 2018 13:29:56 +0100

> And about corner cases, from Documentation/dev-tools/kselftest.rst:
> 
>   These are intended to be small tests to exercise individual code
>   paths in the kernel. Tests are intended to be run after building, 
> installing
>   and booting a kernel.
> 
> and:
> 
>   In general, the rules for selftests are
>   [...]
>* Don't take too long;
> 
> if you plan to request a self-test for every fix in the networking area,
> you need to substantially change the scope of these self-tests. This stuff
> would instead fit in a comprehensive networking test suite.

Nice try, but this logic doesn't hold.

It says don't make any "_INDIVIDUAL_" test take too long to run.
This allows handling timeouts on individual tests more sanely.

It absolutely does not say that we shouldn't have a lot of tests.

Why are you working so hard to avoid adding a nice test case for the
bug you are fixing?  This makes absolultely not sense at all.

I want as many tests as possible for the networking code, so please
write the test case you are being requested to add.

Thank you.

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[Stefano Brivio]

On Sun, 4 Mar 2018 18:11:41 -0700
David Ahern  wrote:

> On 3/4/18 4:12 PM, Stefano Brivio wrote:
> > On Sat, 3 Mar 2018 12:22:36 +0100
> > Stefano Brivio  wrote:
> >   
> >>> And please codify the above expectation as a test under
> >>> tools/testing/selftests/net
> >>
> >> And this, along with v2.  
> > 
> > On a second thought: I start thinking it doesn't make much sense,
> > especially given the current context of self-tests, to explicitly test
> > this, because it's a rather particular corner case.
> > 
> > I think it would make more sense to introduce generic tests first.
> > About, say, PMTU, or route exceptions, but not "tunnel causes route
> > exception and administrative change doesn't affect PMTU".
> >   
> 
> I would argue corner cases in particular should be documented.

Sure, but self-tests are not meant for documentation. I think commit
messages are.

And about corner cases, from Documentation/dev-tools/kselftest.rst:

These are intended to be small tests to exercise individual code
paths in the kernel. Tests are intended to be run after building, 
installing
and booting a kernel.

and:

In general, the rules for selftests are
[...]
 * Don't take too long;

if you plan to request a self-test for every fix in the networking area,
you need to substantially change the scope of these self-tests. This stuff
would instead fit in a comprehensive networking test suite.

> From the commit message it seems like you took the time to create a test
> setup using network namespaces. Throw those commands into a shell script
> -- tools/testing/selftests/net/mtu.sh. It can evolve from there.

My script sets up namespaces, veth and vti6 interfaces, xfrm states and
policies (could be replaced by vxlan, but that's what I have now). Then
it pings, waits, prints exception routes, changes MTU, etc. In the
commit message, I reported only the relevant parts that are enough to
clearly show the issue.

This script is some ugly monster I don't want to have on my conscience,
or wish for anybody to run as "small test to exercise individual code
paths".

I don't think sensible self-tests can evolve from it. They could
instead evolve from some generic, basic PMTU (or route exceptions) test,
rather than from my very particular fix that needs to involve so many
steps to be checked.

-- 
Stefano

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Ahern]

On 3/4/18 4:12 PM, Stefano Brivio wrote:
> On Sat, 3 Mar 2018 12:22:36 +0100
> Stefano Brivio  wrote:
> 
>>> And please codify the above expectation as a test under
>>> tools/testing/selftests/net  
>>
>> And this, along with v2.
> 
> On a second thought: I start thinking it doesn't make much sense,
> especially given the current context of self-tests, to explicitly test
> this, because it's a rather particular corner case.
> 
> I think it would make more sense to introduce generic tests first.
> About, say, PMTU, or route exceptions, but not "tunnel causes route
> exception and administrative change doesn't affect PMTU".
> 

I would argue corner cases in particular should be documented.

>From the commit message it seems like you took the time to create a test
setup using network namespaces. Throw those commands into a shell script
-- tools/testing/selftests/net/mtu.sh. It can evolve from there.

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[Stefano Brivio]

On Sat, 3 Mar 2018 12:22:36 +0100
Stefano Brivio  wrote:

> > And please codify the above expectation as a test under
> > tools/testing/selftests/net  
> 
> And this, along with v2.

On a second thought: I start thinking it doesn't make much sense,
especially given the current context of self-tests, to explicitly test
this, because it's a rather particular corner case.

I think it would make more sense to introduce generic tests first.
About, say, PMTU, or route exceptions, but not "tunnel causes route
exception and administrative change doesn't affect PMTU".

-- 
Stefano

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[Stefano Brivio]

On Fri, 2 Mar 2018 15:39:03 -0700
David Ahern  wrote:

> On 3/2/18 8:36 AM, Stefano Brivio wrote:
> > Currently, administrative MTU changes on a given netdevice are
> > not reflected on route exceptions for MTU-less routes, with a
> > set PMTU value, for that device:
> > 
> >  # ip -6 route get 3000::b
> >  3000::b from :: dev vti_a proto kernel src 3000::a metric 256 pref medium
> >  # ping6 -c 1 -q -s1 3000::b > /dev/null
> >  # ip netns exec a ip -6 route get 3000::b
> >  3000::b from :: dev vti_a src 3000::a metric 0
> >  cache expires 571sec mtu 4926 pref medium
> >  # ip link set dev vti_a mtu 3000
> >  # ip -6 route get 3000::b
> >  3000::b from :: dev vti_a src 3000::a metric 0
> >  cache expires 571sec mtu 4926 pref medium
> >  # ip link set dev vti_a mtu 9000
> >  # ip -6 route get 3000::b
> >  3000::b from :: dev vti_a src 3000::a metric 0
> >  cache expires 571sec mtu 4926 pref medium  
> 
> Addresses in the 2001:db8: range should be used for commit messages.

Thanks for pointing this out. I never related the "documentation
purposes" from RFC3849 to commit messages so far, but in the end this
is nothing else than documentation. I will post a v2 with updated
commit message.

> And please codify the above expectation as a test under
> tools/testing/selftests/net

And this, along with v2.

-- 
Stefano

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[Stefano Brivio]

Hi Maciej,

On Fri, 2 Mar 2018 10:54:36 -0800
Maciej Żenczykowski  wrote:

> I spend a significant fraction of my time making sure we never rely on PMTUD.

Thanks for your comments.

I see your point, but here we are not blindly relying on PMTUD,
rather reflecting an MTU administrative change on the PMTU, and making
the behaviour consistent between regular routes and exceptions, which
is nothing else than a bug fix.

This behaviour reflects RFC 8201, par. 3:

The basic idea is that a source node initially assumes that
the PMTU of a path is the (known) MTU of the first hop in the path.

and the need for it is clearly explained by the existing comment in
rt6_mtu_change_route():

/* For administrative MTU increase, there is no way to discover
   IPv6 PMTU increase, so PMTU increase should be updated here.
   Since RFC 1981 doesn't include administrative MTU increase
   update PMTU increase is a MUST. (i.e. jumbo frame)
 */

Letting that aside for a moment, a PMTU increase due to my fix is only
possible if the old local MTU (administratively set) was the lowest in
the path, no PMTUD happened meanwhile (but we have an exception route
in place e.g. due to a tunnel calling skb_dst_update_mtu()), and we get
a subsequent administrative change of the local MTU.

Relying on some old value set by the user is simply a bug, and breaks
the natural user assumption that increasing the MTU will have an
effect, if PMTU is not otherwise constrained.

If PMTUD is not working, we will rely on the MTU values set by the
user. This looks like the only sane thing to do.

> Debugging MTU related blackholes is a constant bane of my existence.
> 
> [btw. we're considering adding a hack to always fragment UDP to
> min(1280, dev/route/path mtu)...]
> 
> Basically: lower is always better because it's more likely to work...

This is not directly related to my fix, but I wonder if we shouldn't,
in general, simply comply with RFCs, and provide ways out in case the
network is broken, instead of breaking expected behaviours by default,
or making things work "by mistake". The way out, here, is as simple as
setting 1280 as MTU for the local interface.

Somebody might say higher is better because you avoid fragmentation. So
I would just keep the implementation compliant (and, perhaps more
importantly, consistent).

-- 
Stefano

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Ahern]

On 3/2/18 8:36 AM, Stefano Brivio wrote:
> Currently, administrative MTU changes on a given netdevice are
> not reflected on route exceptions for MTU-less routes, with a
> set PMTU value, for that device:
> 
>  # ip -6 route get 3000::b
>  3000::b from :: dev vti_a proto kernel src 3000::a metric 256 pref medium
>  # ping6 -c 1 -q -s1 3000::b > /dev/null
>  # ip netns exec a ip -6 route get 3000::b
>  3000::b from :: dev vti_a src 3000::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 3000
>  # ip -6 route get 3000::b
>  3000::b from :: dev vti_a src 3000::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 9000
>  # ip -6 route get 3000::b
>  3000::b from :: dev vti_a src 3000::a metric 0
>  cache expires 571sec mtu 4926 pref medium

Addresses in the 2001:db8: range should be used for commit messages.

And please codify the above expectation as a test under
tools/testing/selftests/net

Re: [PATCH net] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[Maciej Żenczykowski]

Conceptually this is right.

And I'm 100% fine with dev mtu change triggering pmtu decrease.

I'm not so sold on the pmtu increase.

PMTUD is one of those things that never ever works right in practice.
There's too many icmp blackholes, rate limits, overloaded management
cpus in switches,
misconfigurations, missing tcp mss clamps, icmps routed differently
then the flows due to ecmp hashing, middle boxes that don't affect the
icmp but change the tcp stream, etc.

In particular there's a lot of routing hardware that can handle
gigabits or terabits of traffic, but can generate only 10s-100s of
packet too big messages per second (ie. a tiny fraction of line rate
pps).  Worse yet, under overload it often falls back to simply
dropping and generating no icmp errors.

I spend a significant fraction of my time making sure we never rely on PMTUD.

Debugging MTU related blackholes is a constant bane of my existence.

[btw. we're considering adding a hack to always fragment UDP to
min(1280, dev/route/path mtu)...]

Basically: lower is always better because it's more likely to work...