2018-11-29, 07:33:11 -0800, Roopa Prabhu wrote: > On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca <s...@queasysnail.net> wrote: > > > > 2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote: > > > From: Roopa Prabhu <ro...@cumulusnetworks.com> > > > > > > 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 <ro...@cumulusnetworks.com> > > > --- > > > 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