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

[Nikolay Aleksandrov <razor@blackwall.org>]

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

> +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)

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