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

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 13/02/2022 22:02, Vladimir Oltean wrote:
> 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).
> 

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

> (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.

Cheers,
 Nik