From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Schiffer Subject: Re: [PATCH net] vxlan: Restore initial MTU setting based on lower device Date: Thu, 14 Dec 2017 01:53:25 +0100 Message-ID: <80b1cd2d-cba4-cb00-c319-472f888be4fc@universe-factory.net> 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: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="PKsGeqW7vbBixw0sRnAlqFvu4hIlnGcR2" Cc: "David S . Miller" , netdev@vger.kernel.org, Junhan Yan , Jiri Benc , Hangbin Liu To: Stefano Brivio Return-path: Received: from chaos.universe-factory.net ([31.24.148.19]:44672 "EHLO chaos.universe-factory.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750749AbdLNAx1 (ORCPT ); Wed, 13 Dec 2017 19:53:27 -0500 In-Reply-To: <20171214013148.3da52cc9@elisabeth> Sender: netdev-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --PKsGeqW7vbBixw0sRnAlqFvu4hIlnGcR2 Content-Type: multipart/mixed; boundary="dAomGa7NgeuVJJFQIRCGPT7K14J4WeRew"; protected-headers="v1" From: Matthias Schiffer To: Stefano Brivio Cc: "David S . Miller" , netdev@vger.kernel.org, Junhan Yan , Jiri Benc , Hangbin Liu Message-ID: <80b1cd2d-cba4-cb00-c319-472f888be4fc@universe-factory.net> Subject: Re: [PATCH net] vxlan: Restore initial MTU setting based on lower device References: <0c6caef03156aa51673c50ebb59889fc001b74be.1513198761.git.sbrivio@redhat.com> <20171214011042.6b4a2e8b@elisabeth> <42862507-a573-af7a-d4aa-fd8cdd89c01e@universe-factory.net> <20171214013148.3da52cc9@elisabeth> In-Reply-To: <20171214013148.3da52cc9@elisabeth> --dAomGa7NgeuVJJFQIRCGPT7K14J4WeRew Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 12/14/2017 01:31 AM, Stefano Brivio wrote: > On Thu, 14 Dec 2017 01:25:40 +0100 > Matthias Schiffer wrote: >=20 >> On 12/14/2017 01:10 AM, Stefano Brivio wrote: >>> On Thu, 14 Dec 2017 00:57:32 +0100 >>> Matthias Schiffer wrote: >>> =20 >>>> As you note, there is another occurrence of this calculation in >>>> vxlan_config_apply(): >>>> >>>> >>>> [...] >>>> if (lowerdev) { >>>> [...] >>>> max_mtu =3D lowerdev->mtu - (use_ipv6 ? VXLAN6_HEADR= OOM : >>>> VXLAN_HEADROOM); >>>> } >>>> >>>> if (dev->mtu > max_mtu) >>>> dev->mtu =3D max_mtu; >>>> [...] >>>> >>>> >>>> Unless I'm overlooking something, this should already do the same th= ing and >>>> your patch is redundant. =20 >>> >>> 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 matt= er >>> 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. =20 >> >> 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 al= l): >> >>> --- a/drivers/net/vxlan.c >>> +++ b/drivers/net/vxlan.c >>> @@ -3105,7 +3105,7 @@ static void vxlan_config_apply(struct net_devic= e *dev, >>> VXLAN_HEADROOM); >>> } >>> =20 >>> - if (dev->mtu > max_mtu) >>> + if (dev->mtu > max_mtu || (!changelink && !conf->mtu)) >>> dev->mtu =3D max_mtu; >=20 > You would also need to check that lowerdev is present, though. >=20 > 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). >=20 > Sure you can change the if condition to reflect that, but IMHO it > becomes quite awkward. >=20 Hmm, right. So here's a Reviewed-by: Matthias Schiffer for the minimal change for -stable. --dAomGa7NgeuVJJFQIRCGPT7K14J4WeRew-- --PKsGeqW7vbBixw0sRnAlqFvu4hIlnGcR2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEZmTnvaa2aYgexS51Fu8/ZMsgHZwFAloxy4UACgkQFu8/ZMsg HZwW5g/9Hzr0XlRov1R3oLxQgRTLINYJSZEwDp0JBVSH/cE4HtMsqPfdxbVVgkHZ c7bjZ9s0Ny0cHxkxvJnWpOtNEVdjPV0wBQSLYXyOZWM2O9zET6IIJ1LsRLYUl5l0 Zn/q18R8IOl7IO9+0tVquniSMXxCefyf5pb8F10mjsQKk+0sS4VHmYQGwmY5bRLd GW0mkOJ+a3yvuMJpBWhZb9b3rHcja+yyYBY9wBWYaN3+ym0+AM40s7YM/5RuP7iy OPffpeHwtBKkXHEnioLzQYrU+8p8WeidniVgGk8e38uMA/d0cVNzyMNou3r13Ppq B2zK6z6TNgh6/WmhmWUI3o/t9llh+f1zZzJaclLeIXv1uZdMre9SsBOGcGQNOgk2 ORWVBVKG6kjuqme9H9DD0iZ9T9QfEF8QRWCEcTn2+z1SyXTG5C5kxHQYwbdetsSD 7C/QOd+KuSbUICw5w/4o/n6DmDngbnI7Nuiy2sasB2eO+a2zJgGMIFX1LMRi+V3y ZqjUDFhA2cYSkd15VvCbxqbN+qaezp0parQW7xmeCbJHWeXJBppxOcqgDHePgzbh Vm/wMFZhgMenBskA2jXkBNqHkePohUmGt+iecVw5O3HqhHbA4pW/pY/8Xeup0tNT kTuQmiRB3QjCT+uGHrxyNUuNvBNLl/gnpvrQsnhpgfW+95o4aho= =Z9Ap -----END PGP SIGNATURE----- --PKsGeqW7vbBixw0sRnAlqFvu4hIlnGcR2--