From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Brivio Subject: Re: [PATCH net] vxlan: Restore initial MTU setting based on lower device Date: Thu, 14 Dec 2017 13:36:47 +0100 Message-ID: <20171214133647.1c1b82d1@elisabeth> References: <0c6caef03156aa51673c50ebb59889fc001b74be.1513198761.git.sbrivio@redhat.com> <20171214011042.6b4a2e8b@elisabeth> <42862507-a573-af7a-d4aa-fd8cdd89c01e@universe-factory.net> <20171214013148.3da52cc9@elisabeth> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Matthias Schiffer , "David S . Miller" , netdev@vger.kernel.org, Junhan Yan , Jiri Benc , Hangbin Liu To: Alexey Kodanev Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37332 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751755AbdLNMg6 (ORCPT ); Thu, 14 Dec 2017 07:36:58 -0500 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 14 Dec 2017 14:23:36 +0300 Alexey Kodanev wrote: > On 12/14/2017 03:31 AM, Stefano Brivio wrote: > > On Thu, 14 Dec 2017 01:25:40 +0100 > > Matthias Schiffer wrote: > > > >> On 12/14/2017 01:10 AM, Stefano Brivio wrote: > >>> On Thu, 14 Dec 2017 00:57:32 +0100 > >>> Matthias Schiffer 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