Re: [Patch net-next 2/3] net: bridge: mcast: Notify on offload flag change

[Joseph Huang <joseph.huang.2024@gmail.com>]

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.

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

+static void br_multicast_set_pg_offload_flags(int err,
+                                             struct 
net_bridge_port_group *p)
+{
+       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,
+                                      u8 old_flags, u8 new_flags)
+{
+       return (br_boolopt_get(br, 
BR_BOOLOPT_FAILED_OFFLOAD_NOTIFICATION) &&
+               ((old_flags & MDB_PG_FLAGS_OFFLOAD_FAILED) !=
+               (new_flags & MDB_PG_FLAGS_OFFLOAD_FAILED)));
+}
+
  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))
+                       br_mdb_flag_change_notify(br->dev, mp, p);
         }
  out:
         spin_unlock_bh(&br->multicast_lock);
-err:
         kfree(priv);
  }

Thanks,
Joseph