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

[Nikolay Aleksandrov <razor@blackwall.org>]

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
>