[Patch net-next 0/3] Add support for mdb offload failure notification

[Patch net-next 0/3] Add support for mdb offload failure notification

[Joseph Huang]

Currently the bridge does not provide real-time feedback to user space
on whether or not an attempt to offload an mdb entry was successful.

This patch set adds support to notify user space about successful and
failed offload attempts, and the behavior is controlled by a new knob
mdb_notify_on_flag_change:

0 - the bridge will not notify user space about MDB flag change
1 - the bridge will notify user space about flag change if either
    MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
2 - the bridge will notify user space about flag change only if
    MDB_PG_FLAGS_OFFLOAD_FAILED has changed

The default value is 0.

A break-down of the patches in the series:

Patch 1 adds offload failed flag to indicate that the offload attempt
has failed. The flag is reflected in netlink mdb entry flags.

Patch 2 adds the knob mdb_notify_on_flag_change, and notify user space
accordingly in br_switchdev_mdb_complete() when the result is known.

Patch 3 adds netlink interface to manipulate mdb_notify_on_flag_change
knob.

This patch set was inspired by the patch series "Add support for route
offload failure notifications" discussed here:
https://lore.kernel.org/all/20210207082258.3872086-1-idosch@idosch.org/

Joseph Huang (3):
  net: bridge: mcast: Add offload failed mdb flag
  net: bridge: mcast: Notify on offload flag change
  net: bridge: Add notify on flag change netlink i/f

 include/uapi/linux/if_bridge.h |  9 +++++----
 include/uapi/linux/if_link.h   | 14 ++++++++++++++
 net/bridge/br_mdb.c            | 30 +++++++++++++++++++++++++-----
 net/bridge/br_multicast.c      | 25 +++++++++++++++++++++++++
 net/bridge/br_netlink.c        | 21 +++++++++++++++++++++
 net/bridge/br_private.h        | 26 +++++++++++++++++++++-----
 net/bridge/br_switchdev.c      | 31 ++++++++++++++++++++++++++-----
 7 files changed, 137 insertions(+), 19 deletions(-)

-- 
2.49.0

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

[Joseph Huang]

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.0

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

[Joseph Huang]

Notify user space on offload flag(s) change.

This behavior is controlled by the new knob mdb_notify_on_flag_change:

0 - the bridge will not notify user space about MDB flag change
1 - the bridge will notify user space about flag change if either
    MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
2 - the bridge will notify user space about flag change only if
    MDB_PG_FLAGS_OFFLOAD_FAILED has changed

The default value is 0.

Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
 net/bridge/br_mdb.c       | 28 +++++++++++++++++++++++-----
 net/bridge/br_multicast.c | 25 +++++++++++++++++++++++++
 net/bridge/br_private.h   | 15 +++++++++++++++
 net/bridge/br_switchdev.c | 25 +++++++++++++++++++++++--
 4 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 0639691cd19b..d206b5a160f3 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -519,16 +519,17 @@ static size_t rtnl_mdb_nlmsg_size(const struct net_bridge_port_group *pg)
 	       rtnl_mdb_nlmsg_pg_size(pg);
 }
 
-void br_mdb_notify(struct net_device *dev,
-		   struct net_bridge_mdb_entry *mp,
-		   struct net_bridge_port_group *pg,
-		   int type)
+static void _br_mdb_notify(struct net_device *dev,
+			   struct net_bridge_mdb_entry *mp,
+			   struct net_bridge_port_group *pg,
+			   int type, bool notify_switchdev)
 {
 	struct net *net = dev_net(dev);
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
 
-	br_switchdev_mdb_notify(dev, mp, pg, type);
+	if (notify_switchdev)
+		br_switchdev_mdb_notify(dev, mp, pg, type);
 
 	skb = nlmsg_new(rtnl_mdb_nlmsg_size(pg), GFP_ATOMIC);
 	if (!skb)
@@ -546,6 +547,23 @@ void br_mdb_notify(struct net_device *dev,
 	rtnl_set_sk_err(net, RTNLGRP_MDB, err);
 }
 
+void br_mdb_notify(struct net_device *dev,
+		   struct net_bridge_mdb_entry *mp,
+		   struct net_bridge_port_group *pg,
+		   int type)
+{
+	_br_mdb_notify(dev, mp, pg, type, true);
+}
+
+#ifdef CONFIG_NET_SWITCHDEV
+void br_mdb_flag_change_notify(struct net_device *dev,
+			       struct net_bridge_mdb_entry *mp,
+			       struct net_bridge_port_group *pg)
+{
+	_br_mdb_notify(dev, mp, pg, RTM_NEWMDB, false);
+}
+#endif
+
 static int nlmsg_populate_rtr_fill(struct sk_buff *skb,
 				   struct net_device *dev,
 				   int ifindex, u16 vid, u32 pid,
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b2ae0d2434d2..8d583caecd40 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -4051,6 +4051,10 @@ void br_multicast_ctx_init(struct net_bridge *br,
 	brmctx->ip6_querier.port_ifidx = 0;
 	seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock);
 #endif
+#ifdef CONFIG_NET_SWITCHDEV
+	brmctx->multicast_mdb_notify_on_flag_change =
+		MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE;
+#endif
 
 	timer_setup(&brmctx->ip4_mc_router_timer,
 		    br_ip4_multicast_local_router_expired, 0);
@@ -4708,6 +4712,27 @@ int br_multicast_set_mld_version(struct net_bridge_mcast *brmctx,
 }
 #endif
 
+#ifdef CONFIG_NET_SWITCHDEV
+int br_multicast_set_mdb_notify_on_flag_change(struct net_bridge_mcast *brmctx,
+					       u8 val)
+{
+	switch (val) {
+	case MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE:
+	case MDB_NOTIFY_ON_FLAG_CHANGE_BOTH:
+	case MDB_NOTIFY_ON_FLAG_CHANGE_FAIL_ONLY:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_bh(&brmctx->br->multicast_lock);
+	brmctx->multicast_mdb_notify_on_flag_change = val;
+	spin_unlock_bh(&brmctx->br->multicast_lock);
+
+	return 0;
+}
+#endif
+
 void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx,
 				  unsigned long val)
 {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index cd6b4e91e7d6..8e8de5d54ae3 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -132,6 +132,10 @@ struct net_bridge_mcast_port {
 #endif /* CONFIG_BRIDGE_IGMP_SNOOPING */
 };
 
+#define MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE	0
+#define MDB_NOTIFY_ON_FLAG_CHANGE_BOTH		1
+#define MDB_NOTIFY_ON_FLAG_CHANGE_FAIL_ONLY	2
+
 /* net_bridge_mcast must be always defined due to forwarding stubs */
 struct net_bridge_mcast {
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
@@ -146,6 +150,9 @@ struct net_bridge_mcast {
 	u8				multicast_router;
 #if IS_ENABLED(CONFIG_IPV6)
 	u8				multicast_mld_version;
+#endif
+#ifdef CONFIG_NET_SWITCHDEV
+	u8				multicast_mdb_notify_on_flag_change;
 #endif
 	unsigned long			multicast_last_member_interval;
 	unsigned long			multicast_membership_interval;
@@ -988,6 +995,10 @@ int br_multicast_set_igmp_version(struct net_bridge_mcast *brmctx,
 int br_multicast_set_mld_version(struct net_bridge_mcast *brmctx,
 				 unsigned long val);
 #endif
+#ifdef CONFIG_NET_SWITCHDEV
+int br_multicast_set_mdb_notify_on_flag_change(struct net_bridge_mcast *brmctx,
+					       u8 val);
+#endif
 struct net_bridge_mdb_entry *
 br_mdb_ip_get(struct net_bridge *br, struct br_ip *dst);
 struct net_bridge_mdb_entry *
@@ -1004,6 +1015,10 @@ int br_mdb_hash_init(struct net_bridge *br);
 void br_mdb_hash_fini(struct net_bridge *br);
 void br_mdb_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
 		   struct net_bridge_port_group *pg, int type);
+#ifdef CONFIG_NET_SWITCHDEV
+void br_mdb_flag_change_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
+			       struct net_bridge_port_group *pg);
+#endif
 void br_rtr_notify(struct net_device *dev, struct net_bridge_mcast_port *pmctx,
 		   int type);
 void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
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;
+
 	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);
 	}
 out:
 	spin_unlock_bh(&br->multicast_lock);
-- 
2.49.0

[Patch net-next 3/3] net: bridge: Add notify on flag change netlink i/f

[Joseph Huang]

Add netlink interface to manipulate the mdb_notify_on_flag_change knob.

Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
 include/uapi/linux/if_link.h | 14 ++++++++++++++
 net/bridge/br_netlink.c      | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bfe880fbbb24..8fa830599972 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -741,6 +741,19 @@ enum in6_addr_gen_mode {
  * @IFLA_BR_FDB_MAX_LEARNED
  *   Set the number of max dynamically learned FDB entries for the current
  *   bridge.
+ *
+ * @IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE
+ *   Set how the bridge shall notify user space about MDB flag change via
+ *   RTM_NEWMDB netlink message.
+ *   The valid values are:
+ *
+ *     * 0 - the bridge will not notify user space about MDB flag change
+ *     * 1 - the bridge will notify user space about flag change if either
+ *           MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
+ *     * 2 - the bridge will notify user space about flag change only if
+ *           MDB_PG_FLAGS_OFFLOAD_FAILED has changed
+ *
+ *   The default value is 0.
  */
 enum {
 	IFLA_BR_UNSPEC,
@@ -793,6 +806,7 @@ enum {
 	IFLA_BR_MCAST_QUERIER_STATE,
 	IFLA_BR_FDB_N_LEARNED,
 	IFLA_BR_FDB_MAX_LEARNED,
+	IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE,
 	__IFLA_BR_MAX,
 };
 
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 3e0f47203f2a..e87d39b148d8 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1270,6 +1270,7 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
 		NLA_POLICY_EXACT_LEN(sizeof(struct br_boolopt_multi)),
 	[IFLA_BR_FDB_N_LEARNED] = { .type = NLA_REJECT },
 	[IFLA_BR_FDB_MAX_LEARNED] = { .type = NLA_U32 },
+	[IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE] = { .type = NLA_U8 },
 };
 
 static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
@@ -1514,6 +1515,18 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 			return err;
 	}
 #endif
+
+#ifdef CONFIG_NET_SWITCHDEV
+	if (data[IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE]) {
+		__u8 val;
+
+		val = nla_get_u8(data[IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE]);
+		err = br_multicast_set_mdb_notify_on_flag_change(&br->multicast_ctx,
+								 val);
+		if (err)
+			return err;
+	}
+#endif
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	if (data[IFLA_BR_NF_CALL_IPTABLES]) {
@@ -1625,6 +1638,9 @@ static size_t br_get_size(const struct net_device *brdev)
 	       nla_total_size(sizeof(u8)) +	/* IFLA_BR_MCAST_IGMP_VERSION */
 	       nla_total_size(sizeof(u8)) +	/* IFLA_BR_MCAST_MLD_VERSION */
 	       br_multicast_querier_state_size() + /* IFLA_BR_MCAST_QUERIER_STATE */
+#ifdef CONFIG_NET_SWITCHDEV
+	       nla_total_size(sizeof(u8)) +	/* IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE */
+#endif
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	       nla_total_size(sizeof(u8)) +     /* IFLA_BR_NF_CALL_IPTABLES */
@@ -1722,6 +1738,11 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 	if (nla_put_u8(skb, IFLA_BR_MCAST_MLD_VERSION,
 		       br->multicast_ctx.multicast_mld_version))
 		return -EMSGSIZE;
+#endif
+#ifdef CONFIG_NET_SWITCHDEV
+	if (nla_put_u8(skb, IFLA_BR_MDB_NOTIFY_ON_FLAG_CHANGE,
+		       br->multicast_ctx.multicast_mdb_notify_on_flag_change))
+		return -EMSGSIZE;
 #endif
 	clockval = jiffies_to_clock_t(br->multicast_ctx.multicast_last_member_interval);
 	if (nla_put_u64_64bit(skb, IFLA_BR_MCAST_LAST_MEMBER_INTVL, clockval,
-- 
2.49.0

Re: [Patch net-next 0/3] Add support for mdb offload failure notification

[Nikolay Aleksandrov]

On 3/19/25 00:42, Joseph Huang wrote:
> Currently the bridge does not provide real-time feedback to user space
> on whether or not an attempt to offload an mdb entry was successful.
> 
> This patch set adds support to notify user space about successful and
> failed offload attempts, and the behavior is controlled by a new knob
> mdb_notify_on_flag_change:
> 
> 0 - the bridge will not notify user space about MDB flag change
> 1 - the bridge will notify user space about flag change if either
>     MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
> 2 - the bridge will notify user space about flag change only if
>     MDB_PG_FLAGS_OFFLOAD_FAILED has changed
> 
> The default value is 0.
> 
> A break-down of the patches in the series:
> 
> Patch 1 adds offload failed flag to indicate that the offload attempt
> has failed. The flag is reflected in netlink mdb entry flags.
> 
> Patch 2 adds the knob mdb_notify_on_flag_change, and notify user space
> accordingly in br_switchdev_mdb_complete() when the result is known.
> 
> Patch 3 adds netlink interface to manipulate mdb_notify_on_flag_change
> knob.
> 
> This patch set was inspired by the patch series "Add support for route
> offload failure notifications" discussed here:
> https://lore.kernel.org/all/20210207082258.3872086-1-idosch@idosch.org/
> 
> Joseph Huang (3):
>   net: bridge: mcast: Add offload failed mdb flag
>   net: bridge: mcast: Notify on offload flag change
>   net: bridge: Add notify on flag change netlink i/f
> 
>  include/uapi/linux/if_bridge.h |  9 +++++----
>  include/uapi/linux/if_link.h   | 14 ++++++++++++++
>  net/bridge/br_mdb.c            | 30 +++++++++++++++++++++++++-----
>  net/bridge/br_multicast.c      | 25 +++++++++++++++++++++++++
>  net/bridge/br_netlink.c        | 21 +++++++++++++++++++++
>  net/bridge/br_private.h        | 26 +++++++++++++++++++++-----
>  net/bridge/br_switchdev.c      | 31 ++++++++++++++++++++++++++-----
>  7 files changed, 137 insertions(+), 19 deletions(-)
> 

Hi,
Could you please share more about the motivation - why do you need this and
what will be using it? Also why do you need an option with 3 different modes
instead of just an on/off switch for these notifications?

Thanks,
 Nik

Re: [Patch net-next 0/3] Add support for mdb offload failure notification

[Joseph Huang]

On 3/20/2025 2:17 AM, Nikolay Aleksandrov wrote:
> On 3/19/25 00:42, Joseph Huang wrote:
>> Currently the bridge does not provide real-time feedback to user space
>> on whether or not an attempt to offload an mdb entry was successful.
>>
>> This patch set adds support to notify user space about successful and
>> failed offload attempts, and the behavior is controlled by a new knob
>> mdb_notify_on_flag_change:
>>
>> 0 - the bridge will not notify user space about MDB flag change
>> 1 - the bridge will notify user space about flag change if either
>>      MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
>> 2 - the bridge will notify user space about flag change only if
>>      MDB_PG_FLAGS_OFFLOAD_FAILED has changed
>>
>> The default value is 0.
>>
>> A break-down of the patches in the series:
>>
>> Patch 1 adds offload failed flag to indicate that the offload attempt
>> has failed. The flag is reflected in netlink mdb entry flags.
>>
>> Patch 2 adds the knob mdb_notify_on_flag_change, and notify user space
>> accordingly in br_switchdev_mdb_complete() when the result is known.
>>
>> Patch 3 adds netlink interface to manipulate mdb_notify_on_flag_change
>> knob.
>>
>> This patch set was inspired by the patch series "Add support for route
>> offload failure notifications" discussed here:
>> https://lore.kernel.org/all/20210207082258.3872086-1-idosch@idosch.org/
>>
>> Joseph Huang (3):
>>    net: bridge: mcast: Add offload failed mdb flag
>>    net: bridge: mcast: Notify on offload flag change
>>    net: bridge: Add notify on flag change netlink i/f
>>
>>   include/uapi/linux/if_bridge.h |  9 +++++----
>>   include/uapi/linux/if_link.h   | 14 ++++++++++++++
>>   net/bridge/br_mdb.c            | 30 +++++++++++++++++++++++++-----
>>   net/bridge/br_multicast.c      | 25 +++++++++++++++++++++++++
>>   net/bridge/br_netlink.c        | 21 +++++++++++++++++++++
>>   net/bridge/br_private.h        | 26 +++++++++++++++++++++-----
>>   net/bridge/br_switchdev.c      | 31 ++++++++++++++++++++++++++-----
>>   7 files changed, 137 insertions(+), 19 deletions(-)
>>
> 
> Hi,
> Could you please share more about the motivation - why do you need this and
> what will be using it? 

Hi Nik,

The API for a user space application to join a multicast group is 
write-only (and really best-efforts only), meaning that after an 
application calls setsockopt(), the application has no way to know 
whether the operation actually succeeded or not. Normally for soft 
bridges this is not an issue; however for switchdev-backed bridges, due 
to limited hardware resources, the failure rate is meaningfully higher.

With this patch set, the user space application will now get a 
notification about a failed attempt to join a multicast group. The user 
space application can then have the opportunity to mitigate the failure 
[1][2].

> Also why do you need an option with 3 different modes
> instead of just an on/off switch for these notifications?
> 
> Thanks,
>   Nik
> 

Some user space application might be interested in both successful and 
failed offload attempts (for example the application might want to keep 
an mdb database which is perfectly in sync with the hardware), while 
some other user space application might only be interested in failed 
attempts (so that it can retry the operation or choose a different group 
for example).

This knob is modeled after fib_notify_on_flag_change knob on route 
offload failure notification (see 
https://lore.kernel.org/all/20210207082258.3872086-4-idosch@idosch.org/). 
The rationale is that "Separate value (read: 2) is added for such 
notifications because there are less of them, so they do not impact 
performance and some users will find them more important."

Thanks,
Joseph

--

[1] 
https://datatracker.ietf.org/doc/draft-ietf-pim-zeroconf-mcast-addr-alloc-ps/, 
section 2, the last paragraph
[2] 
https://datatracker.ietf.org/doc/draft-ietf-pim-ipv6-zeroconf-assignment/, 
section 2.1, the first paragraph

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

[Nikolay Aleksandrov]

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

Re: [Patch net-next 0/3] Add support for mdb offload failure notification

[Nikolay Aleksandrov]

On 3/20/25 23:14, Joseph Huang wrote:
> On 3/20/2025 2:17 AM, Nikolay Aleksandrov wrote:
>> On 3/19/25 00:42, Joseph Huang wrote:
>>> Currently the bridge does not provide real-time feedback to user space
>>> on whether or not an attempt to offload an mdb entry was successful.
>>>
>>> This patch set adds support to notify user space about successful and
>>> failed offload attempts, and the behavior is controlled by a new knob
>>> mdb_notify_on_flag_change:
>>>
>>> 0 - the bridge will not notify user space about MDB flag change
>>> 1 - the bridge will notify user space about flag change if either
>>>      MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
>>> 2 - the bridge will notify user space about flag change only if
>>>      MDB_PG_FLAGS_OFFLOAD_FAILED has changed
>>>
>>> The default value is 0.
>>>
>>> A break-down of the patches in the series:
>>>
>>> Patch 1 adds offload failed flag to indicate that the offload attempt
>>> has failed. The flag is reflected in netlink mdb entry flags.
>>>
>>> Patch 2 adds the knob mdb_notify_on_flag_change, and notify user space
>>> accordingly in br_switchdev_mdb_complete() when the result is known.
>>>
>>> Patch 3 adds netlink interface to manipulate mdb_notify_on_flag_change
>>> knob.
>>>
>>> This patch set was inspired by the patch series "Add support for route
>>> offload failure notifications" discussed here:
>>> https://lore.kernel.org/all/20210207082258.3872086-1-idosch@idosch.org/
>>>
>>> Joseph Huang (3):
>>>    net: bridge: mcast: Add offload failed mdb flag
>>>    net: bridge: mcast: Notify on offload flag change
>>>    net: bridge: Add notify on flag change netlink i/f
>>>
>>>   include/uapi/linux/if_bridge.h |  9 +++++----
>>>   include/uapi/linux/if_link.h   | 14 ++++++++++++++
>>>   net/bridge/br_mdb.c            | 30 +++++++++++++++++++++++++-----
>>>   net/bridge/br_multicast.c      | 25 +++++++++++++++++++++++++
>>>   net/bridge/br_netlink.c        | 21 +++++++++++++++++++++
>>>   net/bridge/br_private.h        | 26 +++++++++++++++++++++-----
>>>   net/bridge/br_switchdev.c      | 31 ++++++++++++++++++++++++++-----
>>>   7 files changed, 137 insertions(+), 19 deletions(-)
>>>
>>
>> Hi,
>> Could you please share more about the motivation - why do you need this and
>> what will be using it? 
> 
> Hi Nik,
> 
> The API for a user space application to join a multicast group is write-only (and really best-efforts only), meaning that after an application calls setsockopt(), the application has no way to know whether the operation actually succeeded or not. Normally for soft bridges this is not an issue; however for switchdev-backed bridges, due to limited hardware resources, the failure rate is meaningfully higher.
> 
> With this patch set, the user space application will now get a notification about a failed attempt to join a multicast group. The user space application can then have the opportunity to mitigate the failure [1][2].
> 

Thanks for the pointers.

>> Also why do you need an option with 3 different modes
>> instead of just an on/off switch for these notifications?
>>
>> Thanks,
>>   Nik
>>
> 
> Some user space application might be interested in both successful and failed offload attempts (for example the application might want to keep an mdb database which is perfectly in sync with the hardware), while some other user space application might only be interested in failed attempts (so that it can retry the operation or choose a different group for example).
> 
> This knob is modeled after fib_notify_on_flag_change knob on route offload failure notification (see https://lore.kernel.org/all/20210207082258.3872086-4-idosch@idosch.org/). The rationale is that "Separate value (read: 2) is added for such notifications because there are less of them, so they do not impact performance and some users will find them more important."
> 
> Thanks,
> Joseph
> 

Can we please not add features that don't have actual users? It seems you're interested
in failed attempts, so you can just add a bridge boolopt on/off switch to notify about
those events, if anyone becomes interested in all then we can extend it. Also it can
have a more specific name like mdb_offload_fail_notification instead, saying that we
notify on mdb flags change is misleading because there are more flags which can change.

Also please drop all of the switchdev ifdefs and just always have this option available
it will actually be used only with switchdev enabled so setting it in other
cases is a noop, these flags will never be seen anyway.
 
Cheers,
 Nik

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

[Nikolay Aleksandrov]

On 3/19/25 00:42, Joseph Huang wrote:
> Notify user space on offload flag(s) change.
> 
> This behavior is controlled by the new knob mdb_notify_on_flag_change:
> 
> 0 - the bridge will not notify user space about MDB flag change
> 1 - the bridge will notify user space about flag change if either
>     MDB_PG_FLAGS_OFFLOAD or MDB_PG_FLAGS_OFFLOAD_FAILED has changed
> 2 - the bridge will notify user space about flag change only if
>     MDB_PG_FLAGS_OFFLOAD_FAILED has changed
> 
> The default value is 0.
> 
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> ---
>  net/bridge/br_mdb.c       | 28 +++++++++++++++++++++++-----
>  net/bridge/br_multicast.c | 25 +++++++++++++++++++++++++
>  net/bridge/br_private.h   | 15 +++++++++++++++
>  net/bridge/br_switchdev.c | 25 +++++++++++++++++++++++--
>  4 files changed, 86 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 0639691cd19b..d206b5a160f3 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -519,16 +519,17 @@ static size_t rtnl_mdb_nlmsg_size(const struct net_bridge_port_group *pg)
>  	       rtnl_mdb_nlmsg_pg_size(pg);
>  }
>  
> -void br_mdb_notify(struct net_device *dev,
> -		   struct net_bridge_mdb_entry *mp,
> -		   struct net_bridge_port_group *pg,
> -		   int type)
> +static void _br_mdb_notify(struct net_device *dev,
> +			   struct net_bridge_mdb_entry *mp,
> +			   struct net_bridge_port_group *pg,
> +			   int type, bool notify_switchdev)

please use double underscore "__"

>  {
>  	struct net *net = dev_net(dev);
>  	struct sk_buff *skb;
>  	int err = -ENOBUFS;
>  
> -	br_switchdev_mdb_notify(dev, mp, pg, type);
> +	if (notify_switchdev)
> +		br_switchdev_mdb_notify(dev, mp, pg, type);
>  
>  	skb = nlmsg_new(rtnl_mdb_nlmsg_size(pg), GFP_ATOMIC);
>  	if (!skb)
> @@ -546,6 +547,23 @@ void br_mdb_notify(struct net_device *dev,
>  	rtnl_set_sk_err(net, RTNLGRP_MDB, err);
>  }
>  
> +void br_mdb_notify(struct net_device *dev,
> +		   struct net_bridge_mdb_entry *mp,
> +		   struct net_bridge_port_group *pg,
> +		   int type)
> +{
> +	_br_mdb_notify(dev, mp, pg, type, true);
> +}
> +
> +#ifdef CONFIG_NET_SWITCHDEV
> +void br_mdb_flag_change_notify(struct net_device *dev,
> +			       struct net_bridge_mdb_entry *mp,
> +			       struct net_bridge_port_group *pg)
> +{
> +	_br_mdb_notify(dev, mp, pg, RTM_NEWMDB, false);
> +}
> +#endif
> +
>  static int nlmsg_populate_rtr_fill(struct sk_buff *skb,
>  				   struct net_device *dev,
>  				   int ifindex, u16 vid, u32 pid,
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index b2ae0d2434d2..8d583caecd40 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -4051,6 +4051,10 @@ void br_multicast_ctx_init(struct net_bridge *br,
>  	brmctx->ip6_querier.port_ifidx = 0;
>  	seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock);
>  #endif
> +#ifdef CONFIG_NET_SWITCHDEV
> +	brmctx->multicast_mdb_notify_on_flag_change =
> +		MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE;
> +#endif
>  
>  	timer_setup(&brmctx->ip4_mc_router_timer,
>  		    br_ip4_multicast_local_router_expired, 0);
> @@ -4708,6 +4712,27 @@ int br_multicast_set_mld_version(struct net_bridge_mcast *brmctx,
>  }
>  #endif
>  
> +#ifdef CONFIG_NET_SWITCHDEV
> +int br_multicast_set_mdb_notify_on_flag_change(struct net_bridge_mcast *brmctx,
> +					       u8 val)
> +{
> +	switch (val) {
> +	case MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE:
> +	case MDB_NOTIFY_ON_FLAG_CHANGE_BOTH:
> +	case MDB_NOTIFY_ON_FLAG_CHANGE_FAIL_ONLY:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Please use NLA_POLICY_MAX() instead.

> +
> +	spin_lock_bh(&brmctx->br->multicast_lock);
> +	brmctx->multicast_mdb_notify_on_flag_change = val;
> +	spin_unlock_bh(&brmctx->br->multicast_lock);
> +
> +	return 0;
> +}
> +#endif
> +
>  void br_multicast_set_query_intvl(struct net_bridge_mcast *brmctx,
>  				  unsigned long val)
>  {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index cd6b4e91e7d6..8e8de5d54ae3 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -132,6 +132,10 @@ struct net_bridge_mcast_port {
>  #endif /* CONFIG_BRIDGE_IGMP_SNOOPING */
>  };
>  
> +#define MDB_NOTIFY_ON_FLAG_CHANGE_DISABLE	0
> +#define MDB_NOTIFY_ON_FLAG_CHANGE_BOTH		1
> +#define MDB_NOTIFY_ON_FLAG_CHANGE_FAIL_ONLY	2

This should be an enum and it is also uAPI, you should move it to 
include/uapi/linux/if_bridge.h

> +
>  /* net_bridge_mcast must be always defined due to forwarding stubs */
>  struct net_bridge_mcast {
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
> @@ -146,6 +150,9 @@ struct net_bridge_mcast {
>  	u8				multicast_router;
>  #if IS_ENABLED(CONFIG_IPV6)
>  	u8				multicast_mld_version;
> +#endif
> +#ifdef CONFIG_NET_SWITCHDEV
> +	u8				multicast_mdb_notify_on_flag_change;
>  #endif
>  	unsigned long			multicast_last_member_interval;
>  	unsigned long			multicast_membership_interval;
> @@ -988,6 +995,10 @@ int br_multicast_set_igmp_version(struct net_bridge_mcast *brmctx,
>  int br_multicast_set_mld_version(struct net_bridge_mcast *brmctx,
>  				 unsigned long val);
>  #endif
> +#ifdef CONFIG_NET_SWITCHDEV
> +int br_multicast_set_mdb_notify_on_flag_change(struct net_bridge_mcast *brmctx,
> +					       u8 val);
> +#endif
>  struct net_bridge_mdb_entry *
>  br_mdb_ip_get(struct net_bridge *br, struct br_ip *dst);
>  struct net_bridge_mdb_entry *
> @@ -1004,6 +1015,10 @@ int br_mdb_hash_init(struct net_bridge *br);
>  void br_mdb_hash_fini(struct net_bridge *br);
>  void br_mdb_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
>  		   struct net_bridge_port_group *pg, int type);
> +#ifdef CONFIG_NET_SWITCHDEV
> +void br_mdb_flag_change_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
> +			       struct net_bridge_port_group *pg);
> +#endif
>  void br_rtr_notify(struct net_device *dev, struct net_bridge_mcast_port *pmctx,
>  		   int type);
>  void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
> 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.

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

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

[Joseph Huang]

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

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

[Nikolay Aleksandrov]

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

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

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

[Nikolay Aleksandrov]

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

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

[Joseph Huang]

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.

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

+static void br_multicast_set_pg_offload_flags(int err,
+                                             struct 
net_bridge_port_group *p)
+{
+       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,
+                                      u8 old_flags, u8 new_flags)
+{
+       return (br_boolopt_get(br, 
BR_BOOLOPT_FAILED_OFFLOAD_NOTIFICATION) &&
+               ((old_flags & MDB_PG_FLAGS_OFFLOAD_FAILED) !=
+               (new_flags & MDB_PG_FLAGS_OFFLOAD_FAILED)));
+}
+
  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))
+                       br_mdb_flag_change_notify(br->dev, mp, p);
         }
  out:
         spin_unlock_bh(&br->multicast_lock);
-err:
         kfree(priv);
  }

Thanks,
Joseph

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

[Nikolay Aleksandrov]

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

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

[Nikolay Aleksandrov]

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
>