Date:   Sun, 13 Feb 2022 20:02:55 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Nikolay Aleksandrov <nikolay@...dia.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Roopa Prabhu <roopa@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Ido Schimmel <idosch@...dia.com>,
        Rafael Richter <rafael.richter@....de>,
        Daniel Klauer <daniel.klauer@....de>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [RFC PATCH net-next 0/5] Replay and offload host VLAN entries in
 DSA

Hi Nikolay,

On Sun, Feb 13, 2022 at 08:54:50PM +0200, Nikolay Aleksandrov wrote:
> 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).

(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)?

Powered by blists - more mailing lists