Re: [PATCH v3 net-next 04/11] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 15/02/2022 19:02, Vladimir Oltean wrote:
> Currently, when a VLAN entry is added multiple times in a row to a
> bridge port, nbp_vlan_add() calls br_switchdev_port_vlan_add() each
> time, even if the VLAN already exists and nothing about it has changed:
> 
> bridge vlan add dev lan12 vid 100 master static
> 
> Similarly, when a VLAN is added multiple times in a row to a bridge,
> br_vlan_add_existing() doesn't filter at all the calls to
> br_switchdev_port_vlan_add():
> 
> bridge vlan add dev br0 vid 100 self
> 
> This behavior makes driver-level accounting of VLANs impossible, since
> it is enough for a single deletion event to remove a VLAN, but the
> addition event can be emitted an unlimited number of times.
> 
> The cause for this can be identified as follows: we rely on
> __vlan_add_flags() to retroactively tell us whether it has changed
> anything about the VLAN flags or VLAN group pvid. So we'd first have to
> call __vlan_add_flags() before calling br_switchdev_port_vlan_add(), in
> order to have access to the "bool *changed" information. But we don't
> want to change the event ordering, because we'd have to revert the
> struct net_bridge_vlan changes we've made if switchdev returns an error.
> 
> So to solve this, we need another function that tells us whether any
> change is going to occur in the VLAN or VLAN group, _prior_ to calling
> __vlan_add_flags().
> 
> Split __vlan_add_flags() into a precommit and a commit stage, and rename
> it to __vlan_flags_update(). The precommit stage,
> __vlan_flags_would_change(), will determine whether there is any reason
> to notify switchdev due to a change of flags (note: the BRENTRY flag
> transition from false to true is treated separately: as a new switchdev
> entry, because we skipped notifying the master VLAN when it wasn't a
> brentry yet, and therefore not as a change of flags).
> 
> With this lookahead/precommit function in place, we can avoid notifying
> switchdev if nothing changed for the VLAN and VLAN group.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v2->v3:
> - create precommit and commit wrappers around __vlan_add_flags().
> - special-case the BRENTRY transition from false to true, instead of
>   treating it as a change of flags and letting drivers figure out that
>   it really isn't.
> - avoid setting *changed unless we know that functions will not error
>   out later.
> 
> v1->v2:
> - drop the br_vlan_restore_existing() approach, just figure out ahead of
>   time what will change.
> 

Thanks for seeing this through.

Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>