From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sabrina Dubroca Subject: Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes Date: Thu, 29 Nov 2018 16:51:47 +0100 Message-ID: <20181129155147.GA31740@bistromath.localdomain> References: <1543444079-29883-1-git-send-email-roopa@cumulusnetworks.com> <1543444079-29883-2-git-send-email-roopa@cumulusnetworks.com> <20181129135631.GA20478@bistromath.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: David Miller , netdev To: Roopa Prabhu Return-path: Received: from mx1.redhat.com ([209.132.183.28]:36942 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729020AbeK3C5i (ORCPT ); Thu, 29 Nov 2018 21:57:38 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 2018-11-29, 07:33:11 -0800, Roopa Prabhu wrote: > On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca wrote: > > > > 2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote: > > > From: Roopa Prabhu > > > > > > We started very conservative when supporting changelink > > > especially because not all attribute changes could be > > > tested. This patch opens up a few more attributes for > > > changelink. The reason for choosing this set of attributes > > > is based on code references for these attributes. I have > > > tested TTL changes and did some changelink api testing > > > to sanity test the others. > > > > > > Signed-off-by: Roopa Prabhu > > > --- > > > drivers/net/vxlan.c | 36 ++++-------------------------------- > > > 1 file changed, 4 insertions(+), 32 deletions(-) > > > > > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > > > index 9110662..73caa65 100644 > > > --- a/drivers/net/vxlan.c > > > +++ b/drivers/net/vxlan.c > > > @@ -3438,11 +3438,8 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[], > > > if (data[IFLA_VXLAN_TTL]) > > > conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]); > > > > > > - if (data[IFLA_VXLAN_TTL_INHERIT]) { > > > - if (changelink) > > > - return -EOPNOTSUPP; > > > + if (data[IFLA_VXLAN_TTL_INHERIT]) > > > conf->flags |= VXLAN_F_TTL_INHERIT; > > > - } > > > > This doesn't give us an option to disable TTL_INHERIT after it was > > enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL. > > that is provided by patch3, with the changelink patch. I can squash > them, if thats easier. Yes, I see it now. Sequential review got me :( I think you can just add a note in the commit message, "will be fixed in a further patch introducing the vxlan_nl2flag() helper" or similar. > > > - if (data[IFLA_VXLAN_GBP]) { > > > - if (changelink) > > > - return -EOPNOTSUPP; > > > + if (data[IFLA_VXLAN_GBP]) > > > conf->flags |= VXLAN_F_GBP; > > > - } > > > > > > - if (data[IFLA_VXLAN_GPE]) { > > > - if (changelink) > > > - return -EOPNOTSUPP; > > > + if (data[IFLA_VXLAN_GPE]) > > > conf->flags |= VXLAN_F_GPE; > > > - } > > > > GPE implies running a different setup function (vxlan_raw_setup() vs > > vxlan_ether_setup()), that vxlan_config_apply() only calls for > > !changelink. I think this is incomplete. > > > > I think we'd also end up with mixed tunnel types (GPE/!GPE) on the > > same socket, I'm not sure how that would work. Normally, they each try > > to create a separate socket, and pass the GPE flag on to the > > associated vxlan_sock. I suspect that's also a problem with rx > > offload. > > that is good to know. I will drop the change to GPE and also the rx > offload flag and let somebody else using it > open it up for changelink. thanks for the review Looking at vxlan_rcv(), there's also a possible issue with the GBP flag: if (vs->flags & VXLAN_F_GBP) vxlan_parse_gbp_hdr(&unparsed, skb, vs->flags, md); I don't know why that flag must match on all tunnels for a socket, but given that it was the reason for commit ac5132d1a03f ("vxlan: Only bind to sockets with compatible flags enabled"), I'd leave it alone as well. > > > - if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) { > > > - if (changelink) > > > - return -EOPNOTSUPP; > > > + if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) > > > conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL; > > > - } > > > > > > if (tb[IFLA_MTU]) { > > > if (changelink) > > > -- -- Sabrina