From: Arkadi Sharshevsky <arkadis@mellanox.com>
To: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>,
Jiri Pirko <jiri@resnulli.us>,
netdev@vger.kernel.org
Cc: davem@davemloft.net, idosch@mellanox.com, mlxsw@mellanox.com,
roopa@cumulusnetworks.com, stephen@networkplumber.org,
ivecera@redhat.com
Subject: Re: [patch net-next 02/19] net: bridge: Add support for offloading port attributes
Date: Tue, 6 Jun 2017 16:08:56 +0300 [thread overview]
Message-ID: <8625cccf-c274-9ffd-eeb2-85b2c4336c65@mellanox.com> (raw)
In-Reply-To: <fc8c1046-d567-e668-ef14-618814af36db@cumulusnetworks.com>
On 06/05/2017 04:29 PM, Nikolay Aleksandrov wrote:
> On 05/06/17 12:20, Jiri Pirko wrote:
>> From: Arkadi Sharshevsky <arkadis@mellanox.com>
>>
>> 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 <arkadis@mellanox.com>
>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> 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
>
Hi Nikolay,
Thank you very much for the review, will fix.
>> 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 <net/net_namespace.h>
>> #include <net/sock.h>
>> #include <uapi/linux/if_bridge.h>
>> +#include <net/switchdev.h>
>>
>> #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;
>>
>
next prev parent reply other threads:[~2017-06-06 13:09 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-05 9:20 [patch net-next 00/19] Remove support from bridge bypass for mlxsw/rocker drivers Jiri Pirko
2017-06-05 9:20 ` [patch net-next 01/19] net: switchdev: Add support for querying supported bridge flags by hardware Jiri Pirko
2017-06-05 14:00 ` Ivan Vecera
2017-06-05 9:20 ` [patch net-next 02/19] net: bridge: Add support for offloading port attributes Jiri Pirko
2017-06-05 13:29 ` Nikolay Aleksandrov
2017-06-06 13:08 ` Arkadi Sharshevsky [this message]
2017-06-05 9:20 ` [patch net-next 03/19] net: bridge: Add support for calling FDB external learning under rcu Jiri Pirko
2017-06-05 13:36 ` Nikolay Aleksandrov
2017-06-05 9:20 ` [patch net-next 04/19] net: switchdev: Change notifier chain to be atomic Jiri Pirko
2017-06-05 13:59 ` Ivan Vecera
2017-06-05 9:20 ` [patch net-next 05/19] net: bridge: Add support for notifying devices about FDB add/del Jiri Pirko
2017-06-05 13:35 ` Nikolay Aleksandrov
2017-06-06 14:19 ` Arkadi Sharshevsky
2017-06-05 9:20 ` [patch net-next 06/19] net: bridge: Receive notification about successful FDB offload Jiri Pirko
2017-06-05 13:44 ` Nikolay Aleksandrov
2017-06-05 9:20 ` [patch net-next 07/19] mlxsw: spectrum: Remove support for bridge FDB learning sync Jiri Pirko
2017-06-05 9:20 ` [patch net-next 08/19] mlxsw: spectrum_switchdev: Add support for querying supported bridge flags Jiri Pirko
2017-06-05 9:20 ` [patch net-next 09/19] mlxsw: spectrum: Remove support for bypass bridge port attributes/vlan set Jiri Pirko
2017-06-05 9:20 ` [patch net-next 10/19] mlxsw: spectrum_switchdev: Change switchdev notifier API Jiri Pirko
2017-06-05 9:20 ` [patch net-next 11/19] mlxsw: spectrum_switchdev: Add support for learning FDB through notification Jiri Pirko
2017-06-05 9:20 ` [patch net-next 12/19] mlxsw: spectrum: Remove support for bridge bypass FDB add/del Jiri Pirko
2017-06-05 9:20 ` [patch net-next 13/19] net: Remove support for bridge bypass ndos from stacked devices Jiri Pirko
2017-06-05 9:20 ` [patch net-next 14/19] rocker: Remove support for bridge FDB learning sync Jiri Pirko
2017-06-05 9:20 ` [patch net-next 15/19] rocker: Add support for querying supported bridge flags Jiri Pirko
2017-06-05 9:20 ` [patch net-next 16/19] rocker: Change world_ops API and implementation to be switchdev independant Jiri Pirko
2017-06-05 9:20 ` [patch net-next 17/19] rocker: Add support for learning FDB through notification Jiri Pirko
2017-06-05 9:20 ` [patch net-next 18/19] rocker: Remove support for bypass bridge port attributes/vlan set Jiri Pirko
2017-06-05 9:20 ` [patch net-next 19/19] rocker: Remove support bridge bypass FDB Jiri Pirko
2017-06-05 10:07 ` [patch iproute2] bridge: Distinguish between externally learned vs offloaded FDBs Jiri Pirko
2017-06-14 16:52 ` Stephen Hemminger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8625cccf-c274-9ffd-eeb2-85b2c4336c65@mellanox.com \
--to=arkadis@mellanox.com \
--cc=davem@davemloft.net \
--cc=idosch@mellanox.com \
--cc=ivecera@redhat.com \
--cc=jiri@resnulli.us \
--cc=mlxsw@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=nikolay@cumulusnetworks.com \
--cc=roopa@cumulusnetworks.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.