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, 10:54 PM, Scott Feldman wrote:
> On Fri, Dec 5, 2014 at 3:21 PM, Arad, Ronen <ronen.arad@intel.com> 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?
>>
>> 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.
> Thanks Ronen for bringing up this use-case of stacked masters.  I
> think for VLAN filtering, the stacked master case is handled, not by
> ndo_setlink/dellink at each level, but with ndo_vlan_rx_kill_vid and
> ndo_vlan_rx_add_vid.  So the switch port driver can know VLAN
> membership for port even if port is under bond which is under bridge,
> by using ndo_vlan_rx_xxx and setting NETIF_F_HW_VLAN_CTAG_FILTER.  The
> bonding driver's ndo_vlan_rx_xxx handlers seem to keep ports in bond
> VLAN membership consistent across bond.
>
> But in general, ndo_setlink/dellink don't work for the stack use-case
> for other non-VLAN attributes.  Maybe the answer is to use the VLAN
> propogation model for other attributes.  ndo_setlink/dellink/getlink
> have enough weird-isms it might be time to define cleaner ndo ops to
> propagate the other attrs down.
And, only the switch asic driver is interested in these attrs. So, seems 
like for these cases, we need to send these attrs to the switchdriver 
directly instead of going through the stack of netdevs ?. see my 
response to ronen's other email.

Thanks,
Roopa