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

On Mon, Feb 14, 2022 at 11:05:54AM +0200, Nikolay Aleksandrov wrote:

On 13/02/2022 22:02, Vladimir Oltean wrote:

quoted

Hi Nikolay,

On Sun, Feb 13, 2022 at 08:54:50PM +0200, Nikolay Aleksandrov wrote:

quoted

Hi,
I don't like the VLAN delete on simple flags change to workaround some devices'
broken behaviour, in general I'd like to avoid adding driver workarounds in the bridge.
Either those drivers should be fixed (best approach IMO), or the workaround should only
affect them, and not everyone. The point is that a vlan has much more state than a simple
fdb, and deleting it could result in a lot more unnecessary churn which can be avoided
if these flags can be changed properly.

Agree, but the broken drivers was just an added bonus I thought I'd mention,
since the subtle implications of the API struck me as odd the first time
I realized them.

The point is that it's impossible for a switchdev driver to do correct
refcounting for this example (taken from Tobias):

   br0
   / \
swp0 tap0
 ^     ^
DSA   foreign interface

(1) ip link add br0 type bridge
(2) ip link set swp0 master br0
(3) ip link set tap0 master br0
(4) bridge vlan add dev tap0 vid 100
(5) bridge vlan add dev br0 vid 100 self
(6) bridge vlan add dev br0 vid 100 pvid self
(7) bridge vlan add dev br0 vid 100 pvid untagged self
(8) bridge vlan del dev br0 vid 100 self
(8) bridge vlan del dev tap0 vid 100

basically, if DSA were to keep track of the host-facing users of VID 100
in order to keep the CPU port programmed in that VID, it needs a way to
detect the fact that commands (6) and (7) operate on the same VID as (5),
and on a different VID than (8). So practically, it needs to keep a
shadow copy of each bridge VLAN so that it can figure out whether a
switchdev notification is for an existing VLAN or for a new one.

This is really undesirable in my mind as well, and I see two middle grounds
(both untested):

(a) call br_vlan_get_info() from the DSA switchdev notification handler
    to figure out whether the VLAN is new or not. As far as I can see in
    __vlan_add(), br_switchdev_port_vlan_add() is called before the
    insertion of the VLAN into &vg->vlan_hash, so the absence from there
    could be used as an indicator that the VLAN is new, and that the
    refcount needs to be bumped, regardless of knowing exactly which
    bridge or bridge port the VLAN came from. The important part is that
    it isn't just a flag change, for which we don't want to bump the
    refcount, and that we can rely on the bridge database and not keep a
    separate one. The disadvantage seems to be that the solution is a
    bit fragile and puts a bit too much pressure on the bridge code
    structure, if it even works (need to try).

This is undesirable for many reasons, one of which you already noted. :)

quoted

(b) extend struct switchdev_obj_port_vlan with a "bool existing" flag
    which is set to true by the "_add_existing" bridge code paths.
    This flag can be ignored by non-interested parties, and used by DSA
    and others as a hint whether to bump a refcount on the VID or not.

(c) (just a variation of b) I feel there should have been a
    SWITCHDEV_PORT_OBJ_CHANGE instead of just SWITCHDEV_PORT_OBJ_ADD,
    but it's probably too late for that.

So what do you think about option (b)?

(b) sounds good if it will be enough for DSA, it looks like the least
intrusive way to do it. Also passing that information would make simpler
some inferring by other means that the vlan already exists in drivers.

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?

Cheers,
 Nik