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

[Joseph Huang]

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

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;

?

Thanks,
Joseph