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

[Ido Schimmel <idosch@idosch.org>]

On Mon, Feb 14, 2022 at 11:05:54AM +0200, Nikolay Aleksandrov wrote:
> 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.

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