Re: [Patch net-next 1/3] net: bridge: mcast: Add offload failed mdb flag

[Nikolay Aleksandrov <razor@blackwall.org>]

On 3/28/25 17:53, Joseph Huang wrote:
> On 3/27/2025 6:52 PM, Nikolay Aleksandrov wrote:
>> On 3/27/25 00:38, Joseph Huang wrote:
>>> On 3/21/2025 4:19 AM, Nikolay Aleksandrov wrote:
>>>>> @@ -516,11 +513,14 @@ static void br_switchdev_mdb_complete(struct 
>>>>> net_device *dev, int err, void *pri
>>>>>            pp = &p->next) {
>>>>>           if (p->key.port != port)
>>>>>               continue;
>>>>> -        p->flags |= MDB_PG_FLAGS_OFFLOAD;
>>>>> +
>>>>> +        if (err)
>>>>> +            p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
>>>>> +        else
>>>>> +            p->flags |= MDB_PG_FLAGS_OFFLOAD;
>>>>
>>>> These two should be mutually exclusive, either it's offloaded or it 
>>>> failed an offload,
>>>> shouldn't be possible to have both set. I'd recommend adding some 
>>>> helper that takes
>>>> care of that.
>>>
>>> It is true that these two are mutually exclusive, but strictly 
>>> speaking there are four types of entries:
>>>
>>> 1. Entries which are not offload-able (i.e., the ports are not backed 
>>> by switchdev)
>>> 2. Entries which are being offloaded, but results yet unknown
>>> 3. Entries which are successfully offloaded, and
>>> 4. Entries which failed to be offloaded
>>>
>>> Even if we ignore the ones which are being offloaded (type 2 is 
>>> transient), we still need two flags, otherwise we won't be able to 
>>> tell type 1 from type 4 entries.
>>>
>>> If we need two flags anyway, having separate flags for type 3 and 
>>> type 4 simplifies the logic.
>>>
>>> Or did I misunderstood your comments?
>>>
>>> Thanks,
>>> Joseph
>>
>> I think you misunderstood me, I don't mind having the two flags. :)
> 
> Got it. Thanks.
> 
>> My point is that they must be managed correctly and shouldn't be allowed
>> to be set simultaneously.
>>
>> Cheers,
>>   Nik
>>
> 
> Helper function like this?
> 
> +static void set_mdb_pg_offload_flags(bool err, u8 *flags)
> +{
> +    *flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
> +    *flags |= (err ? MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD);
> +}

This could work, but I have to see how it aligns with the rest of the
code to be able to answer well. Also why not just pass the pg?
Please also choose another helper name, e.g.
br_multicast_set_pg_offload_flags() or something in these lines. You
can check br_private.h for other helpers to get an idea.

> 
> and then from the call site
> 
> -        p->flags |= MDB_PG_FLAGS_OFFLOAD;
> +        set_mdb_pg_offload_flags(err, &p->flags);
> 
> ?
> 
> Or simply clearing the flags in-line:
> 
> -        p->flags |= MDB_PG_FLAGS_OFFLOAD;
> +        p->flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
> +
> +        if (err)
> +            p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
> +        else
> +            p->flags |= MDB_PG_FLAGS_OFFLOAD;
> 
> ?

I'd prefer using a  helper. Thanks.

> 
> Thanks,
> Joseph

Cheers,
  Nik