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

Nikolay Aleksandrov
2022-02-15T15:46:55Z

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