From: Tobias Waldekranz <tobias@waldekranz.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: davem@davemloft.net, kuba@kernel.org,
Andrew Lunn <andrew@lunn.ch>,
Vivien Didelot <vivien.didelot@gmail.com>,
Florian Fainelli <f.fainelli@gmail.com>,
Jiri Pirko <jiri@resnulli.us>, Ivan Vecera <ivecera@redhat.com>,
Roopa Prabhu <roopa@nvidia.com>,
Nikolay Aleksandrov <razor@blackwall.org>,
Russell King <linux@armlinux.org.uk>,
Petr Machata <petrm@nvidia.com>, Cooper Lees <me@cooperlees.com>,
Ido Schimmel <idosch@nvidia.com>,
Matt Johnston <matt@codeconstruct.com.au>,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
bridge@lists.linux-foundation.org
Subject: Re: [PATCH v2 net-next 04/10] net: bridge: mst: Notify switchdev drivers of VLAN MSTI migrations
Date: Wed, 09 Mar 2022 16:34:18 +0100 [thread overview]
Message-ID: <87sfrrm96t.fsf@waldekranz.com> (raw)
In-Reply-To: <20220308171717.s2hqp6raoe5gcgtl@skbuf>
On Tue, Mar 08, 2022 at 19:17, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Mar 08, 2022 at 09:01:04AM +0100, Tobias Waldekranz wrote:
>> On Thu, Mar 03, 2022 at 22:59, Vladimir Oltean <olteanv@gmail.com> 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@waldekranz.com>
>> >> ---
>> >> 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.
Yeah, I like the simplicity of that. Changing.
>> >> +};
>> >> +
>> >> 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.
I yield, I yield! :)
next prev parent reply other threads:[~2022-03-09 15:34 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-01 10:03 [PATCH v2 net-next 00/10] net: bridge: Multiple Spanning Trees Tobias Waldekranz
2022-03-01 10:03 ` [PATCH v2 net-next 01/10] net: bridge: mst: Multiple Spanning Tree (MST) mode Tobias Waldekranz
2022-03-01 23:01 ` Nikolay Aleksandrov
2022-03-07 14:53 ` Tobias Waldekranz
2022-03-03 22:28 ` Vladimir Oltean
2022-03-01 10:03 ` [PATCH v2 net-next 02/10] net: bridge: mst: Allow changing a VLAN's MSTI Tobias Waldekranz
2022-03-03 22:27 ` Vladimir Oltean
2022-03-07 14:54 ` Tobias Waldekranz
2022-03-01 10:03 ` [PATCH v2 net-next 03/10] net: bridge: mst: Support setting and reporting MST port states Tobias Waldekranz
2022-03-01 23:19 ` Nikolay Aleksandrov
2022-03-02 1:53 ` Roopa Prabhu
2022-03-07 15:03 ` Tobias Waldekranz
2022-03-07 15:00 ` Tobias Waldekranz
2022-03-07 15:03 ` Nikolay Aleksandrov
2022-03-01 10:03 ` [PATCH v2 net-next 04/10] net: bridge: mst: Notify switchdev drivers of VLAN MSTI migrations Tobias Waldekranz
2022-03-03 20:59 ` Vladimir Oltean
2022-03-08 8:01 ` Tobias Waldekranz
2022-03-08 17:17 ` Vladimir Oltean
2022-03-09 15:34 ` Tobias Waldekranz [this message]
2022-03-01 10:03 ` [PATCH v2 net-next 05/10] net: bridge: mst: Notify switchdev drivers of MST state changes Tobias Waldekranz
2022-03-01 10:03 ` [PATCH v2 net-next 06/10] net: dsa: Pass VLAN MSTI migration notifications to driver Tobias Waldekranz
2022-03-03 22:29 ` Vladimir Oltean
2022-03-09 15:47 ` Tobias Waldekranz
2022-03-09 17:03 ` Vladimir Oltean
2022-03-01 10:03 ` [PATCH v2 net-next 07/10] net: dsa: Pass MST state changes " Tobias Waldekranz
2022-03-03 22:20 ` Vladimir Oltean
2022-03-10 8:54 ` Tobias Waldekranz
2022-03-10 10:35 ` Vladimir Oltean
2022-03-10 16:05 ` Tobias Waldekranz
2022-03-10 16:18 ` Vladimir Oltean
2022-03-10 22:46 ` Tobias Waldekranz
2022-03-10 23:08 ` Vladimir Oltean
2022-03-10 23:59 ` Tobias Waldekranz
2022-03-11 0:22 ` Vladimir Oltean
2022-03-11 9:01 ` Tobias Waldekranz
2022-03-10 16:20 ` Tobias Waldekranz
2022-03-01 10:03 ` [PATCH v2 net-next 08/10] net: dsa: mv88e6xxx: Disentangle STU from VTU Tobias Waldekranz
2022-03-01 10:03 ` [PATCH v2 net-next 09/10] net: dsa: mv88e6xxx: Export STU as devlink region Tobias Waldekranz
2022-03-01 10:03 ` [PATCH v2 net-next 10/10] net: dsa: mv88e6xxx: MST Offloading Tobias Waldekranz
2022-03-03 22:26 ` Vladimir Oltean
2022-03-10 15:14 ` Tobias Waldekranz
2022-03-10 15:25 ` Vladimir Oltean
2022-03-10 15:33 ` Vladimir Oltean
2022-03-01 16:21 ` [PATCH v2 net-next 00/10] net: bridge: Multiple Spanning Trees Vladimir Oltean
2022-03-01 17:19 ` Stephen Hemminger
2022-03-01 21:20 ` Tobias Waldekranz
2022-03-01 22:30 ` Pavel Šimerda
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=87sfrrm96t.fsf@waldekranz.com \
--to=tobias@waldekranz.com \
--cc=andrew@lunn.ch \
--cc=bridge@lists.linux-foundation.org \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=idosch@nvidia.com \
--cc=ivecera@redhat.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=matt@codeconstruct.com.au \
--cc=me@cooperlees.com \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=petrm@nvidia.com \
--cc=razor@blackwall.org \
--cc=roopa@nvidia.com \
--cc=vivien.didelot@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).