Re: [RFC PATCH net-next 0/5] Replay and offload host VLAN entries in DSA

On 14/02/2022 14:04, Vladimir Oltean wrote:

On Mon, Feb 14, 2022 at 01:11:01PM +0200, Nikolay Aleksandrov wrote:

quoted

On 14/02/2022 12:42, Vladimir Oltean wrote:

quoted

On Mon, Feb 14, 2022 at 12:27:57PM +0200, Nikolay Aleksandrov wrote:

quoted

On 14/02/2022 12:07, Vladimir Oltean wrote:

quoted

On Mon, Feb 14, 2022 at 11:59:02AM +0200, Ido Schimmel wrote:

quoted

Sounds good to me as well. I assume it means patches #1 and #2 will be
changed to make use of this flag and patch #3 will be dropped?

Not quite. Patches 1 and 2 will be kept as-is, since fundamentally,
their goal is to eliminate a useless SWITCHDEV_PORT_OBJ_ADD event when
really nothing has changed (flags == old_flags, no brentry was created).

I don't think that's needed, a two-line change like
"vlan_already_exists == true && old_flags == flags then do nothing"
would do the trick in DSA for all drivers and avoid the churn of these patches.
It will also keep the order of the events consistent with (most of) the rest
of the bridge. I'd prefer the simpler change which avoids config reverts than
these two patches.

I understand you prefer the simpler change which avoids reverting the
struct net_bridge_vlan on error, but "vlan_already_exists == true &&
old_flags == flags then do nothing" is not possible in DSA, because DSA
does not know "old_flags" unless we also pass that via
struct switchdev_obj_port_vlan. If we do that, I don't have much of an
objection, but it still seems cleaner to me if the bridge didn't notify
switchdev at all when it has nothing to say.

Or where do you mean to place the two-line change?

You mention a couple of times in both patches that you'd like to add dsa
vlan refcounting infra similar to dsa's host fdb and mdb. So I assumed that involves
keeping track of vlan entries in dsa? If so, then I thought you'd record the old flags there.

Alternatively I don't mind passing old flags if you don't intend on keeping
vlan information in dsa, it's uglier but it will avoid the reverts which will
also avoid additional notifications when these cases are hit. It makes sense
to have both old flags and new flags if the switchdev checks are done pre-config
change so it can veto any transitions it can't handle for some reason.

A third option is to do the flags check in the bridge and avoid doing the
switchdev call (e.g. in br_switchdev_port_vlan_ calls), that should avoid
the reverts as well.

If you intend to add vlan refcounting in dsa, then I'd go with just keeping track
of the flags, you'll have vlan state anyway, otherwise it's up to you. I'm ok
with both options for old flags. 

I understand it can be confusing why DSA needs to keep VLAN refcounting
yet it doesn't keep track of flags, so let me explain.

First thing to mention is that VLAN flags on CPU and DSA (cascade) ports
don't make much sense at the level of the DSA core. These ports are
really pipes that transport packets between switches and between the
switch and the CPU, so 'pvid' and 'untagged' don't really make sense.
An unwritten convention is for DSA hardware drivers to program the CPU
and DSA ports such that the VLAN information is unmodified w.r.t. how it
was/will be on the wire. The only important aspect about VLAN on DSA and
CPU ports is the VID membership list.

This isn't the major reason, though, so secondly, I need to introduce a topology.

               eth0                                           eth1
           (DSA master)                                (foreign interface)
                |
                |
                | CPU port (no netdev)
        +----------------+
        |     sw0p0      |
        |                |
        |    Switch 0    |
        |                |
        | sw0p0   sw0p1  |
        +----------------+
            |       | DSA/cascade port (no netdev)
            |       |
            |       |
            |       | DSA/cascade port (no netdev)
            |    +--------------+
            |    | sw1p0        |
            |    |              |
            |    |   Switch 1   |
            |    |              |
            |    |sw1p1  sw1p2  |
            |    +--------------+
 user port  |       |      |
(has netdev)   user port  user port

-----------------------------------------------------------

Example 1: you want forwarding in VLAN 100 between sw0p0, sw1p1 and
sw1p2, so you issue these commands:

ip link add br0 type bridge vlan_filtering 1
ip link set sw0p0 master br0 && bridge vlan add dev sw0p0 vid 100
ip link set sw1p1 master br0 && bridge vlan add dev sw1p1 vid 100
ip link set sw1p2 master br0 && bridge vlan add dev sw1p2 vid 100

DSA must figure out that the sw0p1 and sw1p0 (the cascade ports
interconnecting the 2 switches) must also be members of VID 100.
So it must privately add each cascade port of a switch to this VID,
as long as a user port of that switch is in VID 100.

So the refcounting must be kept on the destination of where those VLANs
are programmed (the cascade ports), not on the sources of where those
VLANs came from (the user ports).

In this example, sw0p1 and sw1p0 will be members of VID 100 and will
have a refcount of 3 on it (it is needed by 3 user ports).

-----------------------------------------------------------

Example 2: you figure out that sw1p2 should in fact be pvid untagged in
VID 100, so you change that:

bridge vlan add dev sw1p2 pvid untagged

DSA doesn't care about a change of flags, so it needs to change nothing
about the DSA ports. It just needs to pass on the notification of the
change of flags to the device driver for sw1p2.

-----------------------------------------------------------

Example 3: you realize that you know what, you don't want sw1p2 to be a
member in VID 100 at all:

bridge vlan del dev sw1p2 vid 100

DSA must decrement the refcount of VID 100 on sw0p1 and sw1p0 from 3 to 2.

-----------------------------------------------------------

Example 4: you want local termination in VID 100 for this bridge, so you do:

bridge vlan add dev br0 vid 100 self

To ensure local termination works for all DSA user ports that are
members of the bridge (sw0p0, sw1p1), DSA must ensure that the CPU port
and the DSA ports towards it are members of VID 100.

So sw0p0 joins VID 100 with a refcount of 1, while sw0p1 and sw1p0 just
bump the refcount again from 2 to 3.

-----------------------------------------------------------

Example 5: you actually want to forward in VID 100 to a foreign
interface as well:

ip link set eth1 master br0 && bridge vlan add dev eth1 vid 100

DSA must bump the refcount of VID 100 on sw0p0 (the CPU port) from 1 to 2,
and for the cascade ports from 3 to 4. This is such that, if we decide
that we no longer want local termination (example 4), VID 100 is not
removed on those ports. Local termination vs software forwarding is
identical from DSA's perspective, but different from the stack's perspective.


The way this worked until now was by an approximation that held quite
honorably more often than not: whenever a bridge VLAN is added to a user
port, just add it to all DSA and CPU ports too, and never remove it.
However, it gets quite limiting.



So the way in which VLAN membership on CPU and DSA ports is refcounted
makes it not really practical to keep original flags of each VLAN on
each source port it came from. Your suggestion of keeping each vlan->flags
in DSA practically boils down to keeping an entire copy of the bridge
VLAN database. I hope it's clearer now why it isn't really the path I
would like to follow.

I see, thank you for the examples and the detailed explanation.

I can look into pruning useless calls in br_switchdev_port_vlan_add()
itself, if that's what you prefer. Before looking closer at the code,
that's what I originally had in mind, but it would pollute all callers
of that function, since we'd have to provide a "bool existing" +
"u16 old_flags" set of extra arguments to it, and most call paths would
pass "false, 0". When I noticed that the call paths I'm interested in
already compute "bool changed" based on __vlan_add_flags(), I thought it
would be a bit redundant to duplicate logic from that function.
Just reorder the call to __vlan_add_flags() and I have the info I need.

You already have to add "existing", so I don't see why not add
old flags as well. As I mentioned it makes sense to have it for pre-config
veto of unsupported state transitions. I think it will be simpler and
we'll avoid the config reverts and more notifications.

Cheers,
 Nik