| Message ID | 3b72fc01841507f8439a90f618ef6f6240b9463f.1592473442.git.sd@queasysnail.net |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] geneve: allow changing DF behavior after creation | expand |
On Thu, 18 Jun 2020 12:13:22 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote: > Currently, trying to change the DF parameter of a geneve device does > nothing: > > # ip -d link show geneve1 > 14: geneve1: <snip> > link/ether <snip> > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > # ip link set geneve1 type geneve id 1 df unset > # ip -d link show geneve1 > 14: geneve1: <snip> > link/ether <snip> > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > > We just need to update the value in geneve_changelink. > > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour") > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > --- > drivers/net/geneve.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 75266580b586..4661ef865807 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[], > geneve->collect_md = metadata; > geneve->use_udp6_rx_checksums = use_udp6_rx_checksums; > geneve->ttl_inherit = ttl_inherit; > + geneve->df = df; I introduced this bug as I didn't notice the asymmetry with VXLAN, where vxlan_nl2conf() takes care of this for both new links and link changes. Here, this block is duplicated in geneve_configure(), which, somewhat surprisingly given the name, is not called from geneve_changelink(). Did you consider factoring out (at least) this block to have it shared? Either way, Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
2020-06-18, 12:26:29 +0200, Stefano Brivio wrote: > On Thu, 18 Jun 2020 12:13:22 +0200 > Sabrina Dubroca <sd@queasysnail.net> wrote: > > > Currently, trying to change the DF parameter of a geneve device does > > nothing: > > > > # ip -d link show geneve1 > > 14: geneve1: <snip> > > link/ether <snip> > > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > > # ip link set geneve1 type geneve id 1 df unset > > # ip -d link show geneve1 > > 14: geneve1: <snip> > > link/ether <snip> > > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > > > > We just need to update the value in geneve_changelink. > > > > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour") > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > > --- > > drivers/net/geneve.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > index 75266580b586..4661ef865807 100644 > > --- a/drivers/net/geneve.c > > +++ b/drivers/net/geneve.c > > @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[], > > geneve->collect_md = metadata; > > geneve->use_udp6_rx_checksums = use_udp6_rx_checksums; > > geneve->ttl_inherit = ttl_inherit; > > + geneve->df = df; > > I introduced this bug as I didn't notice the asymmetry with VXLAN, > where vxlan_nl2conf() takes care of this for both new links and link > changes. Yeah, I didn't notice either :/ > Here, this block is duplicated in geneve_configure(), which, > somewhat surprisingly given the name, is not called from > geneve_changelink(). Did you consider factoring out (at least) this > block to have it shared? Then I'd have to introduce another lovely function with an absurdly long argument list. I'd rather clean that up in all of geneve and introduce something like struct vxlan_config, but it's a bit much for net. I'll do that once this fix finds its way into net-next. > > Either way, > > Reviewed-by: Stefano Brivio <sbrivio@redhat.com> Thanks.
On Thu, 18 Jun 2020 15:03:47 +0200 Sabrina Dubroca <sd@queasysnail.net> wrote: > 2020-06-18, 12:26:29 +0200, Stefano Brivio wrote: > > On Thu, 18 Jun 2020 12:13:22 +0200 > > Sabrina Dubroca <sd@queasysnail.net> wrote: > > > > > Currently, trying to change the DF parameter of a geneve device does > > > nothing: > > > > > > # ip -d link show geneve1 > > > 14: geneve1: <snip> > > > link/ether <snip> > > > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > > > # ip link set geneve1 type geneve id 1 df unset > > > # ip -d link show geneve1 > > > 14: geneve1: <snip> > > > link/ether <snip> > > > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > > > > > > We just need to update the value in geneve_changelink. > > > > > > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour") > > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > > > --- > > > drivers/net/geneve.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > > index 75266580b586..4661ef865807 100644 > > > --- a/drivers/net/geneve.c > > > +++ b/drivers/net/geneve.c > > > @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[], > > > geneve->collect_md = metadata; > > > geneve->use_udp6_rx_checksums = use_udp6_rx_checksums; > > > geneve->ttl_inherit = ttl_inherit; > > > + geneve->df = df; > > > > I introduced this bug as I didn't notice the asymmetry with VXLAN, > > where vxlan_nl2conf() takes care of this for both new links and link > > changes. > > Yeah, I didn't notice either :/ > > > Here, this block is duplicated in geneve_configure(), which, > > somewhat surprisingly given the name, is not called from > > geneve_changelink(). Did you consider factoring out (at least) this > > block to have it shared? > > Then I'd have to introduce another lovely function with an absurdly > long argument list. I'd rather clean that up in all of geneve and > introduce something like struct vxlan_config, but it's a bit much for > net. I'll do that once this fix finds its way into net-next. Yeah, sure, I didn't mean you should simply copy and paste that somewhere. Either something like struct vxlan_config used around the current logic, or perhaps even better, something like vxlan_nl2conf() does. I didn't check whether something in GENEVE is so special compared to VXLAN as to prevent this, though.
From: Sabrina Dubroca <sd@queasysnail.net> Date: Thu, 18 Jun 2020 12:13:22 +0200 > Currently, trying to change the DF parameter of a geneve device does > nothing: > > # ip -d link show geneve1 > 14: geneve1: <snip> > link/ether <snip> > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > # ip link set geneve1 type geneve id 1 df unset > # ip -d link show geneve1 > 14: geneve1: <snip> > link/ether <snip> > geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> > > We just need to update the value in geneve_changelink. > > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour") > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> Applied and queued up for -stable, thanks.
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 75266580b586..4661ef865807 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[], geneve->collect_md = metadata; geneve->use_udp6_rx_checksums = use_udp6_rx_checksums; geneve->ttl_inherit = ttl_inherit; + geneve->df = df; geneve_unquiesce(geneve, gs4, gs6); return 0;
Currently, trying to change the DF parameter of a geneve device does nothing: # ip -d link show geneve1 14: geneve1: <snip> link/ether <snip> geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> # ip link set geneve1 type geneve id 1 df unset # ip -d link show geneve1 14: geneve1: <snip> link/ether <snip> geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip> We just need to update the value in geneve_changelink. Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- drivers/net/geneve.c | 1 + 1 file changed, 1 insertion(+)