Message-ID: <55501BF1.10202@cumulusnetworks.com>
Date:	Sun, 10 May 2015 20:03:13 -0700
From:	roopa <roopa@...ulusnetworks.com>
To:	Scott Feldman <sfeldma@...il.com>
CC:	Jiri Pirko <jiri@...nulli.us>, Netdev <netdev@...r.kernel.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Florian Fainelli <f.fainelli@...il.com>,
	"andrew@...n.ch" <andrew@...n.ch>,
	"simon.horman@...ronome.com" <simon.horman@...ronome.com>,
	Joe Perches <joe@...ches.com>,
	"Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
	"Arad, Ronen" <ronen.arad@...el.com>
Subject: Re: [PATCH net-next v6 14/23] bridge: restore br_setlink back to
 original

On 5/10/15, 7:46 PM, Scott Feldman wrote:
> On Sun, May 10, 2015 at 5:55 PM, roopa <roopa@...ulusnetworks.com> wrote:
>> On 5/10/15, 4:55 PM, Scott Feldman wrote:
>>> On Sun, May 10, 2015 at 9:10 AM, roopa <roopa@...ulusnetworks.com> wrote:
>>>> On 5/9/15, 12:00 PM, Jiri Pirko wrote:
>>>>> Sat, May 09, 2015 at 07:40:16PM CEST, sfeldma@...il.com wrote:
>>>>>> From: Scott Feldman <sfeldma@...il.com>
>>>>>>
>>>>>> Restore br_setlink back to original and don't call into SELF port
>>>>>> driver.
>>>>>> rtnetlink.c:bridge_setlink() already does a call into port driver for
>>>>>> SELF.
>>>>>>
>>>>>> bridge set link cmd defaults to MASTER.  From man page for bridge link
>>>>>> set
>>>>>> cmd:
>>>>>>
>>>>>>          self   link setting is configured on specified physical device
>>>>>>
>>>>>>          master link setting is configured on the software bridge
>>>>>> (default)
>>>>>>
>>>>>> The link setting has two values: the device-side value and the software
>>>>>> bridge-side value.  These are independent and settable using the bridge
>>>>>> link set cmd by specifying some combination of [master] | [self].
>>>>>> Futhermore, the device-side and bridge-side settings have their own
>>>>>> initial
>>>>>> value, viewable from bridge -d link show cmd.
>>>>>>
>>>>>> Restoring br_setlink back to original makes rocker (the only in-kernel
>>>>>> user
>>>>>> of SELF link settings) work as first implement: two-sided values.
>>>>>>
>>>>>> It's true that when both MASTER and SELF are specified from the
>>>>>> command,
>>>>>> two netlink notifications are generated, one for each side of the
>>>>>> settings.
>>>>>> The user-space app can distiquish between the two notifications by
>>>>>> observing the MASTER or SELF flag.
>>>>> This is revert of:
>>>>>
>>>>> commit 68e331c785b85b78f4155e2ab6f90e976b609dc1
>>>>> Author: Roopa Prabhu <roopa@...ulusnetworks.com>
>>>>> Date:   Thu Jan 29 22:40:14 2015 -0800
>>>>>
>>>>>        bridge: offload bridge port attributes to switch asic if feature
>>>>> flag
>>>>> set
>>>>>
>>>>> Noting that because I want to make sure everybody is ok with new
>>>>> behaviour. I tend to like it more.
>>>>>
>>>> I am not ok with it. I have raised this earlier. same argument as the fib
>>>> code, app now has to remember to call with both master and self. I do
>>>> however feel that this code needs some rework..,.add to hardware first
>>>> and
>>>> then software
>>>> just like fib and rollback hardware on failure. In which case, i am ok
>>>> with
>>>> submitting a new patch to do it differently.
>>> The problem with your patch to br_setlink/br_dellink is it hard-coded
>>> a default in the kernel bridge driver, inconsistent with the default
>>> of the application (iproute2/bridge).  Reverting it keeps the kernel
>>> out of the default decision and lets the application define a default
>>> that suits it.
>> sorry, I am not understanding how this is different from how we do offload
>> for fib and stp.
>> just like stp offload from the bridge driver, i am hoping we can also
>> offload vlans (current patch under discussion)
>> and fdb entries. The switch driver can decide if it is only interested in
>> calls with flags set to 'self' (rocker for example).
>>
>> another example: mstp daemon running in userspace will use bridge setlink to
>> propagate stp states to the in-kernel bridge driver. If the current patch is
>> removed, mstp daemon will now have to make sure it calls bridge setlink with
>> self and master flags to hit both the bridge driver and hardware.
> You're making my point with this example: let the application set the
> flags it wants, and let the kernel provide the mechanism.
>
> The kernel mechanism for both FDB (rtnl_fdb_add) and bridge settings
> (rtnl_bridge_setlink) are the same: they both have this basic logic to
> handle MASTER and SELF flags:
>
> if (!flags || flags & MASTER)
>      if (master->ops->ndo_xxx)
>          master->ops->ndo_xxx(...);
> if (flags & SELF)
>      if (port->ops->ndo_xxx)
>          port->ops->ndo_xxx(...);
>
> That's all the kernel should do.  What is the default flags when
> explicit flags aren't specified by the user is up to the application.
> Current example apps:
>
> iproute2/bridge/fbd app passes SELF when no flags are given by user.
> iproute2/bridge/vlan app passes no flags when no flags are given by user.
> iproute2/bridge/link set app passes SELF if setting hwmode, otherwise
> passes no flags when no flags are given by user.
> mstp app passes no flags (I assume, based on what you wrote above).
>
> So if you want different app defaults than above, we need to change
> the app, not the kernel.
yes, I understand your point... and this is where we have a difference 
of opinion. I have only been saying that
the kernel defaults should be in favor of existing apps (for easier 
migration of these
apps to switch-devices).
>
> (FIB isn't part of this discussion because there is (currently) no
> MASTER|SELF flags for FIB entries, so I'm not sure why you're bringing
> up FIB).
>
I brought up FIB because you and I had a similar thread last week for 
FIB on the use of RTNH_F_EXTERNAL
by apps. RTNH_F_EXTERNAL is analogous to NTF_SELF in the FIB context IMO.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists