On 12/14/2017 03:31 AM, Stefano Brivio wrote:
> On Thu, 14 Dec 2017 01:25:40 +0100
> Matthias Schiffer <mschif...@universe-factory.net> wrote:
>
>> On 12/14/2017 01:10 AM, Stefano Brivio wrote:
>>> On Thu, 14 Dec 2017 00:57:32 +0100
>>> Matthias Schiffer <mschif...@universe-factory.net> wrote:
>>>
>>>> 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):
>>
>>> --- 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...
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 19b9cc5..1000b0e 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3103,6 +3103,11 @@ static void vxlan_config_apply(struct net_device *dev,
max_mtu = lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADROOM :
VXLAN_HEADROOM);
+ if (max_mtu < ETH_MIN_MTU)
+ max_mtu = ETH_MIN_MTU;
+
+ if (!changelink && !conf->mtu)
+ dev->mtu = max_mtu;
}
if (dev->mtu > max_mtu)
Thanks,
Alexey
> 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.
>
