Date:	Mon, 8 Dec 2014 12:14:04 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:	Scott Feldman <sfeldma@...il.com>,
	"Arad, Ronen" <ronen.arad@...el.com>,
	Netdev <netdev@...r.kernel.org>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Benjamin LaHaise <bcrl@...ck.org>, Thomas Graf <tgraf@...g.ch>,
	john fastabend <john.fastabend@...il.com>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	John Linville <linville@...driver.com>,
	"nhorman@...driver.com" <nhorman@...driver.com>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>,
	"vyasevic@...hat.com" <vyasevic@...hat.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"buytenh@...tstofly.org" <buytenh@...tstofly.org>,
	Aviad Raveh <aviadr@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>,
	"shm@...ulusnetworks.com" <shm@...ulusnetworks.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>
Subject: Re: [PATCH 2/3] bridge: offload bridge port attributes to switch
 asic if feature flag set

Sun, Dec 07, 2014 at 09:24:19PM CET, roopa@...ulusnetworks.com wrote:
>On 12/5/14, 10:54 PM, Scott Feldman wrote:
>>On Fri, Dec 5, 2014 at 3:21 PM, Arad, Ronen <ronen.arad@...el.com> wrote:
>>>
>>>>-----Original Message-----
>>>>From: netdev-owner@...r.kernel.org [mailto:netdev-
>>>>owner@...r.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@...workplumber.org; John Linville;
>>>>nhorman@...driver.com; Nicolas Dichtel; vyasevic@...hat.com; Florian
>>>>Fainelli; buytenh@...tstofly.org; Aviad Raveh; Netdev; David S. Miller;
>>>>shm@...ulusnetworks.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@...ulusnetworks.com> wrote:
>>>>>>From: Roopa Prabhu <roopa@...ulusnetworks.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.

I think that this should be handled similar to ndo_vlan_rx_add_vid,
ndo_vlan_rx_kill_vid, ndo_change_mtu and others. Master devices like
bridge, bond, team, etc should take care of propagating the calls to
lower devices. It mignt not make sense sometimes so let the masters to
decide.

I think that the feature bit (ethtool flag) should serve only for user
to actually enable or disable the offload. And thinking about that,
maybe the bit checking should be implemented in switch drivers, not in
bridge and friends.


--
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