Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 15/02/2022 01:31, 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(). In fact, we even make __vlan_add_flags() return void.
> 
> With this lookahead 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>
> ---
> v1->v2: drop the br_vlan_restore_existing() approach, just figure out
>         ahead of time what will change.
> 
>  net/bridge/br_vlan.c | 71 ++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1402d5ca242d..c5355695c976 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -34,36 +34,29 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
>  	return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
>  }
>  
> -static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
> +static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
>  			    const struct net_bridge_vlan *v)
>  {
>  	if (vg->pvid == v->vid)
> -		return false;
> +		return;
>  
>  	smp_wmb();
>  	br_vlan_set_pvid_state(vg, v->state);
>  	vg->pvid = v->vid;
> -
> -	return true;
>  }
>  
> -static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
> +static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>  {
>  	if (vg->pvid != vid)
> -		return false;
> +		return;
>  
>  	smp_wmb();
>  	vg->pvid = 0;
> -
> -	return true;
>  }
>  
> -/* return true if anything changed, false otherwise */
> -static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
> +static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>  {
>  	struct net_bridge_vlan_group *vg;
> -	u16 old_flags = v->flags;
> -	bool ret;
>  
>  	if (br_vlan_is_master(v))
>  		vg = br_vlan_group(v->br);
> @@ -71,16 +64,36 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>  		vg = nbp_vlan_group(v->port);
>  
>  	if (flags & BRIDGE_VLAN_INFO_PVID)
> -		ret = __vlan_add_pvid(vg, v);
> +		__vlan_add_pvid(vg, v);
>  	else
> -		ret = __vlan_delete_pvid(vg, v->vid);
> +		__vlan_delete_pvid(vg, v->vid);
>  
>  	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>  		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>  	else
>  		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
> +}
> +
> +/* return true if anything will change as a result of __vlan_add_flags,
> + * false otherwise
> + */
> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> +{
> +	struct net_bridge_vlan_group *vg;
> +	u16 old_flags = v->flags;
> +	bool pvid_changed;
>  
> -	return ret || !!(old_flags ^ v->flags);
> +	if (br_vlan_is_master(v))
> +		vg = br_vlan_group(v->br);
> +	else
> +		vg = nbp_vlan_group(v->port);
> +
> +	if (flags & BRIDGE_VLAN_INFO_PVID)
> +		pvid_changed = (vg->pvid == v->vid);
> +	else
> +		pvid_changed = (vg->pvid != v->vid);
> +
> +	return pvid_changed || !!(old_flags ^ v->flags);
>  }

These two have to depend on each other, otherwise it's error-prone and
surely in the future someone will forget to update both.
How about add a "commit" argument to __vlan_add_flags and possibly rename
it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
with commit == false and __vlan_update_flags_commit with commit == true.
Or some other naming, the point is to always use the same flow and checks
when updating the flags to make sure people don't forget.

>  
>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> @@ -672,9 +685,13 @@ static int br_vlan_add_existing(struct net_bridge *br,
>  {
>  	int err;
>  
> -	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
> -	if (err && err != -EOPNOTSUPP)
> -		return err;
> +	*changed = __vlan_flags_would_change(vlan, flags);
> +	if (*changed) {
> +		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
> +						 extack);
> +		if (err && err != -EOPNOTSUPP)
> +			return err;
> +	}

You should revert *changed to false in the error path below,
otherwise we will emit a notification without anything changed
if the br_vlan_is_brentry(vlan)) { } block errors out.

>  
>  	if (!br_vlan_is_brentry(vlan)) {
>  		/* Trying to change flags of non-existent bridge vlan */
> @@ -696,8 +713,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
>  		br_multicast_toggle_one_vlan(vlan, true);
>  	}
>  
> -	if (__vlan_add_flags(vlan, flags))
> -		*changed = true;
> +	__vlan_add_flags(vlan, flags);
>  
>  	return 0;
>  
> @@ -1247,11 +1263,16 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
>  	*changed = false;
>  	vlan = br_vlan_find(nbp_vlan_group(port), vid);
>  	if (vlan) {
> -		/* Pass the flags to the hardware bridge */
> -		ret = br_switchdev_port_vlan_add(port->dev, vid, flags, extack);
> -		if (ret && ret != -EOPNOTSUPP)
> -			return ret;
> -		*changed = __vlan_add_flags(vlan, flags);
> +		*changed = __vlan_flags_would_change(vlan, flags);
> +		if (*changed) {
> +			/* Pass the flags to the hardware bridge */
> +			ret = br_switchdev_port_vlan_add(port->dev, vid,
> +							 flags, extack);
> +			if (ret && ret != -EOPNOTSUPP)
> +				return ret;
> +		}
> +
> +		__vlan_add_flags(vlan, flags);
>  
>  		return 0;
>  	}