Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set

[Roopa Prabhu <roopa@cumulusnetworks.com>]

On 12/5/14, 3:21 PM, Arad, Ronen wrote:
>
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Roopa Prabhu
>> Sent: Thursday, December 04, 2014 11:02 PM
>> To: Scott Feldman
>> Cc: Jiří Pírko; Jamal Hadi Salim; Benjamin LaHaise; Thomas Graf; john
>> fastabend; stephen@networkplumber.org; John Linville;
>> nhorman@tuxdriver.com; Nicolas Dichtel; vyasevic@redhat.com; Florian
>> Fainelli; buytenh@wantstofly.org; Aviad Raveh; Netdev; David S. Miller;
>> shm@cumulusnetworks.com; Andy Gospodarek
>> Subject: Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic
>> if feature flag set
>>
>> On 12/4/14, 10:41 PM, Scott Feldman wrote:
>>> On Thu, Dec 4, 2014 at 6:26 PM,  <roopa@cumulusnetworks.com> wrote:
>>>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>>>
>>>> This allows offloading to switch asic without having the user to set
>>>> any flag. And this is done in the bridge driver to rollback kernel
>>>> settings on hw offload failure if required in the future.
>>>>
>>>> With this, it also makes sure a notification goes out only after the
>>>> attributes are set both in the kernel and hw.
>>> I like this approach as it streamlines the steps for the user in
>>> setting port flags.  There is one case for FLOODING where you'll have
>>> to turn off flooding for both, and then turn on flooding in hw.  You
>>> don't want flooding turned on on kernel and hw.
>> ok, maybe using the higher bits as in
>> https://patchwork.ozlabs.org/patch/413211/
>>
>> might help with that. Let me think some more.
>>>> ---
>>>>    net/bridge/br_netlink.c |   27 ++++++++++++++++++++++++++-
>>>>    1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index
>>>> 9f5eb55..ce173f0 100644
>>>> --- a/net/bridge/br_netlink.c
>>>> +++ b/net/bridge/br_netlink.c
>>>> @@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct
>> nlmsghdr *nlh)
>>>>                                   afspec, RTM_SETLINK);
>>>>           }
>>>>
>>>> +       if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) &&
>>>> +                       dev->netdev_ops->ndo_bridge_setlink) {
>>>> +               int ret = dev->netdev_ops->ndo_bridge_setlink(dev,
>>>> + nlh);
>>> I think you want to up-level this to net/core/rtnetlink.c because
>>> you're only enabling the feature for one instance of a driver that
>>> implements ndo_bridge_setlink: the bridge driver.  If another driver
>>> was MASTER and implemented ndo_bridge_setlink, you'd want same check
>>> to push setting down to SELF port driver.
>> yeah, i thought about that. But i moved it here so that rollback would be
>> easier.
> There is a need for propagating setlink/dellink requests down multiple levels.
> The use-case I have in mind is a bridge at the top, team/bond in the middle, and port devices at the bottom.
> A setlink for VLAN filtering attributes would come with MASTER flag set, and either port or bond/team netdev.
> How would this be handled?

Good point. glad that you brought this up.
>
> The propagation rules between bridge and enslaved port device could be different from those between bond/team and enslaved devices.
> The current bridge driver does not propagate VLAN filtering from bridge to its ports as each port could have different configuration. In a case of a bond/team all members need to have the same configuration such that the a bond/team would be indistinguishable from a simple port.
>
> Therefore rtnetlink.c might not have the knowledge for propagation across multiple levels.
> It seems that each device which implements ndo_bridge_setlink/ndo_bridge_dellink  and could have master role, need to take care of propagation to its slaves.

Are you suggesting all devices in such stack will implement 
ndo_bridge_setlink/ndo_bridge_dellink ?.
If yes,  Then that will not scale in terms of supporting all devices.

I am thinking an ndo op will not help here. On our systems, for such 
stacked devices, the switch driver is aware of all interfaces it cares 
about. In this case since the bond slaves are switch ports, it tracks 
the bond too. When the bond goes into the bridge, it tracks the bond as 
a bridge port.
Which means it uses any information a bridge driver calls on its port 
via ndo_bridge_setlink(). the port can be a bond or any device as long 
as the bridge port leads to a switch port.

As long as the bond driver does not need to do anything in software for 
this when its a bridge port, seems like the bridge driver should notify 
any switch devices that are interested in this setlink attributes (This 
includes the vlan information that you talk about).
To me this calls for switch device ops..... to go to the switch driver 
directly (context for this is my short talk at the BOF at LPC dusseldorf).
These swdev ops could be registered by the switch driver for all devices 
and at the time of the callback the switch driver can decide to service 
it or not, OR it could be registered only on devices the switch driver 
is interested in. (In this case when the bond becomes part of the bridge )

Will think about this some more, and will work on some RFC patches for 
this case.

Thanks,
Roopa