From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kodanev Subject: Re: [PATCH net] vxlan: Restore initial MTU setting based on lower device Date: Thu, 14 Dec 2017 14:23:36 +0300 Message-ID: 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=utf-8 Content-Transfer-Encoding: 7bit Cc: "David S . Miller" , netdev@vger.kernel.org, Junhan Yan , Jiri Benc , Hangbin Liu To: Stefano Brivio , Matthias Schiffer Return-path: Received: from userp2120.oracle.com ([156.151.31.85]:43758 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751646AbdLNLOq (ORCPT ); Thu, 14 Dec 2017 06:14:46 -0500 In-Reply-To: <20171214013148.3da52cc9@elisabeth> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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... 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. >