From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [patch net-next 02/19] net: bridge: Add support for offloading port attributes Date: Mon, 5 Jun 2017 16:29:26 +0300 Message-ID: References: <20170605092043.3523-1-jiri@resnulli.us> <20170605092043.3523-3-jiri@resnulli.us> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, idosch@mellanox.com, arkadis@mellanox.com, mlxsw@mellanox.com, roopa@cumulusnetworks.com, stephen@networkplumber.org, ivecera@redhat.com To: Jiri Pirko , netdev@vger.kernel.org Return-path: Received: from mail-wm0-f53.google.com ([74.125.82.53]:38545 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751280AbdFEN3e (ORCPT ); Mon, 5 Jun 2017 09:29:34 -0400 Received: by mail-wm0-f53.google.com with SMTP id n195so75888000wmg.1 for ; Mon, 05 Jun 2017 06:29:28 -0700 (PDT) In-Reply-To: <20170605092043.3523-3-jiri@resnulli.us> Sender: netdev-owner@vger.kernel.org List-ID: On 05/06/17 12:20, Jiri Pirko wrote: > From: Arkadi Sharshevsky > > Currently the flood, learning and learning_sync port attributes are > offloaded by setting the SELF flag. Add support for offloading the > flood and learning attribute through the bridge code. In case of > setting an unsupported flag on a offloded port the operation will > fail. > > The learning_sync attribute doesn't have any software representation > and cannot be offloaded through the bridge code. > > Signed-off-by: Arkadi Sharshevsky > Reviewed-by: Ido Schimmel > Signed-off-by: Jiri Pirko > --- > net/bridge/br_netlink.c | 112 +++++++++++++++++++++++++++++++++++++++--------- > net/bridge/br_private.h | 4 ++ > 2 files changed, 96 insertions(+), 20 deletions(-) > Hi Arkadi, A few comments below > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 1e63ec4..1afafb7 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > #include "br_private.h" > #include "br_private_stp.h" > @@ -662,16 +663,52 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state) > } > > /* Set/clear or port flags based on attribute */ > -static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], > - int attrtype, unsigned long mask) > +static int br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], > + int attrtype, unsigned long mask) > { > - if (tb[attrtype]) { > - u8 flag = nla_get_u8(tb[attrtype]); > - if (flag) > - p->flags |= mask; > - else > - p->flags &= ~mask; > + struct switchdev_attr attr = { > + .orig_dev = p->dev, > + .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT, > + }; > + unsigned long flags; > + int err; > + > + if (!tb[attrtype]) > + return 0; > + > + if (nla_get_u8(tb[attrtype])) > + flags = p->flags | mask; > + else > + flags = p->flags & ~mask; > + > + if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD) > + goto out; > + > + err = switchdev_port_attr_get(p->dev, &attr); > + if (err == -EOPNOTSUPP) > + goto out; > + if (err) > + return err; > + > + /* Check if specific bridge flag attribute offload is supported */ > + if (!(attr.u.brport_flags_support & mask)) { > + br_warn(p->br, "bridge flag offload is not supported %u(%s)\n", > + (unsigned int)p->port_no, p->dev->name); > + return -EOPNOTSUPP; > + } > + > + attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS; > + attr.flags = SWITCHDEV_F_DEFER; > + attr.u.brport_flags = flags; > + err = switchdev_port_attr_set(p->dev, &attr); > + if (err) { > + br_warn(p->br, "error setting offload FLAG on port %u(%s)\n", Why all caps (FLAG) ? > + (unsigned int)p->port_no, p->dev->name); > + return err; > } I think all of this switchdev-specific code should be contained into br_switchdev.c and exported via some function. Anyone changing only the bridge can easily get confused. > +out: > + p->flags = flags; > + return 0; > } > > /* Process bridge protocol info on port */ > @@ -681,20 +718,55 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[]) > bool br_vlan_tunnel_old = false; > int err; > > - br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); > - br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); > - br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, BR_MULTICAST_FAST_LEAVE); > - br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK); > - br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING); > - br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD); > - br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD); > - br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST, BR_MULTICAST_TO_UNICAST); > - br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD); > - br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP); > - br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI); > + err = br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, BR_MULTICAST_FAST_LEAVE); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST, BR_MULTICAST_TO_UNICAST); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP); > + if (err) > + return err; > + > + err = br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI); > + if (err) > + return err; > > br_vlan_tunnel_old = (p->flags & BR_VLAN_TUNNEL) ? true : false; > - br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); > + err = br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); > + if (err) > + return err; > + > if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL)) > nbp_vlan_tunnel_info_flush(p); > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 2062692..5dc30ed 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -42,6 +42,10 @@ > /* Path to usermode spanning tree program */ > #define BR_STP_PROG "/sbin/bridge-stp" > > +/* Flags that can be offloaded to hardware */ > +#define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ > + BR_MCAST_FLOOD | BR_BCAST_FLOOD) > + > typedef struct bridge_id bridge_id; > typedef struct mac_addr mac_addr; > typedef __u16 port_id; >