Message-ID: <E4CD12F19ABA0C4D8729E087A761DC3505D864BB@ORSMSX101.amr.corp.intel.com>
Date:	Mon, 8 Dec 2014 18:40:52 +0000
From:	"Arad, Ronen" <ronen.arad@...el.com>
To:	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Scott Feldman <sfeldma@...il.com>,
	Netdev <netdev@...r.kernel.org>
CC:	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



> -----Original Message-----
> From: Roopa Prabhu [mailto:roopa@...ulusnetworks.com]
> Sent: Sunday, December 07, 2014 12:24 PM
> To: Scott Feldman
> Cc: Arad, Ronen; Netdev; Jirí 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; 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/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.
This could have worked if the entire VLAN filtering policy was propagated. Currently only VLAN membership is propagated using ndo_vlan_rx_add_vid.
Missing are untagged flag for egress and PVID flag.
> >
> > 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

Powered by blists - more mailing lists