Date:	Fri, 5 Dec 2014 22:29:24 -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 5:04 PM, Arad, Ronen <ronen.arad@...el.com> wrote:
> 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.

Agreed, it would be nice to have it propagated down, but there is a
non-ideal work-around.  If you set default_pvid=0 to turn off PVID,
then the switch port driver can pick some internal VLAN ID just for HW
purposes in matching untagged pkts.  It's non-ideal because the switch
port driver needs to reserve a block of VLAN IDs for internal usage or
use some other matching mechanism to keep untagged pkts within this
bridge.

Better to have default_pvid value propagated down.  But, default_pvid
is a per-bridge property, not a per-bridge-port property.
RTM_SETLINK/RTM_GETLINK for PF_BRIDGE does have AFSPEC for per-bridge
and PROTINFO for per-bridge-port, so it seems PVID needs to be part of
AFSPEC.

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