Re: [PATCH net] vxlan: Restore initial MTU setting based on lower device

On Thu, 14 Dec 2017 14:23:36 +0300
Alexey Kodanev [off-list ref] wrote:

On 12/14/2017 03:31 AM, Stefano Brivio wrote:

quoted

On Thu, 14 Dec 2017 01:25:40 +0100
Matthias Schiffer [off-list ref] wrote:
  

quoted

On 12/14/2017 01:10 AM, Stefano Brivio wrote:  

quoted

On Thu, 14 Dec 2017 00:57:32 +0100
Matthias Schiffer [off-list ref] wrote:
    

quoted

As you note, there is another occurrence of this calculation in
vxlan_config_apply():


[...]
        if (lowerdev) {
[...]
                max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
                                           VXLAN_HEADROOM);
        }

        if (dev->mtu > max_mtu)
                dev->mtu = max_mtu;
[...]


Unless I'm overlooking something, this should already do the same thing and
your patch is redundant.    

The code above sets max_mtu, and only if dev->mtu exceeds that, the
latter is then clamped.

What my patch does is to actually set dev->mtu to that value, no matter
what's the previous value set by ether_setup() (only on creation, and
only if lowerdev is there), just like the previous behaviour used to be.

Let's consider these two cases, on the existing code:

1. lowerdev->mtu is 1500:
   - ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
   - here max_mtu is 1450
   - we enter the second if clause above (dev->mtu > max_mtu)
   - at the end of vxlan_config_apply(), dev->mtu will be 1450

which is consistent with the previous behaviour.

2. lowerdev->mtu is 9000:
   - ether_setup(), called by vxlan_setup(), sets dev->mtu to 1500
   - here max_mtu is 8950
   - we do not enter the second if clause above (dev->mtu < max_mtu)
   - at the end of vxlan_config_apply(), dev->mtu will still be 1500

which is not consistent with the previous behaviour, where it used to
be 8950 instead.    

Ah, thank you for the explanation, I was missing the context that this was
about higher rather than lower MTUs.

Personally, I would prefer a change like the following, as it does not
introduce another duplication of the MTU calculation (not tested at all):
 

quoted

--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c

@@ -3105,7 +3105,7 @@ static void vxlan_config_apply(struct net_device *dev,
 					   VXLAN_HEADROOM);
 	}
 
-	if (dev->mtu > max_mtu)
+	if (dev->mtu > max_mtu || (!changelink && !conf->mtu))
 		dev->mtu = max_mtu;  

You would also need to check that lowerdev is present, though.
  

if we move it up in "if (lowerdev) { ..." branch we will be checking the presence
of "lowerdev" and also not calculating it again. Also I would check max_mtu for
minimum as it might happen to be negative, though unlikely corner case...

Indeed it might happen to be negative (only for IPv6, down to -2), good
catch.

For the benefit of others: it took me a few minutes to see how this is
*not* unrelated, because we are introducing a direct assignment of
dev->mtu to set max_mtu, whereas earlier it was just used in
comparisons, so it didn't matter whether it was negative.

I don't really have a strong preference here. On one hand, you're
hiding this a bit from the "device creation" path. On the other hand,
it's a bit more compact. So I'm also fine with this.

Can you perhaps submit a formal patch?

-- 
Stefano