Add MDB_FLAGS_OFFLOAD_FAILED and MDB_PG_FLAGS_OFFLOAD_FAILED to indicate
that an attempt to offload the MDB entry to switchdev has failed.
Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
include/uapi/linux/if_bridge.h | 9 +++++----
net/bridge/br_mdb.c | 2 ++
net/bridge/br_private.h | 11 ++++++-----
net/bridge/br_switchdev.c | 10 +++++-----
4 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index a5b743a2f775..f2a6de424f3f 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -699,10 +699,11 @@ struct br_mdb_entry {
#define MDB_TEMPORARY 0
#define MDB_PERMANENT 1
__u8 state;
-#define MDB_FLAGS_OFFLOAD (1 << 0)
-#define MDB_FLAGS_FAST_LEAVE (1 << 1)
-#define MDB_FLAGS_STAR_EXCL (1 << 2)
-#define MDB_FLAGS_BLOCKED (1 << 3)
+#define MDB_FLAGS_OFFLOAD (1 << 0)
+#define MDB_FLAGS_FAST_LEAVE (1 << 1)
+#define MDB_FLAGS_STAR_EXCL (1 << 2)
+#define MDB_FLAGS_BLOCKED (1 << 3)
+#define MDB_FLAGS_OFFLOAD_FAILED (1 << 4)
__u8 flags;
__u16 vid;
struct {
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 1a52a0bca086..0639691cd19b 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -144,6 +144,8 @@ static void __mdb_entry_fill_flags(struct br_mdb_entry *e, unsigned char flags)
e->flags |= MDB_FLAGS_STAR_EXCL;
if (flags & MDB_PG_FLAGS_BLOCKED)
e->flags |= MDB_FLAGS_BLOCKED;
+ if (flags & MDB_PG_FLAGS_OFFLOAD_FAILED)
+ e->flags |= MDB_FLAGS_OFFLOAD_FAILED;
}
static void __mdb_entry_to_br_ip(struct br_mdb_entry *entry, struct br_ip *ip,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1054b8a88edc..cd6b4e91e7d6 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -306,11 +306,12 @@ struct net_bridge_fdb_flush_desc {
u16 vlan_id;
};
-#define MDB_PG_FLAGS_PERMANENT BIT(0)
-#define MDB_PG_FLAGS_OFFLOAD BIT(1)
-#define MDB_PG_FLAGS_FAST_LEAVE BIT(2)
-#define MDB_PG_FLAGS_STAR_EXCL BIT(3)
-#define MDB_PG_FLAGS_BLOCKED BIT(4)
+#define MDB_PG_FLAGS_PERMANENT BIT(0)
+#define MDB_PG_FLAGS_OFFLOAD BIT(1)
+#define MDB_PG_FLAGS_FAST_LEAVE BIT(2)
+#define MDB_PG_FLAGS_STAR_EXCL BIT(3)
+#define MDB_PG_FLAGS_BLOCKED BIT(4)
+#define MDB_PG_FLAGS_OFFLOAD_FAILED BIT(5)
#define PG_SRC_ENT_LIMIT 32
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 7b41ee8740cb..68dccc2ff7b1 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -505,9 +505,6 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
struct net_bridge_port *port = data->port;
struct net_bridge *br = port->br;
- if (err)
- goto err;
-
spin_lock_bh(&br->multicast_lock);
mp = br_mdb_ip_get(br, &data->ip);
if (!mp)
@@ -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;
}
out:
spin_unlock_bh(&br->multicast_lock);
-err:
kfree(priv);
}
--
2.49.0On 3/19/25 00:42, Joseph Huang wrote:
> Add MDB_FLAGS_OFFLOAD_FAILED and MDB_PG_FLAGS_OFFLOAD_FAILED to indicate
> that an attempt to offload the MDB entry to switchdev has failed.
>
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> ---
> include/uapi/linux/if_bridge.h | 9 +++++----
> net/bridge/br_mdb.c | 2 ++
> net/bridge/br_private.h | 11 ++++++-----
> net/bridge/br_switchdev.c | 10 +++++-----
> 4 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index a5b743a2f775..f2a6de424f3f 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -699,10 +699,11 @@ struct br_mdb_entry {
> #define MDB_TEMPORARY 0
> #define MDB_PERMANENT 1
> __u8 state;
> -#define MDB_FLAGS_OFFLOAD (1 << 0)
> -#define MDB_FLAGS_FAST_LEAVE (1 << 1)
> -#define MDB_FLAGS_STAR_EXCL (1 << 2)
> -#define MDB_FLAGS_BLOCKED (1 << 3)
> +#define MDB_FLAGS_OFFLOAD (1 << 0)
> +#define MDB_FLAGS_FAST_LEAVE (1 << 1)
> +#define MDB_FLAGS_STAR_EXCL (1 << 2)
> +#define MDB_FLAGS_BLOCKED (1 << 3)
> +#define MDB_FLAGS_OFFLOAD_FAILED (1 << 4)
> __u8 flags;
> __u16 vid;
> struct {
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 1a52a0bca086..0639691cd19b 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -144,6 +144,8 @@ static void __mdb_entry_fill_flags(struct br_mdb_entry *e, unsigned char flags)
> e->flags |= MDB_FLAGS_STAR_EXCL;
> if (flags & MDB_PG_FLAGS_BLOCKED)
> e->flags |= MDB_FLAGS_BLOCKED;
> + if (flags & MDB_PG_FLAGS_OFFLOAD_FAILED)
> + e->flags |= MDB_FLAGS_OFFLOAD_FAILED;
> }
>
> static void __mdb_entry_to_br_ip(struct br_mdb_entry *entry, struct br_ip *ip,
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 1054b8a88edc..cd6b4e91e7d6 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -306,11 +306,12 @@ struct net_bridge_fdb_flush_desc {
> u16 vlan_id;
> };
>
> -#define MDB_PG_FLAGS_PERMANENT BIT(0)
> -#define MDB_PG_FLAGS_OFFLOAD BIT(1)
> -#define MDB_PG_FLAGS_FAST_LEAVE BIT(2)
> -#define MDB_PG_FLAGS_STAR_EXCL BIT(3)
> -#define MDB_PG_FLAGS_BLOCKED BIT(4)
> +#define MDB_PG_FLAGS_PERMANENT BIT(0)
> +#define MDB_PG_FLAGS_OFFLOAD BIT(1)
> +#define MDB_PG_FLAGS_FAST_LEAVE BIT(2)
> +#define MDB_PG_FLAGS_STAR_EXCL BIT(3)
> +#define MDB_PG_FLAGS_BLOCKED BIT(4)
> +#define MDB_PG_FLAGS_OFFLOAD_FAILED BIT(5)
>
> #define PG_SRC_ENT_LIMIT 32
>
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 7b41ee8740cb..68dccc2ff7b1 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -505,9 +505,6 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
> struct net_bridge_port *port = data->port;
> struct net_bridge *br = port->br;
>
> - if (err)
> - goto err;
> -
> spin_lock_bh(&br->multicast_lock);
> mp = br_mdb_ip_get(br, &data->ip);
> if (!mp)
> @@ -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.
> }
> out:
> spin_unlock_bh(&br->multicast_lock);
> -err:
> kfree(priv);
> }
>
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
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. :)
My point is that they must be managed correctly and shouldn't be allowed
to be set simultaneously.
Cheers,
Nik
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
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
© 2016 - 2025 Red Hat, Inc.