All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] team: set IFF_SLAVE on team ports
@ 2015-07-09  9:58 Jan Blunck
  2015-07-09 10:07 ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Blunck @ 2015-07-09  9:58 UTC (permalink / raw)
  To: linux-kernel; +Cc: jiri, netdev

The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
decide if it should start the address configuration. Since team ports
shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
a port to the team master.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/team/team.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index daa054b..4cd02c8 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1086,6 +1086,7 @@ static int team_upper_dev_link(struct net_device *dev,
 	err = netdev_master_upper_dev_link(port_dev, dev);
 	if (err)
 		return err;
+	port_dev->flags |= IFF_SLAVE;
 	port_dev->priv_flags |= IFF_TEAM_PORT;
 	return 0;
 }
@@ -1094,6 +1095,7 @@ static void team_upper_dev_unlink(struct net_device *dev,
 				  struct net_device *port_dev)
 {
 	netdev_upper_dev_unlink(port_dev, dev);
+	port_dev->flags &= ~IFF_SLAVE;
 	port_dev->priv_flags &= ~IFF_TEAM_PORT;
 }
 
-- 
2.1.4


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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2015-07-09  9:58 [PATCH] team: set IFF_SLAVE on team ports Jan Blunck
@ 2015-07-09 10:07 ` Jiri Pirko
  2015-07-09 15:36   ` Jan Blunck
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2015-07-09 10:07 UTC (permalink / raw)
  To: Jan Blunck; +Cc: linux-kernel, netdev

Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>decide if it should start the address configuration. Since team ports
>shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>a port to the team master.

I don't want to use IFF_SLAVE in team. Other master-slave devices are
not using that as well, for example bridge, ovs, etc.

I think that this should be fixed in addrconf_notify. It should lookup
if there is a master on top and bail out in that case.


>
>Signed-off-by: Jan Blunck <jblunck@infradead.org>
>---
> drivers/net/team/team.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index daa054b..4cd02c8 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -1086,6 +1086,7 @@ static int team_upper_dev_link(struct net_device *dev,
> 	err = netdev_master_upper_dev_link(port_dev, dev);
> 	if (err)
> 		return err;
>+	port_dev->flags |= IFF_SLAVE;
> 	port_dev->priv_flags |= IFF_TEAM_PORT;
> 	return 0;
> }
>@@ -1094,6 +1095,7 @@ static void team_upper_dev_unlink(struct net_device *dev,
> 				  struct net_device *port_dev)
> {
> 	netdev_upper_dev_unlink(port_dev, dev);
>+	port_dev->flags &= ~IFF_SLAVE;
> 	port_dev->priv_flags &= ~IFF_TEAM_PORT;
> }
> 
>-- 
>2.1.4
>

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2015-07-09 10:07 ` Jiri Pirko
@ 2015-07-09 15:36   ` Jan Blunck
  2015-07-10  6:41     ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Blunck @ 2015-07-09 15:36 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: LKML, netdev

On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>decide if it should start the address configuration. Since team ports
>>shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>a port to the team master.
>
> I don't want to use IFF_SLAVE in team. Other master-slave devices are
> not using that as well, for example bridge, ovs, etc.
>

Maybe they need to get fixed too. I've used that flag because it is
documented as
a "slave of a load balancer" which describes what a team port is.


> I think that this should be fixed in addrconf_notify. It should lookup
> if there is a master on top and bail out in that case.

There are other virtual interfaces that have a master assigned and want to
participate in IPv6 address configuration.

Unless we want to have a cascade of conditionals testing the priv_flags in
addrconf_notify() this is asking for a new net_device_flags flag.
Maybe something
generic like IFF_L2PORT ?

Thanks,
Jan

[ Jiri, sorry for getting that mail twice ]

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2015-07-09 15:36   ` Jan Blunck
@ 2015-07-10  6:41     ` Jiri Pirko
  2018-09-27 14:04       ` Chas Williams
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2015-07-10  6:41 UTC (permalink / raw)
  To: Jan Blunck; +Cc: LKML, netdev

Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>decide if it should start the address configuration. Since team ports
>>>shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>a port to the team master.
>>
>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>> not using that as well, for example bridge, ovs, etc.
>>
>
>Maybe they need to get fixed too. I've used that flag because it is
>documented as
>a "slave of a load balancer" which describes what a team port is.
>
>
>> I think that this should be fixed in addrconf_notify. It should lookup
>> if there is a master on top and bail out in that case.
>
>There are other virtual interfaces that have a master assigned and want to
>participate in IPv6 address configuration.

Can you give me an example?


>
>Unless we want to have a cascade of conditionals testing the priv_flags in
>addrconf_notify() this is asking for a new net_device_flags flag.
>Maybe something
>generic like IFF_L2PORT ?
>
>Thanks,
>Jan
>
>[ Jiri, sorry for getting that mail twice ]

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2015-07-10  6:41     ` Jiri Pirko
@ 2018-09-27 14:04       ` Chas Williams
  2018-09-30  7:14         ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Chas Williams @ 2018-09-27 14:04 UTC (permalink / raw)
  To: Jiri Pirko, Jan Blunck; +Cc: LKML, netdev



On 07/10/15 02:41, Jiri Pirko wrote:
> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>> decide if it should start the address configuration. Since team ports
>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>> a port to the team master.
>>>
>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>> not using that as well, for example bridge, ovs, etc.
>>>
>>
>> Maybe they need to get fixed too. I've used that flag because it is
>> documented as
>> a "slave of a load balancer" which describes what a team port is.
>>
>>
>>> I think that this should be fixed in addrconf_notify. It should lookup
>>> if there is a master on top and bail out in that case.
>>
>> There are other virtual interfaces that have a master assigned and want to
>> participate in IPv6 address configuration.
> 
> Can you give me an example?

I would like to revisit this patch (yes, I know it has been a while).  I 
believe the VRF implementation uses master to group the interfaces under
a single interface.

I don't see a reason not to use IFF_SLAVE since team and bonding are 
fairly similar.

>>
>> Unless we want to have a cascade of conditionals testing the priv_flags in
>> addrconf_notify() this is asking for a new net_device_flags flag.
>> Maybe something
>> generic like IFF_L2PORT ?
>>
>> Thanks,
>> Jan
>>
>> [ Jiri, sorry for getting that mail twice ]
> 
> 
> 

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-09-27 14:04       ` Chas Williams
@ 2018-09-30  7:14         ` Jiri Pirko
  2018-09-30  9:38           ` Stephen Hemminger
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2018-09-30  7:14 UTC (permalink / raw)
  To: Chas Williams; +Cc: Jan Blunck, LKML, netdev

Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>
>
>On 07/10/15 02:41, Jiri Pirko wrote:
>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>> > On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>> > > > The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>> > > > decide if it should start the address configuration. Since team ports
>> > > > shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>> > > > a port to the team master.
>> > > 
>> > > I don't want to use IFF_SLAVE in team. Other master-slave devices are
>> > > not using that as well, for example bridge, ovs, etc.
>> > > 
>> > 
>> > Maybe they need to get fixed too. I've used that flag because it is
>> > documented as
>> > a "slave of a load balancer" which describes what a team port is.
>> > 
>> > 
>> > > I think that this should be fixed in addrconf_notify. It should lookup
>> > > if there is a master on top and bail out in that case.
>> > 
>> > There are other virtual interfaces that have a master assigned and want to
>> > participate in IPv6 address configuration.
>> 
>> Can you give me an example?
>
>I would like to revisit this patch (yes, I know it has been a while).  I
>believe the VRF implementation uses master to group the interfaces under
>a single interface.
>
>I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>similar.

Again, why do you need team port to have IFF_SLAVE flag? What do you
want to achieve?


>
>> > 
>> > Unless we want to have a cascade of conditionals testing the priv_flags in
>> > addrconf_notify() this is asking for a new net_device_flags flag.
>> > Maybe something
>> > generic like IFF_L2PORT ?
>> > 
>> > Thanks,
>> > Jan
>> > 
>> > [ Jiri, sorry for getting that mail twice ]
>> 
>> 
>> 

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-09-30  9:38           ` Stephen Hemminger
@ 2018-09-30  9:34             ` Jiri Pirko
  2018-10-01 14:06               ` Chas Williams
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2018-09-30  9:34 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Chas Williams, Jan Blunck, LKML, netdev

Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>On Sun, 30 Sep 2018 09:14:14 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>> >
>> >
>> >On 07/10/15 02:41, Jiri Pirko wrote:  
>> >> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:  
>> >> > On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:  
>> >> > > Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:  
>> >> > > > The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>> >> > > > decide if it should start the address configuration. Since team ports
>> >> > > > shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>> >> > > > a port to the team master.  
>> >> > > 
>> >> > > I don't want to use IFF_SLAVE in team. Other master-slave devices are
>> >> > > not using that as well, for example bridge, ovs, etc.
>> >> > >   
>> >> > 
>> >> > Maybe they need to get fixed too. I've used that flag because it is
>> >> > documented as
>> >> > a "slave of a load balancer" which describes what a team port is.
>> >> > 
>> >> >   
>> >> > > I think that this should be fixed in addrconf_notify. It should lookup
>> >> > > if there is a master on top and bail out in that case.  
>> >> > 
>> >> > There are other virtual interfaces that have a master assigned and want to
>> >> > participate in IPv6 address configuration.  
>> >> 
>> >> Can you give me an example?  
>> >
>> >I would like to revisit this patch (yes, I know it has been a while).  I
>> >believe the VRF implementation uses master to group the interfaces under
>> >a single interface.
>> >
>> >I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>> >similar.  
>> 
>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>> want to achieve
>
>Without setting this flag IPv6 will try and make a link specific address.

Why is it not an issue with bridge, ovs, and other master-slave devices?

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-09-30  7:14         ` Jiri Pirko
@ 2018-09-30  9:38           ` Stephen Hemminger
  2018-09-30  9:34             ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2018-09-30  9:38 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Chas Williams, Jan Blunck, LKML, netdev

On Sun, 30 Sep 2018 09:14:14 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
> >
> >
> >On 07/10/15 02:41, Jiri Pirko wrote:  
> >> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:  
> >> > On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:  
> >> > > Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:  
> >> > > > The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
> >> > > > decide if it should start the address configuration. Since team ports
> >> > > > shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
> >> > > > a port to the team master.  
> >> > > 
> >> > > I don't want to use IFF_SLAVE in team. Other master-slave devices are
> >> > > not using that as well, for example bridge, ovs, etc.
> >> > >   
> >> > 
> >> > Maybe they need to get fixed too. I've used that flag because it is
> >> > documented as
> >> > a "slave of a load balancer" which describes what a team port is.
> >> > 
> >> >   
> >> > > I think that this should be fixed in addrconf_notify. It should lookup
> >> > > if there is a master on top and bail out in that case.  
> >> > 
> >> > There are other virtual interfaces that have a master assigned and want to
> >> > participate in IPv6 address configuration.  
> >> 
> >> Can you give me an example?  
> >
> >I would like to revisit this patch (yes, I know it has been a while).  I
> >believe the VRF implementation uses master to group the interfaces under
> >a single interface.
> >
> >I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
> >similar.  
> 
> Again, why do you need team port to have IFF_SLAVE flag? What do you
> want to achieve

Without setting this flag IPv6 will try and make a link specific address.

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-09-30  9:34             ` Jiri Pirko
@ 2018-10-01 14:06               ` Chas Williams
  2018-10-02 11:12                 ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Chas Williams @ 2018-10-01 14:06 UTC (permalink / raw)
  To: Jiri Pirko, Stephen Hemminger; +Cc: Jan Blunck, LKML, netdev



On 09/30/18 05:34, Jiri Pirko wrote:
> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>> On Sun, 30 Sep 2018 09:14:14 +0200
>> Jiri Pirko <jiri@resnulli.us> wrote:
>>
>>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>>>>
>>>>
>>>> On 07/10/15 02:41, Jiri Pirko wrote:
>>>>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>>>>>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>>>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>>>>>> decide if it should start the address configuration. Since team ports
>>>>>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>>>>>> a port to the team master.
>>>>>>>
>>>>>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>>>>>> not using that as well, for example bridge, ovs, etc.
>>>>>>>    
>>>>>>
>>>>>> Maybe they need to get fixed too. I've used that flag because it is
>>>>>> documented as
>>>>>> a "slave of a load balancer" which describes what a team port is.
>>>>>>
>>>>>>    
>>>>>>> I think that this should be fixed in addrconf_notify. It should lookup
>>>>>>> if there is a master on top and bail out in that case.
>>>>>>
>>>>>> There are other virtual interfaces that have a master assigned and want to
>>>>>> participate in IPv6 address configuration.
>>>>>
>>>>> Can you give me an example?
>>>>
>>>> I would like to revisit this patch (yes, I know it has been a while).  I
>>>> believe the VRF implementation uses master to group the interfaces under
>>>> a single interface.
>>>>
>>>> I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>>>> similar.
>>>
>>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>>> want to achieve
>>
>> Without setting this flag IPv6 will try and make a link specific address.
> 
> Why is it not an issue with bridge, ovs, and other master-slave devices?
> 

It very well might be an issue for bridge and ovs.  Other master-slave 
devices include the existing VRF implementation in the kernel and those 
slave interfaces will certainly want to use IPv6.

However, IFF_SLAVE has a specific meaning:

./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.

The bonding driver is not the only user:

./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) 
== IFF_SLAVE)
./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;

The team driver would like to use this same flag since it is a load 
balancer as well.  The side effect of not assigning IPv6 is a bonus. 
The fact that bridges and ovs are also likely broken is a different 
issue.  Should there be a another flag that says "layer 2 only"?  Very 
possibly, but that is something all these interfaces should be using to 
include bonding, team, eql, obs, bridge etc.  That's not a reasonable 
objection to labeling the team slave as slaves since they are literally 
slaves of a load balancer.




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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-10-01 14:06               ` Chas Williams
@ 2018-10-02 11:12                 ` Jiri Pirko
  2018-10-02 21:20                   ` Chas Williams
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2018-10-02 11:12 UTC (permalink / raw)
  To: Chas Williams; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev

Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>
>
>On 09/30/18 05:34, Jiri Pirko wrote:
>> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>> > On Sun, 30 Sep 2018 09:14:14 +0200
>> > Jiri Pirko <jiri@resnulli.us> wrote:
>> > 
>> > > Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>> > > > 
>> > > > 
>> > > > On 07/10/15 02:41, Jiri Pirko wrote:
>> > > > > Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>> > > > > > On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > > > > > Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>> > > > > > > > The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>> > > > > > > > decide if it should start the address configuration. Since team ports
>> > > > > > > > shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>> > > > > > > > a port to the team master.
>> > > > > > > 
>> > > > > > > I don't want to use IFF_SLAVE in team. Other master-slave devices are
>> > > > > > > not using that as well, for example bridge, ovs, etc.
>> > > > > > 
>> > > > > > Maybe they need to get fixed too. I've used that flag because it is
>> > > > > > documented as
>> > > > > > a "slave of a load balancer" which describes what a team port is.
>> > > > > > 
>> > > > > > > I think that this should be fixed in addrconf_notify. It should lookup
>> > > > > > > if there is a master on top and bail out in that case.
>> > > > > > 
>> > > > > > There are other virtual interfaces that have a master assigned and want to
>> > > > > > participate in IPv6 address configuration.
>> > > > > 
>> > > > > Can you give me an example?
>> > > > 
>> > > > I would like to revisit this patch (yes, I know it has been a while).  I
>> > > > believe the VRF implementation uses master to group the interfaces under
>> > > > a single interface.
>> > > > 
>> > > > I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>> > > > similar.
>> > > 
>> > > Again, why do you need team port to have IFF_SLAVE flag? What do you
>> > > want to achieve
>> > 
>> > Without setting this flag IPv6 will try and make a link specific address.

You are talking about addrconf_notify() right? Easy to fix to check
something more convenient. Like netif_is_lag_port() if you want to avoid
it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
of helpers to cover this.



>> 
>> Why is it not an issue with bridge, ovs, and other master-slave devices?
>> 
>
>It very well might be an issue for bridge and ovs.  Other master-slave
>devices include the existing VRF implementation in the kernel and those slave
>interfaces will certainly want to use IPv6.
>
>However, IFF_SLAVE has a specific meaning:
>
>./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.

I know that some userspace apps are using this flag to determine a
"bonding slave". I don't think that they care much about eql...


>
>The bonding driver is not the only user:
>
>./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) ==
>IFF_SLAVE)
>./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
>./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;
>
>The team driver would like to use this same flag since it is a load balancer
>as well.  The side effect of not assigning IPv6 is a bonus. The fact that

No, please leave IFF_SLAVE as it is. Both kernel and userspace have
their clear indications right now about the master/slave relationships.


>bridges and ovs are also likely broken is a different issue.  Should there be
>a another flag that says "layer 2 only"?  Very possibly, but that is
>something all these interfaces should be using to include bonding, team, eql,
>obs, bridge etc.  That's not a reasonable objection to labeling the team
>slave as slaves since they are literally slaves of a load balancer.
>
>
>

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-10-02 11:12                 ` Jiri Pirko
@ 2018-10-02 21:20                   ` Chas Williams
  2018-10-03 10:44                     ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Chas Williams @ 2018-10-02 21:20 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev



On 10/02/18 07:12, Jiri Pirko wrote:
> Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>>
>>
>> On 09/30/18 05:34, Jiri Pirko wrote:
>>> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>>>> On Sun, 30 Sep 2018 09:14:14 +0200
>>>> Jiri Pirko <jiri@resnulli.us> wrote:
>>>>
>>>>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>>>>>>
>>>>>>
>>>>>> On 07/10/15 02:41, Jiri Pirko wrote:
>>>>>>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>>>>>>>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>>>>>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>>>>>>>> decide if it should start the address configuration. Since team ports
>>>>>>>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>>>>>>>> a port to the team master.
>>>>>>>>>
>>>>>>>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>>>>>>>> not using that as well, for example bridge, ovs, etc.
>>>>>>>>
>>>>>>>> Maybe they need to get fixed too. I've used that flag because it is
>>>>>>>> documented as
>>>>>>>> a "slave of a load balancer" which describes what a team port is.
>>>>>>>>
>>>>>>>>> I think that this should be fixed in addrconf_notify. It should lookup
>>>>>>>>> if there is a master on top and bail out in that case.
>>>>>>>>
>>>>>>>> There are other virtual interfaces that have a master assigned and want to
>>>>>>>> participate in IPv6 address configuration.
>>>>>>>
>>>>>>> Can you give me an example?
>>>>>>
>>>>>> I would like to revisit this patch (yes, I know it has been a while).  I
>>>>>> believe the VRF implementation uses master to group the interfaces under
>>>>>> a single interface.
>>>>>>
>>>>>> I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>>>>>> similar.
>>>>>
>>>>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>>>>> want to achieve
>>>>
>>>> Without setting this flag IPv6 will try and make a link specific address.
> 
> You are talking about addrconf_notify() right? Easy to fix to check
> something more convenient. Like netif_is_lag_port() if you want to avoid
> it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
> of helpers to cover this.

OK, IPv6 should probably be using this.

> 
> 
> 
>>>
>>> Why is it not an issue with bridge, ovs, and other master-slave devices?
>>>
>>
>> It very well might be an issue for bridge and ovs.  Other master-slave
>> devices include the existing VRF implementation in the kernel and those slave
>> interfaces will certainly want to use IPv6.
>>
>> However, IFF_SLAVE has a specific meaning:
>>
>> ./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
> 
> I know that some userspace apps are using this flag to determine a
> "bonding slave". I don't think that they care much about eql...
> 
> 
>>
>> The bonding driver is not the only user:
>>
>> ./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) ==
>> IFF_SLAVE)
>> ./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
>> ./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;
>>
>> The team driver would like to use this same flag since it is a load balancer
>> as well.  The side effect of not assigning IPv6 is a bonus. The fact that
> 
> No, please leave IFF_SLAVE as it is. Both kernel and userspace have
> their clear indications right now about the master/slave relationships.

The team driver does create a master/slave relationship.  The team 
slaves are literally slaves of the master device.  It's not clear to me
why you we can't mark the slaves of the team master as actually being
slave interfaces?

> 
> 
>> bridges and ovs are also likely broken is a different issue.  Should there be
>> a another flag that says "layer 2 only"?  Very possibly, but that is
>> something all these interfaces should be using to include bonding, team, eql,
>> obs, bridge etc.  That's not a reasonable objection to labeling the team
>> slave as slaves since they are literally slaves of a load balancer.
>>
>>
>>

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-10-02 21:20                   ` Chas Williams
@ 2018-10-03 10:44                     ` Jiri Pirko
  2018-10-03 17:30                       ` Chas Williams
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2018-10-03 10:44 UTC (permalink / raw)
  To: Chas Williams; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev

Tue, Oct 02, 2018 at 11:20:25PM CEST, 3chas3@gmail.com wrote:
>
>
>On 10/02/18 07:12, Jiri Pirko wrote:
>> Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>> > 
>> > 
>> > On 09/30/18 05:34, Jiri Pirko wrote:
>> > > Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>> > > > On Sun, 30 Sep 2018 09:14:14 +0200
>> > > > Jiri Pirko <jiri@resnulli.us> wrote:
>> > > > 
>> > > > > Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>> > > > > > 
>> > > > > > 
>> > > > > > On 07/10/15 02:41, Jiri Pirko wrote:
>> > > > > > > Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>> > > > > > > > On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > > > > > > > Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>> > > > > > > > > > The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>> > > > > > > > > > decide if it should start the address configuration. Since team ports
>> > > > > > > > > > shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>> > > > > > > > > > a port to the team master.
>> > > > > > > > > 
>> > > > > > > > > I don't want to use IFF_SLAVE in team. Other master-slave devices are
>> > > > > > > > > not using that as well, for example bridge, ovs, etc.
>> > > > > > > > 
>> > > > > > > > Maybe they need to get fixed too. I've used that flag because it is
>> > > > > > > > documented as
>> > > > > > > > a "slave of a load balancer" which describes what a team port is.
>> > > > > > > > 
>> > > > > > > > > I think that this should be fixed in addrconf_notify. It should lookup
>> > > > > > > > > if there is a master on top and bail out in that case.
>> > > > > > > > 
>> > > > > > > > There are other virtual interfaces that have a master assigned and want to
>> > > > > > > > participate in IPv6 address configuration.
>> > > > > > > 
>> > > > > > > Can you give me an example?
>> > > > > > 
>> > > > > > I would like to revisit this patch (yes, I know it has been a while).  I
>> > > > > > believe the VRF implementation uses master to group the interfaces under
>> > > > > > a single interface.
>> > > > > > 
>> > > > > > I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>> > > > > > similar.
>> > > > > 
>> > > > > Again, why do you need team port to have IFF_SLAVE flag? What do you
>> > > > > want to achieve
>> > > > 
>> > > > Without setting this flag IPv6 will try and make a link specific address.
>> 
>> You are talking about addrconf_notify() right? Easy to fix to check
>> something more convenient. Like netif_is_lag_port() if you want to avoid
>> it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
>> of helpers to cover this.
>
>OK, IPv6 should probably be using this.
>
>> 
>> 
>> 
>> > > 
>> > > Why is it not an issue with bridge, ovs, and other master-slave devices?
>> > > 
>> > 
>> > It very well might be an issue for bridge and ovs.  Other master-slave
>> > devices include the existing VRF implementation in the kernel and those slave
>> > interfaces will certainly want to use IPv6.
>> > 
>> > However, IFF_SLAVE has a specific meaning:
>> > 
>> > ./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
>> 
>> I know that some userspace apps are using this flag to determine a
>> "bonding slave". I don't think that they care much about eql...
>> 
>> 
>> > 
>> > The bonding driver is not the only user:
>> > 
>> > ./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) ==
>> > IFF_SLAVE)
>> > ./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
>> > ./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;
>> > 
>> > The team driver would like to use this same flag since it is a load balancer
>> > as well.  The side effect of not assigning IPv6 is a bonus. The fact that
>> 
>> No, please leave IFF_SLAVE as it is. Both kernel and userspace have
>> their clear indications right now about the master/slave relationships.
>
>The team driver does create a master/slave relationship.  The team slaves are
>literally slaves of the master device.  It's not clear to me
>why you we can't mark the slaves of the team master as actually being
>slave interfaces?

So? IFF_SLAVE flag serves a different purpose. That's it. Team does not
need it, bridge does not need it, macvlan does not need it, etc.


>
>> 
>> 
>> > bridges and ovs are also likely broken is a different issue.  Should there be
>> > a another flag that says "layer 2 only"?  Very possibly, but that is
>> > something all these interfaces should be using to include bonding, team, eql,
>> > obs, bridge etc.  That's not a reasonable objection to labeling the team
>> > slave as slaves since they are literally slaves of a load balancer.
>> > 
>> > 
>> > 

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-10-03 10:44                     ` Jiri Pirko
@ 2018-10-03 17:30                       ` Chas Williams
  2018-10-05  6:46                         ` Jiri Pirko
  0 siblings, 1 reply; 15+ messages in thread
From: Chas Williams @ 2018-10-03 17:30 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev



On 10/03/18 06:44, Jiri Pirko wrote:
> Tue, Oct 02, 2018 at 11:20:25PM CEST, 3chas3@gmail.com wrote:
>>
>>
>> On 10/02/18 07:12, Jiri Pirko wrote:
>>> Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>>>>
>>>>
>>>> On 09/30/18 05:34, Jiri Pirko wrote:
>>>>> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>>>>>> On Sun, 30 Sep 2018 09:14:14 +0200
>>>>>> Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>
>>>>>>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 07/10/15 02:41, Jiri Pirko wrote:
>>>>>>>>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>>>>>>>>>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>>>>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>>>>>>>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>>>>>>>>>> decide if it should start the address configuration. Since team ports
>>>>>>>>>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>>>>>>>>>> a port to the team master.
>>>>>>>>>>>
>>>>>>>>>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>>>>>>>>>> not using that as well, for example bridge, ovs, etc.
>>>>>>>>>>
>>>>>>>>>> Maybe they need to get fixed too. I've used that flag because it is
>>>>>>>>>> documented as
>>>>>>>>>> a "slave of a load balancer" which describes what a team port is.
>>>>>>>>>>
>>>>>>>>>>> I think that this should be fixed in addrconf_notify. It should lookup
>>>>>>>>>>> if there is a master on top and bail out in that case.
>>>>>>>>>>
>>>>>>>>>> There are other virtual interfaces that have a master assigned and want to
>>>>>>>>>> participate in IPv6 address configuration.
>>>>>>>>>
>>>>>>>>> Can you give me an example?
>>>>>>>>
>>>>>>>> I would like to revisit this patch (yes, I know it has been a while).  I
>>>>>>>> believe the VRF implementation uses master to group the interfaces under
>>>>>>>> a single interface.
>>>>>>>>
>>>>>>>> I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>>>>>>>> similar.
>>>>>>>
>>>>>>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>>>>>>> want to achieve
>>>>>>
>>>>>> Without setting this flag IPv6 will try and make a link specific address.
>>>
>>> You are talking about addrconf_notify() right? Easy to fix to check
>>> something more convenient. Like netif_is_lag_port() if you want to avoid
>>> it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
>>> of helpers to cover this.
>>
>> OK, IPv6 should probably be using this.
>>
>>>
>>>
>>>
>>>>>
>>>>> Why is it not an issue with bridge, ovs, and other master-slave devices?
>>>>>
>>>>
>>>> It very well might be an issue for bridge and ovs.  Other master-slave
>>>> devices include the existing VRF implementation in the kernel and those slave
>>>> interfaces will certainly want to use IPv6.
>>>>
>>>> However, IFF_SLAVE has a specific meaning:
>>>>
>>>> ./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
>>>
>>> I know that some userspace apps are using this flag to determine a
>>> "bonding slave". I don't think that they care much about eql...
>>>
>>>
>>>>
>>>> The bonding driver is not the only user:
>>>>
>>>> ./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) ==
>>>> IFF_SLAVE)
>>>> ./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
>>>> ./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;
>>>>
>>>> The team driver would like to use this same flag since it is a load balancer
>>>> as well.  The side effect of not assigning IPv6 is a bonus. The fact that
>>>
>>> No, please leave IFF_SLAVE as it is. Both kernel and userspace have
>>> their clear indications right now about the master/slave relationships.
>>
>> The team driver does create a master/slave relationship.  The team slaves are
>> literally slaves of the master device.  It's not clear to me
>> why you we can't mark the slaves of the team master as actually being
>> slave interfaces?
> 
> So? IFF_SLAVE flag serves a different purpose. That's it. Team does not
> need it, bridge does not need it, macvlan does not need it, etc.

I agree.  But team *is* a load balancer.  Why can't team mark its slave 
interfaces as IFF_SLAVE?  They are literally slaves of a load balancer 
which is the exact meaning of the IFF_SLAVE flag.

> 
> 
>>
>>>
>>>
>>>> bridges and ovs are also likely broken is a different issue.  Should there be
>>>> a another flag that says "layer 2 only"?  Very possibly, but that is
>>>> something all these interfaces should be using to include bonding, team, eql,
>>>> obs, bridge etc.  That's not a reasonable objection to labeling the team
>>>> slave as slaves since they are literally slaves of a load balancer.
>>>>
>>>>
>>>>

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-10-03 17:30                       ` Chas Williams
@ 2018-10-05  6:46                         ` Jiri Pirko
  2018-10-06 13:28                           ` Chas Williams
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2018-10-05  6:46 UTC (permalink / raw)
  To: Chas Williams; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev

Wed, Oct 03, 2018 at 07:30:06PM CEST, 3chas3@gmail.com wrote:
>
>
>On 10/03/18 06:44, Jiri Pirko wrote:
>> Tue, Oct 02, 2018 at 11:20:25PM CEST, 3chas3@gmail.com wrote:
>> > 
>> > 
>> > On 10/02/18 07:12, Jiri Pirko wrote:
>> > > Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>> > > > 
>> > > > 
>> > > > On 09/30/18 05:34, Jiri Pirko wrote:
>> > > > > Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>> > > > > > On Sun, 30 Sep 2018 09:14:14 +0200
>> > > > > > Jiri Pirko <jiri@resnulli.us> wrote:
>> > > > > > 
>> > > > > > > Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>> > > > > > > > 
>> > > > > > > > 
>> > > > > > > > On 07/10/15 02:41, Jiri Pirko wrote:
>> > > > > > > > > Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>> > > > > > > > > > On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > > > > > > > > > Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>> > > > > > > > > > > > The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>> > > > > > > > > > > > decide if it should start the address configuration. Since team ports
>> > > > > > > > > > > > shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>> > > > > > > > > > > > a port to the team master.
>> > > > > > > > > > > 
>> > > > > > > > > > > I don't want to use IFF_SLAVE in team. Other master-slave devices are
>> > > > > > > > > > > not using that as well, for example bridge, ovs, etc.
>> > > > > > > > > > 
>> > > > > > > > > > Maybe they need to get fixed too. I've used that flag because it is
>> > > > > > > > > > documented as
>> > > > > > > > > > a "slave of a load balancer" which describes what a team port is.
>> > > > > > > > > > 
>> > > > > > > > > > > I think that this should be fixed in addrconf_notify. It should lookup
>> > > > > > > > > > > if there is a master on top and bail out in that case.
>> > > > > > > > > > 
>> > > > > > > > > > There are other virtual interfaces that have a master assigned and want to
>> > > > > > > > > > participate in IPv6 address configuration.
>> > > > > > > > > 
>> > > > > > > > > Can you give me an example?
>> > > > > > > > 
>> > > > > > > > I would like to revisit this patch (yes, I know it has been a while).  I
>> > > > > > > > believe the VRF implementation uses master to group the interfaces under
>> > > > > > > > a single interface.
>> > > > > > > > 
>> > > > > > > > I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>> > > > > > > > similar.
>> > > > > > > 
>> > > > > > > Again, why do you need team port to have IFF_SLAVE flag? What do you
>> > > > > > > want to achieve
>> > > > > > 
>> > > > > > Without setting this flag IPv6 will try and make a link specific address.
>> > > 
>> > > You are talking about addrconf_notify() right? Easy to fix to check
>> > > something more convenient. Like netif_is_lag_port() if you want to avoid
>> > > it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
>> > > of helpers to cover this.
>> > 
>> > OK, IPv6 should probably be using this.
>> > 
>> > > 
>> > > 
>> > > 
>> > > > > 
>> > > > > Why is it not an issue with bridge, ovs, and other master-slave devices?
>> > > > > 
>> > > > 
>> > > > It very well might be an issue for bridge and ovs.  Other master-slave
>> > > > devices include the existing VRF implementation in the kernel and those slave
>> > > > interfaces will certainly want to use IPv6.
>> > > > 
>> > > > However, IFF_SLAVE has a specific meaning:
>> > > > 
>> > > > ./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
>> > > 
>> > > I know that some userspace apps are using this flag to determine a
>> > > "bonding slave". I don't think that they care much about eql...
>> > > 
>> > > 
>> > > > 
>> > > > The bonding driver is not the only user:
>> > > > 
>> > > > ./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) ==
>> > > > IFF_SLAVE)
>> > > > ./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
>> > > > ./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;
>> > > > 
>> > > > The team driver would like to use this same flag since it is a load balancer
>> > > > as well.  The side effect of not assigning IPv6 is a bonus. The fact that
>> > > 
>> > > No, please leave IFF_SLAVE as it is. Both kernel and userspace have
>> > > their clear indications right now about the master/slave relationships.
>> > 
>> > The team driver does create a master/slave relationship.  The team slaves are
>> > literally slaves of the master device.  It's not clear to me
>> > why you we can't mark the slaves of the team master as actually being
>> > slave interfaces?
>> 
>> So? IFF_SLAVE flag serves a different purpose. That's it. Team does not
>> need it, bridge does not need it, macvlan does not need it, etc.
>
>I agree.  But team *is* a load balancer.  Why can't team mark its slave
>interfaces as IFF_SLAVE?  They are literally slaves of a load balancer which
>is the exact meaning of the IFF_SLAVE flag.

I described that multiple times, don't want to repeat myself. Please
read the thread again.

>
>> 
>> 
>> > 
>> > > 
>> > > 
>> > > > bridges and ovs are also likely broken is a different issue.  Should there be
>> > > > a another flag that says "layer 2 only"?  Very possibly, but that is
>> > > > something all these interfaces should be using to include bonding, team, eql,
>> > > > obs, bridge etc.  That's not a reasonable objection to labeling the team
>> > > > slave as slaves since they are literally slaves of a load balancer.
>> > > > 
>> > > > 
>> > > > 

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

* Re: [PATCH] team: set IFF_SLAVE on team ports
  2018-10-05  6:46                         ` Jiri Pirko
@ 2018-10-06 13:28                           ` Chas Williams
  0 siblings, 0 replies; 15+ messages in thread
From: Chas Williams @ 2018-10-06 13:28 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Stephen Hemminger, Jan Blunck, LKML, netdev



On 10/05/18 02:46, Jiri Pirko wrote:
> Wed, Oct 03, 2018 at 07:30:06PM CEST, 3chas3@gmail.com wrote:
>>
>>
>> On 10/03/18 06:44, Jiri Pirko wrote:
>>> Tue, Oct 02, 2018 at 11:20:25PM CEST, 3chas3@gmail.com wrote:
>>>>
>>>>
>>>> On 10/02/18 07:12, Jiri Pirko wrote:
>>>>> Mon, Oct 01, 2018 at 04:06:16PM CEST, 3chas3@gmail.com wrote:
>>>>>>
>>>>>>
>>>>>> On 09/30/18 05:34, Jiri Pirko wrote:
>>>>>>> Sun, Sep 30, 2018 at 11:38:05AM CEST, stephen@networkplumber.org wrote:
>>>>>>>> On Sun, 30 Sep 2018 09:14:14 +0200
>>>>>>>> Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>>>
>>>>>>>>> Thu, Sep 27, 2018 at 04:04:26PM CEST, 3chas3@gmail.com wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 07/10/15 02:41, Jiri Pirko wrote:
>>>>>>>>>>> Thu, Jul 09, 2015 at 05:36:55PM CEST, jblunck@infradead.org wrote:
>>>>>>>>>>>> On Thu, Jul 9, 2015 at 12:07 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>>>>>>>>>> Thu, Jul 09, 2015 at 11:58:34AM CEST, jblunck@infradead.org wrote:
>>>>>>>>>>>>>> The code in net/ipv6/addrconf.c:addrconf_notify() tests for IFF_SLAVE to
>>>>>>>>>>>>>> decide if it should start the address configuration. Since team ports
>>>>>>>>>>>>>> shouldn't get link-local addresses assigned lets set IFF_SLAVE when linking
>>>>>>>>>>>>>> a port to the team master.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't want to use IFF_SLAVE in team. Other master-slave devices are
>>>>>>>>>>>>> not using that as well, for example bridge, ovs, etc.
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe they need to get fixed too. I've used that flag because it is
>>>>>>>>>>>> documented as
>>>>>>>>>>>> a "slave of a load balancer" which describes what a team port is.
>>>>>>>>>>>>
>>>>>>>>>>>>> I think that this should be fixed in addrconf_notify. It should lookup
>>>>>>>>>>>>> if there is a master on top and bail out in that case.
>>>>>>>>>>>>
>>>>>>>>>>>> There are other virtual interfaces that have a master assigned and want to
>>>>>>>>>>>> participate in IPv6 address configuration.
>>>>>>>>>>>
>>>>>>>>>>> Can you give me an example?
>>>>>>>>>>
>>>>>>>>>> I would like to revisit this patch (yes, I know it has been a while).  I
>>>>>>>>>> believe the VRF implementation uses master to group the interfaces under
>>>>>>>>>> a single interface.
>>>>>>>>>>
>>>>>>>>>> I don't see a reason not to use IFF_SLAVE since team and bonding are fairly
>>>>>>>>>> similar.
>>>>>>>>>
>>>>>>>>> Again, why do you need team port to have IFF_SLAVE flag? What do you
>>>>>>>>> want to achieve
>>>>>>>>
>>>>>>>> Without setting this flag IPv6 will try and make a link specific address.
>>>>>
>>>>> You are talking about addrconf_notify() right? Easy to fix to check
>>>>> something more convenient. Like netif_is_lag_port() if you want to avoid
>>>>> it for bond/team. netif_is_ovs_port(), netif_is_bridge_port() etc. Lot's
>>>>> of helpers to cover this.
>>>>
>>>> OK, IPv6 should probably be using this.
>>>>
>>>>>
>>>>>
>>>>>
>>>>>>>
>>>>>>> Why is it not an issue with bridge, ovs, and other master-slave devices?
>>>>>>>
>>>>>>
>>>>>> It very well might be an issue for bridge and ovs.  Other master-slave
>>>>>> devices include the existing VRF implementation in the kernel and those slave
>>>>>> interfaces will certainly want to use IPv6.
>>>>>>
>>>>>> However, IFF_SLAVE has a specific meaning:
>>>>>>
>>>>>> ./include/uapi/linux/if.h: * @IFF_SLAVE: slave of a load balancer. Volatile.
>>>>>
>>>>> I know that some userspace apps are using this flag to determine a
>>>>> "bonding slave". I don't think that they care much about eql...
>>>>>
>>>>>
>>>>>>
>>>>>> The bonding driver is not the only user:
>>>>>>
>>>>>> ./drivers/net/eql.c:#define eql_is_slave(dev)	((dev->flags & IFF_SLAVE) ==
>>>>>> IFF_SLAVE)
>>>>>> ./drivers/net/eql.c:	slave->dev->flags &= ~IFF_SLAVE;
>>>>>> ./drivers/net/eql.c:		slave->dev->flags |= IFF_SLAVE;
>>>>>>
>>>>>> The team driver would like to use this same flag since it is a load balancer
>>>>>> as well.  The side effect of not assigning IPv6 is a bonus. The fact that
>>>>>
>>>>> No, please leave IFF_SLAVE as it is. Both kernel and userspace have
>>>>> their clear indications right now about the master/slave relationships.
>>>>
>>>> The team driver does create a master/slave relationship.  The team slaves are
>>>> literally slaves of the master device.  It's not clear to me
>>>> why you we can't mark the slaves of the team master as actually being
>>>> slave interfaces?
>>>
>>> So? IFF_SLAVE flag serves a different purpose. That's it. Team does not
>>> need it, bridge does not need it, macvlan does not need it, etc.
>>
>> I agree.  But team *is* a load balancer.  Why can't team mark its slave
>> interfaces as IFF_SLAVE?  They are literally slaves of a load balancer which
>> is the exact meaning of the IFF_SLAVE flag.
> 
> I described that multiple times, don't want to repeat myself. Please
> read the thread again.

I have read the flag and you never described what the flag is for.  The 
only vague mention is "to indicate a bonding slave".  A team slave is 
exactly the same thing as a bonding slave.  If there is some application
using IFF_SLAVE to find those slaves, it should worry about team slaves 
as well. Given that the eql driver is using this flag for the same 
purpose doesn't give bonding exclusive rights to use this flag with its 
interfaces.

> 
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> bridges and ovs are also likely broken is a different issue.  Should there be
>>>>>> a another flag that says "layer 2 only"?  Very possibly, but that is
>>>>>> something all these interfaces should be using to include bonding, team, eql,
>>>>>> obs, bridge etc.  That's not a reasonable objection to labeling the team
>>>>>> slave as slaves since they are literally slaves of a load balancer.
>>>>>>
>>>>>>
>>>>>>

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

end of thread, other threads:[~2018-10-06 13:28 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-09  9:58 [PATCH] team: set IFF_SLAVE on team ports Jan Blunck
2015-07-09 10:07 ` Jiri Pirko
2015-07-09 15:36   ` Jan Blunck
2015-07-10  6:41     ` Jiri Pirko
2018-09-27 14:04       ` Chas Williams
2018-09-30  7:14         ` Jiri Pirko
2018-09-30  9:38           ` Stephen Hemminger
2018-09-30  9:34             ` Jiri Pirko
2018-10-01 14:06               ` Chas Williams
2018-10-02 11:12                 ` Jiri Pirko
2018-10-02 21:20                   ` Chas Williams
2018-10-03 10:44                     ` Jiri Pirko
2018-10-03 17:30                       ` Chas Williams
2018-10-05  6:46                         ` Jiri Pirko
2018-10-06 13:28                           ` Chas Williams

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.