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

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

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.

Otherwise, you're changing the behaviour again, that is, if lowerdev is
not present, we want to keep 1500 and not set ETH_MAX_MTU (65535).

Sure you can change the if condition to reflect that, but IMHO it
becomes quite awkward.

Hmm, right. So here's a

Reviewed-by: Matthias Schiffer <redacted>

for the minimal change for -stable.