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

Nikolay Aleksandrov
2022-02-15T11:08:39Z

On 15/02/2022 12:30, Vladimir Oltean wrote:
> On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
>> On 15/02/2022 11:54, Vladimir Oltean wrote:
>>> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
>>>>> +/* 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?
>

By the way I just noticed that __vlan_flags_would_change has another bug, it's testing
vlan's flags against themselves without any change (old_flags == v->flags).

I meant something similar to this (quickly hacked, untested, add flags probably
could be renamed to something more appropriate):

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1402d5ca242d..1de69090d3cb 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -34,53 +34,66 @@ 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 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;
+}
+
+static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
+{
+ return __vlan_add_flags(v, flags, false);
+}
+
+static bool __vlan_flags_commit(struct net_bridge_vlan *v, u16 flags)
+{
+ return __vlan_add_flags(v, flags, true);
}

static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,