[PATCH net] geneve: allow changing DF behavior after creation

public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed

* [PATCH net] geneve: allow changing DF behavior after creation
@ 2020-06-18 10:13 Sabrina Dubroca
  2020-06-18 10:26 ` Stefano Brivio
  2020-06-20  3:07 ` David Miller
  0 siblings, 2 replies; 5+ messages in thread
From: Sabrina Dubroca @ 2020-06-18 10:13 UTC (permalink / raw)
  To: netdev; +Cc: Sabrina Dubroca, Stefano Brivio

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;
 	geneve_unquiesce(geneve, gs4, gs6);
 
 	return 0;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net] geneve: allow changing DF behavior after creation
  2020-06-18 10:13 [PATCH net] geneve: allow changing DF behavior after creation Sabrina Dubroca
@ 2020-06-18 10:26 ` Stefano Brivio
  2020-06-18 13:03   ` Sabrina Dubroca
  2020-06-20  3:07 ` David Miller
  1 sibling, 1 reply; 5+ messages in thread
From: Stefano Brivio @ 2020-06-18 10:26 UTC (permalink / raw)
  To: Sabrina Dubroca; +Cc: netdev

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>

-- 
Stefano


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] geneve: allow changing DF behavior after creation
  2020-06-18 10:26 ` Stefano Brivio
@ 2020-06-18 13:03   ` Sabrina Dubroca
  2020-06-18 15:20     ` Stefano Brivio
  0 siblings, 1 reply; 5+ messages in thread
From: Sabrina Dubroca @ 2020-06-18 13:03 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: netdev

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.

-- 
Sabrina


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] geneve: allow changing DF behavior after creation
  2020-06-18 13:03   ` Sabrina Dubroca
@ 2020-06-18 15:20     ` Stefano Brivio
  0 siblings, 0 replies; 5+ messages in thread
From: Stefano Brivio @ 2020-06-18 15:20 UTC (permalink / raw)
  To: Sabrina Dubroca; +Cc: netdev

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.

-- 
Stefano


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] geneve: allow changing DF behavior after creation
  2020-06-18 10:13 [PATCH net] geneve: allow changing DF behavior after creation Sabrina Dubroca
  2020-06-18 10:26 ` Stefano Brivio
@ 2020-06-20  3:07 ` David Miller
  1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2020-06-20  3:07 UTC (permalink / raw)
  To: sd; +Cc: netdev, sbrivio

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-06-20  3:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-18 10:13 [PATCH net] geneve: allow changing DF behavior after creation Sabrina Dubroca
2020-06-18 10:26 ` Stefano Brivio
2020-06-18 13:03   ` Sabrina Dubroca
2020-06-18 15:20     ` Stefano Brivio
2020-06-20  3:07 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox