Re: [PATCH v2 net-next 04/10] net: bridge: mst: Notify switchdev drivers of VLAN MSTI migrations

[Vladimir Oltean]

On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote:
> On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> > On Tue, Mar 01, 2022 at 11:03:15AM +0100, Tobias Waldekranz wrote:
> >> Whenever a VLAN moves to a new MSTI, send a switchdev notification so
> >> that switchdevs can...
> >> 
> >> ...either refuse the migration if the hardware does not support
> >> offloading of MST...
> >> 
> >> ..or track a bridge's VID to MSTI mapping when offloading is
> >> supported.
> >> 
> >> Signed-off-by: Tobias Waldekranz <tobias@xxxxxxxxxxxxxx>
> >> ---
> >>  include/net/switchdev.h   | 10 +++++++
> >>  net/bridge/br_mst.c       | 15 +++++++++++
> >>  net/bridge/br_switchdev.c | 57 +++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 82 insertions(+)
> >> 
> >> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> >> index 3e424d40fae3..39e57aa5005a 100644
> >> --- a/include/net/switchdev.h
> >> +++ b/include/net/switchdev.h
> >> @@ -28,6 +28,7 @@ enum switchdev_attr_id {
> >>  	SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> >>  	SWITCHDEV_ATTR_ID_BRIDGE_MROUTER,
> >>  	SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
> >> +	SWITCHDEV_ATTR_ID_VLAN_MSTI,
> >>  };
> >>  
> >>  struct switchdev_brport_flags {
> >> @@ -35,6 +36,14 @@ struct switchdev_brport_flags {
> >>  	unsigned long mask;
> >>  };
> >>  
> >> +struct switchdev_vlan_attr {
> >> +	u16 vid;
> >> +
> >> +	union {
> >> +		u16 msti;
> >> +	};
> >
> > Do you see other VLAN attributes that would be added in the future, such
> > as to justify making this a single-element union from the get-go?
> 
> I could imagine being able to control things like multicast snooping on
> a per-VLAN basis. Being able to act as a multicast router in one VLAN
> but not another.
> 
> > Anyway if that is the case, we're lacking an id for the attribute type,
> > so we'd end up needing to change drivers when a second union element
> > appears. Otherwise they'd all expect an u16 msti.
> 
> My idea was that `enum switchdev_attr_id` would hold all of that
> information. In this example SWITCHDEV_ATTR_ID_VLAN_MSTI, denotes both
> that `vlan_attr` is the valid member of `u` and that `msti` is the valid
> member of `vlan_attr`. If we add SWITCHDEV_ATTR_ID_VLAN_SNOOPING, that
> would point to both `vlan_attr` and a new `bool snooping` in the union.
> 
> Do you think we should just have a SWITCHDEV_ATTR_ID_VLAN_ATTR for all
> per-VLAN attributes and then have a separate union?

It's the first nested union that I see, and a bit confusing.

I think it would be better if we had a

struct switchdev_vlan_attr_msti {
	u16 vid;
	u16 msti;
};

and different structures for other, future VLAN attributes. Basically
keep a 1:1 mapping between an attribute id and a union.

> >> +};
> >> +
> >>  struct switchdev_attr {
> >>  	struct net_device *orig_dev;
> >>  	enum switchdev_attr_id id;
> >> @@ -50,6 +59,7 @@ struct switchdev_attr {
> >>  		u16 vlan_protocol;			/* BRIDGE_VLAN_PROTOCOL */
> >>  		bool mc_disabled;			/* MC_DISABLED */
> >>  		u8 mrp_port_role;			/* MRP_PORT_ROLE */
> >> +		struct switchdev_vlan_attr vlan_attr;	/* VLAN_* */
> >>  	} u;
> >>  };
> >>  
> >> diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
> >> index 8dea8e7257fd..aba603675165 100644
> >> --- a/net/bridge/br_mst.c
> >> +++ b/net/bridge/br_mst.c
> >> @@ -7,6 +7,7 @@
> >>   */
> >>  
> >>  #include <linux/kernel.h>
> >> +#include <net/switchdev.h>
> >>  
> >>  #include "br_private.h"
> >>  
> >> @@ -65,9 +66,23 @@ static void br_mst_vlan_sync_state(struct net_bridge_vlan *pv, u16 msti)
> >>  
> >>  int br_mst_vlan_set_msti(struct net_bridge_vlan *mv, u16 msti)
> >>  {
> >> +	struct switchdev_attr attr = {
> >> +		.id = SWITCHDEV_ATTR_ID_VLAN_MSTI,
> >> +		.flags = SWITCHDEV_F_DEFER,
> >
> > Is the bridge spinlock held (atomic context), or otherwise why is
> > SWITCHDEV_F_DEFER needed here?
> 
> Nope, just copypasta. In fact, it shouldn't be needed when setting the
> state either, as you can only change the state via a netlink message. I
> will remove it.
> 
> >> +		.orig_dev = mv->br->dev,
> >> +		.u.vlan_attr = {
> >> +			.vid = mv->vid,
> >> +			.msti = msti,
> >> +		},
> >> +	};
> >>  	struct net_bridge_vlan_group *vg;
> >>  	struct net_bridge_vlan *pv;
> >>  	struct net_bridge_port *p;
> >> +	int err;
> >> +
> >> +	err = switchdev_port_attr_set(mv->br->dev, &attr, NULL);
> >
> > Treating a "VLAN attribute" as a "port attribute of the bridge" is
> > pushing the taxonomy just a little, but I don't have a better suggestion.
> 
> Isn't there prior art here? I thought things like VLAN filtering already
> worked like this?

Hmm, I can think of VLAN filtering as being an attribute of the bridge
device, but 'which MSTI does VLAN X belong to' is an attribute of the
VLAN (in itself a switchdev object, i.e. something countable).

If the prior art would apply as straightforward as you say, then we'd be
replaying the VLAN MSTIs together with the other port attributes - in
"pull" mode, in dsa_port_switchdev_sync_attrs(), rather than in "push"
mode with the rest of the objects - in nbp_switchdev_sync_objs().
But we're not doing that.

To prove that there is a difference between VLAN filtering as a port
property of the bridge device, and VLAN MSTIs (or other per-VLAN global
bridge options), consider this.
You create a bridge, add 10 VLANs on br0, enable VLAN filtering, then
delete the 10 VLANs and re-create them. The bridge is still VLAN
filtering.
So VLAN filtering is a property of the bridge.

Next you create a bridge, add 10 VLANs on br0, run your new command:
'bridge vlan global set dev br0 vid <VID> msti <MSTI>'
then delete the 10 VLANs and create them back.
Their MSTI is 0, not what was set via the bridge vlan global options...
Because the MSTI is a property of the VLANs, not of the bridge.

A real port attribute wouldn't behave like that.

At least this is what I understand from your patch set, I haven't run it;
sorry if I'm mistaken about something, but I can't find a clearer way to
express what I find strange.

Anyway, I'll stop uselessly commenting here - I can understand the
practical reasons why you wouldn't want to bother expanding the taxonomy
to describe this for what it really is - an "object attribute" of sorts -
because a port attribute for the bridge device has the call path you
need already laid out, including replication towards all bridge ports.