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

Hi Nikolay,

On Tue, Feb 15, 2022 at 01:08:13PM +0200, Nikolay Aleksandrov wrote:

quoted

+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?