On Thu, 14 Dec 2017 14:23:36 +0300
Alexey Kodanev <alexey.koda...@oracle.com> wrote:
> 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...
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.
> 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;
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
