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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 14 Dec 2017 14:23:36 +0300
Alexey Kodanev <alexey.kodanev@oracle.com> wrote:

> On 12/14/2017 03:31 AM, Stefano Brivio wrote:
> > On Thu, 14 Dec 2017 01:25:40 +0100
> > Matthias Schiffer <mschiffer@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 <mschiffer@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