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.0On 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);
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
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
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
>
© 2016 - 2025 Red Hat, Inc.