Message-ID: <CAE4R7bAKTtWi6TbOaeNDxKkDFXf5H2E3NUdbNSizvq77ADtuNg@mail.gmail.com>
Date:	Fri, 5 Dec 2014 22:54:43 -0800
From:	Scott Feldman <sfeldma@...il.com>
To:	"Arad, Ronen" <ronen.arad@...el.com>
Cc:	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Netdev <netdev@...r.kernel.org>,
	Jirí Pírko <jiri@...nulli.us>,
	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

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