| Message ID | 20250204194921.46692-13-ericwouds@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bridge-fastpath and related improvements | expand |
On 2/4/25 21:49, Eric Woudstra wrote: > In network setup as below: > > fastpath bypass > .----------------------------------------. > / \ > | IP - forwarding | > | / \ v > | / wan ... > | / > | | > | | > | brlan.1 > | | > | +-------------------------------+ > | | vlan 1 | > | | | > | | brlan (vlan-filtering) | > | | +---------------+ > | | | DSA-SWITCH | > | | vlan 1 | | > | | to | | > | | untagged 1 vlan 1 | > | +---------------+---------------+ > . / \ > ----->wlan1 lan0 > . . > . ^ > ^ vlan 1 tagged packets > untagged packets > > br_vlan_fill_forward_path_mode() sets DEV_PATH_BR_VLAN_UNTAG_HW when > filling in from brlan.1 towards wlan1. But it should be set to > DEV_PATH_BR_VLAN_UNTAG in this case. Using BR_VLFLAG_ADDED_BY_SWITCHDEV > is not correct. The dsa switchdev adds it as a foreign port. > > The same problem for all foreignly added dsa vlans on the bridge. > > First add the vlan, trying only native devices. > If this fails, we know this may be a vlan from a foreign device. > > Use BR_VLFLAG_TAGGING_BY_SWITCHDEV to make sure DEV_PATH_BR_VLAN_UNTAG_HW > is set only when there if no foreign device involved. > > Signed-off-by: Eric Woudstra <ericwouds@gmail.com> > --- > include/net/switchdev.h | 1 + > net/bridge/br_private.h | 10 ++++++++++ > net/bridge/br_switchdev.c | 15 +++++++++++++++ > net/bridge/br_vlan.c | 7 ++++++- > net/switchdev/switchdev.c | 2 +- > 5 files changed, 33 insertions(+), 2 deletions(-) > Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
On Tue, Feb 04, 2025 at 08:49:19PM +0100, Eric Woudstra wrote: > In network setup as below: > > fastpath bypass > .----------------------------------------. > / \ > | IP - forwarding | > | / \ v > | / wan ... > | / > | | > | | > | brlan.1 > | | > | +-------------------------------+ > | | vlan 1 | > | | | > | | brlan (vlan-filtering) | > | | +---------------+ > | | | DSA-SWITCH | > | | vlan 1 | | > | | to | | > | | untagged 1 vlan 1 | > | +---------------+---------------+ > . / \ > ----->wlan1 lan0 > . . > . ^ > ^ vlan 1 tagged packets > untagged packets > > br_vlan_fill_forward_path_mode() sets DEV_PATH_BR_VLAN_UNTAG_HW when > filling in from brlan.1 towards wlan1. But it should be set to > DEV_PATH_BR_VLAN_UNTAG in this case. Using BR_VLFLAG_ADDED_BY_SWITCHDEV > is not correct. The dsa switchdev adds it as a foreign port. > > The same problem for all foreignly added dsa vlans on the bridge. > > First add the vlan, trying only native devices. > If this fails, we know this may be a vlan from a foreign device. > > Use BR_VLFLAG_TAGGING_BY_SWITCHDEV to make sure DEV_PATH_BR_VLAN_UNTAG_HW > is set only when there if no foreign device involved. > > Signed-off-by: Eric Woudstra <ericwouds@gmail.com> > --- Shouldn't mlxsw_sp_switchdev_vxlan_vlans_add() also respect the SWITCHDEV_F_NO_FOREIGN flag? My (maybe incorrect) understanding of bridging topologies with vxlan and mlxsw is that they are neighbor bridge ports, and mlxsw doesn't (seem to) call switchdev_bridge_port_offload() for the vxlan bridge port. This technically makes vxlan a foreign bridge port to mlxsw, so it should skip reacting on VLAN switchdev objects when that flag is set, just for uniform behavior across the board. (your patch repeats the notifier without the SWITCHDEV_F_NO_FOREIGN flag anyway, so it only matters for flowtable offload).
On 2/7/25 4:03 PM, Vladimir Oltean wrote: > On Tue, Feb 04, 2025 at 08:49:19PM +0100, Eric Woudstra wrote: >> In network setup as below: >> >> fastpath bypass >> .----------------------------------------. >> / \ >> | IP - forwarding | >> | / \ v >> | / wan ... >> | / >> | | >> | | >> | brlan.1 >> | | >> | +-------------------------------+ >> | | vlan 1 | >> | | | >> | | brlan (vlan-filtering) | >> | | +---------------+ >> | | | DSA-SWITCH | >> | | vlan 1 | | >> | | to | | >> | | untagged 1 vlan 1 | >> | +---------------+---------------+ >> . / \ >> ----->wlan1 lan0 >> . . >> . ^ >> ^ vlan 1 tagged packets >> untagged packets >> >> br_vlan_fill_forward_path_mode() sets DEV_PATH_BR_VLAN_UNTAG_HW when >> filling in from brlan.1 towards wlan1. But it should be set to >> DEV_PATH_BR_VLAN_UNTAG in this case. Using BR_VLFLAG_ADDED_BY_SWITCHDEV >> is not correct. The dsa switchdev adds it as a foreign port. >> >> The same problem for all foreignly added dsa vlans on the bridge. >> >> First add the vlan, trying only native devices. >> If this fails, we know this may be a vlan from a foreign device. >> >> Use BR_VLFLAG_TAGGING_BY_SWITCHDEV to make sure DEV_PATH_BR_VLAN_UNTAG_HW >> is set only when there if no foreign device involved. >> >> Signed-off-by: Eric Woudstra <ericwouds@gmail.com> >> --- > > Shouldn't mlxsw_sp_switchdev_vxlan_vlans_add() also respect the > SWITCHDEV_F_NO_FOREIGN flag? My (maybe incorrect) understanding of > bridging topologies with vxlan and mlxsw is that they are neighbor > bridge ports, and mlxsw doesn't (seem to) call > switchdev_bridge_port_offload() for the vxlan bridge port. This > technically makes vxlan a foreign bridge port to mlxsw, so it should > skip reacting on VLAN switchdev objects when that flag is set, just > for uniform behavior across the board. > > (your patch repeats the notifier without the SWITCHDEV_F_NO_FOREIGN > flag anyway, so it only matters for flowtable offload). Or should mlxsw_sp_switchdev_blocking_event() use switchdev_handle_port_obj_add_foreign() to add the vxlan foreign port? Then all foreign ports are added in a uniform manner and SWITCHDEV_F_NO_FOREIGN is respected. I do not have the hardware to test any changes in that code.
On Fri, Feb 07, 2025 at 09:04:28PM +0100, Eric Woudstra wrote: > Or should mlxsw_sp_switchdev_blocking_event() use > switchdev_handle_port_obj_add_foreign() to add the vxlan > foreign port? > > Then all foreign ports are added in a uniform manner and > SWITCHDEV_F_NO_FOREIGN is respected. > > I do not have the hardware to test any changes in that code. Personally, in your place I wouldn't have the courage to refactor that much in a driver as complex as spectrum, but if you CC the right people from Nvidia who can test, I guess you could give that a try. Actually, how I came to spectrum was that I was thinking about an alternative mechanism of detecting "foreign or not", other than emitting two switchdev notifiers. You emit just the usual, single one, but whoever handles it for a foreign bridge port will set a new bool port_obj_info->handled_by_foreign, very similar to the existing bool port_obj_info->handled. I was looking around to see who else open-codes the switchdev object handling rather than use the switchdev_handle_*() helpers, and that's how I came across spectrum. It would seem, at first glance, easier to set just this in spectrum: diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c index 6397ff0dc951..6926aaae7278 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c @@ -3953,6 +3953,7 @@ mlxsw_sp_switchdev_vxlan_vlans_add(struct net_device *vxlan_dev, return 0; port_obj_info->handled = true; + port_obj_info->handled_by_foreign = true; bridge_device = mlxsw_sp_bridge_device_find(mlxsw_sp->bridge, br_dev); if (!bridge_device) and this in the object replication helper: diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index c48f66643e99..be82e79b5feb 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -763,6 +763,8 @@ static int __switchdev_handle_port_obj_add(struct net_device *dev, if (!foreign_dev_check_cb(switchdev, dev)) return err; + port_obj_info->handled_by_foreign = true; + return __switchdev_handle_port_obj_add(br, port_obj_info, check_cb, foreign_dev_check_cb, add_cb); } Just some care needs to be taken to only consider "handled_by_foreign" just when "handled" is true. I haven't yet decided which variant I like better, just thought I'd mention this as something which requires a single switchdev notification. Anyway, in the future I'll have to do some more tweaks with these flags in the context of LAG. These flags (BR_VLFLAG_ADDED_BY_SWITCHDEV, now also BR_VLFLAG_TAGGING_BY_SWITCHDEV after this patch) can dynamically change, and the existing code isn't great because it doesn't handle that. For example: ip link add br0 type bridge ip link set swp0 master br0 ip link set bond0 master br0 # bond0 is a foreign interface to swp0 at this time bridge vlan add dev bond0 vid 100 # this won't get BR_VLFLAG_TAGGING_BY_SWITCHDEV ip link set swp1 master bond0 # bond0 is no longer a foreign interface to swp0, assuming the same phys_switch_id # vid 100 should get BR_VLFLAG_TAGGING_BY_SWITCHDEV during br_switchdev_vlan_replay() Considering that br_switchdev_vlan_replay() will need to re-evaluate the BR_VLFLAG_TAGGING_BY_SWITCHDEV flag, I guess I do prefer the simpler variant after all - it is one call less that will have to be made during replay as well.
diff --git a/include/net/switchdev.h b/include/net/switchdev.h index 8346b0d29542..ee500706496b 100644 --- a/include/net/switchdev.h +++ b/include/net/switchdev.h @@ -15,6 +15,7 @@ #define SWITCHDEV_F_NO_RECURSE BIT(0) #define SWITCHDEV_F_SKIP_EOPNOTSUPP BIT(1) #define SWITCHDEV_F_DEFER BIT(2) +#define SWITCHDEV_F_NO_FOREIGN BIT(3) enum switchdev_attr_id { SWITCHDEV_ATTR_ID_UNDEFINED, diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index a0b950390a16..b950db453d8d 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -180,6 +180,7 @@ enum { BR_VLFLAG_MCAST_ENABLED = BIT(2), BR_VLFLAG_GLOBAL_MCAST_ENABLED = BIT(3), BR_VLFLAG_NEIGH_SUPPRESS_ENABLED = BIT(4), + BR_VLFLAG_TAGGING_BY_SWITCHDEV = BIT(5), }; /** @@ -2184,6 +2185,8 @@ void br_switchdev_mdb_notify(struct net_device *dev, int type); int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags, bool changed, struct netlink_ext_ack *extack); +int br_switchdev_port_vlan_no_foreign_add(struct net_device *dev, u16 vid, u16 flags, + bool changed, struct netlink_ext_ack *extack); int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid); void br_switchdev_init(struct net_bridge *br); @@ -2267,6 +2270,13 @@ static inline int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, return -EOPNOTSUPP; } +static inline int br_switchdev_port_vlan_no_foreign_add(struct net_device *dev, u16 vid, + u16 flags, bool changed, + struct netlink_ext_ack *extack) +{ + return -EOPNOTSUPP; +} + static inline int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid) { return -EOPNOTSUPP; diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index 7b41ee8740cb..efa7a055b8f9 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -187,6 +187,21 @@ int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags, return switchdev_port_obj_add(dev, &v.obj, extack); } +int br_switchdev_port_vlan_no_foreign_add(struct net_device *dev, u16 vid, u16 flags, + bool changed, struct netlink_ext_ack *extack) +{ + struct switchdev_obj_port_vlan v = { + .obj.orig_dev = dev, + .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN, + .obj.flags = SWITCHDEV_F_NO_FOREIGN, + .flags = flags, + .vid = vid, + .changed = changed, + }; + + return switchdev_port_obj_add(dev, &v.obj, extack); +} + int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid) { struct switchdev_obj_port_vlan v = { diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 07dae3655c26..3e50adaf8e1b 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -109,6 +109,11 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br, /* Try switchdev op first. In case it is not supported, fallback to * 8021q add. */ + err = br_switchdev_port_vlan_no_foreign_add(dev, v->vid, flags, false, extack); + if (err != -EOPNOTSUPP) { + v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV | BR_VLFLAG_TAGGING_BY_SWITCHDEV; + return err; + } err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, extack); if (err == -EOPNOTSUPP) return vlan_vid_add(dev, br->vlan_proto, v->vid); @@ -1491,7 +1496,7 @@ int br_vlan_fill_forward_path_mode(struct net_bridge *br, if (path->bridge.vlan_mode == DEV_PATH_BR_VLAN_TAG) path->bridge.vlan_mode = DEV_PATH_BR_VLAN_KEEP; - else if (v->priv_flags & BR_VLFLAG_ADDED_BY_SWITCHDEV) + else if (v->priv_flags & BR_VLFLAG_TAGGING_BY_SWITCHDEV) path->bridge.vlan_mode = DEV_PATH_BR_VLAN_UNTAG_HW; else path->bridge.vlan_mode = DEV_PATH_BR_VLAN_UNTAG; diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index 6488ead9e464..c48f66643e99 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -749,7 +749,7 @@ static int __switchdev_handle_port_obj_add(struct net_device *dev, /* Event is neither on a bridge nor a LAG. Check whether it is on an * interface that is in a bridge with us. */ - if (!foreign_dev_check_cb) + if (!foreign_dev_check_cb || port_obj_info->obj->flags & SWITCHDEV_F_NO_FOREIGN) return err; br = netdev_master_upper_dev_get(dev);
In network setup as below: fastpath bypass .----------------------------------------. / \ | IP - forwarding | | / \ v | / wan ... | / | | | | | brlan.1 | | | +-------------------------------+ | | vlan 1 | | | | | | brlan (vlan-filtering) | | | +---------------+ | | | DSA-SWITCH | | | vlan 1 | | | | to | | | | untagged 1 vlan 1 | | +---------------+---------------+ . / \ ----->wlan1 lan0 . . . ^ ^ vlan 1 tagged packets untagged packets br_vlan_fill_forward_path_mode() sets DEV_PATH_BR_VLAN_UNTAG_HW when filling in from brlan.1 towards wlan1. But it should be set to DEV_PATH_BR_VLAN_UNTAG in this case. Using BR_VLFLAG_ADDED_BY_SWITCHDEV is not correct. The dsa switchdev adds it as a foreign port. The same problem for all foreignly added dsa vlans on the bridge. First add the vlan, trying only native devices. If this fails, we know this may be a vlan from a foreign device. Use BR_VLFLAG_TAGGING_BY_SWITCHDEV to make sure DEV_PATH_BR_VLAN_UNTAG_HW is set only when there if no foreign device involved. Signed-off-by: Eric Woudstra <ericwouds@gmail.com> --- include/net/switchdev.h | 1 + net/bridge/br_private.h | 10 ++++++++++ net/bridge/br_switchdev.c | 15 +++++++++++++++ net/bridge/br_vlan.c | 7 ++++++- net/switchdev/switchdev.c | 2 +- 5 files changed, 33 insertions(+), 2 deletions(-)