Re: [Patch net-next 2/3] net: bridge: mcast: Notify on offload flag change
From: Nikolay Aleksandrov
Date: Tue Apr 01 2025 - 09:37:48 EST
On 4/1/25 15:49, Nikolay Aleksandrov wrote:
On 3/31/25 23:11, Joseph Huang wrote:
On 3/21/2025 4:47 AM, Nikolay Aleksandrov wrote:
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 68dccc2ff7b1..5b09cfcdf3f3 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -504,20 +504,41 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
struct net_bridge_mdb_entry *mp;
struct net_bridge_port *port = data->port;
struct net_bridge *br = port->br;
+ bool offload_changed = false;
+ bool failed_changed = false;
+ u8 notify;
spin_lock_bh(&br->multicast_lock);
mp = br_mdb_ip_get(br, &data->ip);
if (!mp)
goto out;
+
+ notify = br->multicast_ctx.multicast_mdb_notify_on_flag_change;
let's not waste cycles if there was an error and notify == 0, please keep the original
code path and avoid walking over the group ports.
But we do want to keep the error flag so that the error shows up in 'bridge mdb show', right? Notify should only affect the real-time notifications, and not the error status itself.
Fair enough, sounds good.
+
for (pp = &mp->ports; (p = mlock_dereference(*pp, br)) != NULL;
pp = &p->next) {
if (p->key.port != port)
continue;
- if (err)
+ if (err) {
+ if (!(p->flags & MDB_PG_FLAGS_OFFLOAD_FAILED))
+ failed_changed = true;
p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
- else
+ } else {
+ if (!(p->flags & MDB_PG_FLAGS_OFFLOAD))
+ offload_changed = true;
p->flags |= MDB_PG_FLAGS_OFFLOAD;
+ }
+
+ if (notify == MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE ||
+ (!offload_changed && !failed_changed))
+ continue;
+
+ if (notify == MDB_NOTIFY_ON_FLAG_CHANGE_FAIL_ONLY &&
+ !failed_changed)
+ continue;
+
+ br_mdb_flag_change_notify(br->dev, mp, p);
This looks like a mess.. First you need to manage these flags properly as I wrote in my
other reply, they must be mutually exclusive and you can do this in a helper. Also
please read the old flags in the beginning, then check what flags changed, make a mask
what flags are for notifications (again can come from a helper, it can be generated when
the option changes so you don't compute it every time) and decide what to do if any of
those flags changed.
Note you have to keep proper flags state regardless of the notify option.
}
out:
spin_unlock_bh(&br->multicast_lock);
How does this look:
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -496,6 +496,21 @@ struct br_switchdev_mdb_complete_info {
struct br_ip ip;
};
#define MDB_NOTIFY_FLAGS MDB_PG_FLAGS_OFFLOAD_FAILED
pardon me, you can drop this define as the flag is guarded by a specific
option so we don't always notify when we see it, you can check for it
explicitly below in changed_flags below...
+static void br_multicast_set_pg_offload_flags(int err,
+ struct net_bridge_port_group *p)
swap these two arguments please, since we don't use err you can probably
rename it to "failed" and make it a bool
alternatively if you prefer maybe rename it to
br_multicast_set_pg_offload_flag() and pass the correct flag from the
caller
e.g. br_multicast_set_pg_offload_flag(pg, err ?
MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD)
I don't mind either way.
+{
+ p->flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
+ p->flags |= (err ? MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD);
+}
+
+static bool br_multicast_should_notify(struct net_bridge *br,
hmm perhaps br_mdb_should_notify() to be more specific? I don't mind the
current name, just a thought.
also const br
+ u8 old_flags, u8 new_flags)
u8 changed_flags should suffice
+{
+ return (br_boolopt_get(br, BR_BOOLOPT_FAILED_OFFLOAD_NOTIFICATION) &&
+ ((old_flags & MDB_PG_FLAGS_OFFLOAD_FAILED) !=
+ (new_flags & MDB_PG_FLAGS_OFFLOAD_FAILED)));
if (changed_flags & MDB_NOTIFY_FLAGS)
... here just do an explicit check for the offload flag in changed_flags
instead of using a define, it is guarded by a specific option so it's ok
also no need for the extra () around the whole statement
+}
+
both of these helpers should go into br_private.h
static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *priv)
{
struct br_switchdev_mdb_complete_info *data = priv;
@@ -504,23 +519,25 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
struct net_bridge_mdb_entry *mp;
struct net_bridge_port *port = data->port;
struct net_bridge *br = port->br;
-
- if (err)
- goto err;
+ u8 old_flags;
spin_lock_bh(&br->multicast_lock);
mp = br_mdb_ip_get(br, &data->ip);
if (!mp)
goto out;
for (pp = &mp->ports; (p = mlock_dereference(*pp, br)) != NULL;
pp = &p->next) {
if (p->key.port != port)
continue;
- p->flags |= MDB_PG_FLAGS_OFFLOAD;
+
+ old_flags = p->flags;
+ br_multicast_set_pg_offload_flags(err, p);
+ if (br_multicast_should_notify(br, old_flags, p->flags))
and here it would become:
br_multicast_should_notify(br, old_flags ^ p->flags)
+ br_mdb_flag_change_notify(br->dev, mp, p);
}
out:
spin_unlock_bh(&br->multicast_lock);
-err:
kfree(priv);
}
Thanks,
Joseph
Cheers,
Nik