Message-ID: <E4CD12F19ABA0C4D8729E087A761DC3505D8436D@ORSMSX101.amr.corp.intel.com>
Date:	Sat, 6 Dec 2014 01:04:09 +0000
From:	"Arad, Ronen" <ronen.arad@...el.com>
To:	"Arad, Ronen" <ronen.arad@...el.com>,
	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

I have another case of propagation which is not covered by the proposed patch.
A recent patch introduced default_pvid attribute for a bridge (so far supported only via sysfs and not via netlink).
When a port joins a bridge, it inherits a PVID from the default_pvid of the bridge.
The bridge driver propagates that to the newly created net_bridge_port. This is done in br_vlan.c:

int nbp_vlan_init(struct net_bridge_port *p)
{
	int rc = 0;

	if (p->br->default_pvid) {
		rc = nbp_vlan_add(p, p->br->default_pvid,
				  BRIDGE_VLAN_INFO_PVID |
				  BRIDGE_VLAN_INFO_UNTAGGED);
	}

	return rc;
}

When L2 switching is offloaded to the HW, this PVID setting need to be propagated. However, it does not come via ndo_bridge_setlink. The proposed propagation at br_setlink or an up level one at rtnetlink are not capable of handling this case.
One possible way for handling that is to replace the call to nbp_vlan_add with a call to a new function let's say
int br_propagate_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
This function will compose a netlink message with VLAN filtering information (i.e. AF_SPEC with VLAN_INFO) and call br_setlink - leveraging the offload support proposed by Roopa.

If this is an acceptable course of action, I could work on such patch.


> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-
> owner@...r.kernel.org] On Behalf Of Arad, Ronen
> Sent: Friday, December 05, 2014 3:21 PM
> To: Roopa Prabhu; Scott Feldman; Netdev
> Cc: 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
> 
> 
> 
> > -----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.
> 
> > >
> > >> +               if (ret && ret != -EOPNOTSUPP) {
> > >> +                       /* XXX Fix this in the future to rollback
> > >> +                        * kernel settings and return error
> > >> +                        */
> > > The future is now.  Let's fix this now for the rollback case (again
> > > up in rtnetlink.c).  So then a general question comes to mind: for
> > > these dual target sets, is it best to try HW first and then SW, or
> > > the other way around?  Either way, on failure on second you need to
> > > rollback first.  And, on failure, you need to know rollback value
> > > for first, so you have to do a getlink on first before attempting set.
> > yep, exactly, I went through the same thought process yesterday when i
> > was trying to implement rollback.
> > >
> > >> +                       br_warn(p->br, "error offloading bridge attributes "
> > >> +                                       "on port %u(%s)\n", (unsigned int) p->port_no,
> > >> +                                       p->dev->name);
> > >> +               }
> > >> +       }
> > >> +
> > >>          if (err == 0)
> > >>                  br_ifinfo_notify(RTM_NEWLINK, p);
> > >> -
> > >>   out:
> > >>          return err;
> > >>   }
> > >> @@ -433,6 +445,19 @@ int br_dellink(struct net_device *dev, struct
> > nlmsghdr *nlh)
> > >>          err = br_afspec((struct net_bridge *)netdev_priv(dev), p,
> > >>                          afspec, RTM_DELLINK);
> > >>
> > >> +       if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD
> > >> +                       && dev->netdev_ops->ndo_bridge_setlink) {
> > >> +               int ret = dev->netdev_ops->ndo_bridge_dellink(dev, nlh);
> > >> +               if (ret && ret != -EOPNOTSUPP) {
> > >> +                       /* XXX Fix this in the future to rollback
> > >> +                        * kernel settings and return error
> > >> +                        */
> > >> +                       br_warn(p->br, "error offloading bridge attributes "
> > >> +                                       "on port %u(%s)\n", (unsigned int) p->port_no,
> > >> +                                       p->dev->name);
> > >> +               }
> > >> +       }
> > >> +
> > > Same comments as setlink above.
> > >
> > >>          return err;
> > >>   }
> > >>   static int br_validate(struct nlattr *tb[], struct nlattr
> > >> *data[])
> > >> --
> > >> 1.7.10.4
> > >>
> >
> > --
> > 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
> . {.n +       +%  lzwm  b 맲  r  zw u   ^n r   z .  h    &  . G   h .( 階 ݢj"  . .m     z ޖ
> f   h   ~ m

Powered by blists - more mailing lists