Messages in this thread |  | | Subject | Re: [PATCH] team: set IFF_SLAVE on team ports | From | Chas Williams <> | Date | Wed, 3 Oct 2018 13:30:06 -0400 |
| |
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. >>>> >>>> >>>>
|  |