Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed
flat view
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: 2022-02-15 10:30:16
Subsystem:
ethernet bridge, networking [general], the rest · Maintainers:
Nikolay Aleksandrov, Ido Schimmel, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
On 15/02/2022 11:54, Vladimir Oltean wrote:quoted
On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:quoted
quoted
+/* 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.You want to squash __vlan_flags_would_change() and __vlan_add_flags() into a single function? But "would_change" returns bool, and "add" returns void.Hence the wrappers for commit == false and commit == true. You could name the precommit one __vlan_flags_would_change or something more appropriate. The point is to make sure we always update both when flags are changed.
I still have a little doubt that I understood you properly. Do you mean like this?
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1402d5ca242d..0883c11897d6 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c@@ -34,36 +34,32 @@ 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) +/* Commit a change of flags to @v. Do not use directly, instead go through + * __vlan_update_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 +67,48 @@ 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. Do not use directly, instead go through + * __vlan_update_flags(). + */ +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 bool __vlan_update_flags(struct net_bridge_vlan *v, u16 flags, + bool commit) +{ + if (!commit) + return __vlan_flags_would_change(v, flags); + + __vlan_add_flags(v, flags); + + return false; } static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
@@ -310,7 +338,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags, goto out_fdb_insert; __vlan_add_list(v); - __vlan_add_flags(v, flags); + __vlan_update_flags(v, flags, true); br_multicast_toggle_one_vlan(v, true); if (p)
@@ -670,11 +698,15 @@ static int br_vlan_add_existing(struct net_bridge *br, u16 flags, bool *changed, struct netlink_ext_ack *extack) { + bool would_change = __vlan_update_flags(vlan, flags, false); int err; - err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack); - if (err && err != -EOPNOTSUPP) - return err; + if (would_change) { + 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,7 +728,8 @@ static int br_vlan_add_existing(struct net_bridge *br, br_multicast_toggle_one_vlan(vlan, true); } - if (__vlan_add_flags(vlan, flags)) + __vlan_update_flags(vlan, flags, true); + if (would_change) *changed = true; return 0;
@@ -1247,11 +1280,18 @@ 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); + bool would_change = __vlan_update_flags(vlan, flags, false); + + if (would_change) { + /* 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_update_flags(vlan, flags, true); + *changed = would_change; return 0; }