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

Vladimir Oltean
2022-02-14T23:32:12Z

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);
}

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;
+ }

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;
}
--
2.25.1