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 17:38, Vladimir Oltean wrote:
> Hi Nikolay,
> 
> On Tue, Feb 15, 2022 at 01:08:13PM +0200, Nikolay Aleksandrov wrote:
>> +static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags, bool commit)
>>  {
>>         struct net_bridge_vlan_group *vg;
>> -       u16 old_flags = v->flags;
>> -       bool ret;
>> +       bool change;
>>  
>>         if (br_vlan_is_master(v))
>>                 vg = br_vlan_group(v->br);
>>         else
>>                 vg = nbp_vlan_group(v->port);
>>  
>> +       /* check if anything would be changed on commit */
>> +       change = (!!(flags & BRIDGE_VLAN_INFO_PVID) == !!(vg->pvid != v->vid) ||
>> +                 ((flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED));
>> +
>> +       if (!commit)
>> +               goto out;
>> +
>>         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 ret || !!(old_flags ^ v->flags);
>> +out:
>> +       return change;
>> +}
> 
> I'm really sorry that I keep insisting, but could you please clarify
> something for me.
> 
> Here you change the behavior of __vlan_add_flags(): yes, it only changes
> BRIDGE_VLAN_INFO_UNTAGGED and BRIDGE_VLAN_INFO_PVID, but it used to
> return true if other flags changed as well, like BRIDGE_VLAN_INFO_BRENTRY.
> Now it does not, since you've explicitly made it so, and for good
> reason: make the function do what it says on the box.
> 
> However, this forces me to add extra logic in br_vlan_add_existing()
> such that a transition of master vlan flags from !BRENTRY -> BRENTRY is
> still notified to switchdev.
> 
> Which begs the question, why exactly do we even bother to notify to
> switchdev master VLANs that aren't brentries?! All drivers that I could
> find just ignore VLANs that aren't brentries.
> 
> I can suppress the switchdev notification of these private bridge data
> structures like this, and nothing else seems to be affected:
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 6fc2e366f13a..4607689c4e6a 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -300,10 +300,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
>  		}
>  		br_multicast_port_ctx_init(p, v, &v->port_mcast_ctx);
>  	} else {
> -		err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, 0,
> -						 extack);
> -		if (err && err != -EOPNOTSUPP)
> -			goto out;
> +		if (br_vlan_should_use(v)) {
> +			err = br_switchdev_port_vlan_add(dev, v->vid, flags,
> +							 false, 0, extack);
> +			if (err && err != -EOPNOTSUPP)
> +				goto out;
> +		}
>  		br_multicast_ctx_init(br, v, &v->br_mcast_ctx);
>  		v->priv_flags |= BR_VLFLAG_GLOBAL_MCAST_ENABLED;
>  	}
> 
> Would you mind if I added an extra patch that also does this (it would
> also remove the need for this check in drivers, plus it would change the
> definition of "changed" to something IMO more reasonable, i.e. a master
> VLAN that isn't brentry doesn't really exist, so even though the
> !BRENTRY->BRENTRY is a flag change, it isn't a change of a switchdev
> VLAN object that anybody offloads), or is there some reason I'm missing?

I agree, patch looks good to me. I see that everyone already filters the
!BRENTRY case anyway.

Thanks,
 Nik