Notify user space on mdb offload failure if mdb_offload_fail_notification
is set.
Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
net/bridge/br_mdb.c | 26 +++++++++++++++++++++-----
net/bridge/br_private.h | 9 +++++++++
net/bridge/br_switchdev.c | 4 ++++
3 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 0639691cd19b..5f53f387d251 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,21 @@ 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);
+}
+
+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);
+}
+
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_private.h b/net/bridge/br_private.h
index 02188b7ff8e6..fc43ccc06ccb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1005,6 +1005,8 @@ 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);
+void br_mdb_flag_change_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
+ struct net_bridge_port_group *pg);
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,
@@ -1354,6 +1356,13 @@ br_multicast_set_pg_offload_flags(struct net_bridge_port_group *p,
p->flags |= (offloaded ? MDB_PG_FLAGS_OFFLOAD :
MDB_PG_FLAGS_OFFLOAD_FAILED);
}
+
+static inline bool
+br_mdb_should_notify(const struct net_bridge *br, u8 changed_flags)
+{
+ return br_opt_get(br, BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION) &&
+ (changed_flags & MDB_PG_FLAGS_OFFLOAD_FAILED);
+}
#else
static inline int br_multicast_rcv(struct net_bridge_mcast **brmctx,
struct net_bridge_mcast_port **pmctx,
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 40f0b16e4df8..9b5005d0742a 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -504,6 +504,7 @@ 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;
+ u8 old_flags;
spin_lock_bh(&br->multicast_lock);
mp = br_mdb_ip_get(br, &data->ip);
@@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
if (p->key.port != port)
continue;
+ old_flags = p->flags;
br_multicast_set_pg_offload_flags(p, !err);
+ if (br_mdb_should_notify(br, old_flags ^ p->flags))
+ br_mdb_flag_change_notify(br->dev, mp, p);
}
out:
spin_unlock_bh(&br->multicast_lock);
--
2.49.0On 4/4/25 02:44, Joseph Huang wrote:
> Notify user space on mdb offload failure if mdb_offload_fail_notification
> is set.
>
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> ---
> net/bridge/br_mdb.c | 26 +++++++++++++++++++++-----
> net/bridge/br_private.h | 9 +++++++++
> net/bridge/br_switchdev.c | 4 ++++
> 3 files changed, 34 insertions(+), 5 deletions(-)
>
The patch looks good, but one question - it seems we'll mark mdb entries with
"offload failed" when we get -EOPNOTSUPP as an error as well. Is that intended?
That is, if the option is enabled and we have mixed bridge ports, we'll mark mdbs
to the non-switch ports as offload failed, but it is not due to a switch offload
error.
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 0639691cd19b..5f53f387d251 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,21 @@ 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);
> +}
> +
> +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);
> +}
> +
> 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_private.h b/net/bridge/br_private.h
> index 02188b7ff8e6..fc43ccc06ccb 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1005,6 +1005,8 @@ 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);
> +void br_mdb_flag_change_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
> + struct net_bridge_port_group *pg);
> 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,
> @@ -1354,6 +1356,13 @@ br_multicast_set_pg_offload_flags(struct net_bridge_port_group *p,
> p->flags |= (offloaded ? MDB_PG_FLAGS_OFFLOAD :
> MDB_PG_FLAGS_OFFLOAD_FAILED);
> }
> +
> +static inline bool
> +br_mdb_should_notify(const struct net_bridge *br, u8 changed_flags)
> +{
> + return br_opt_get(br, BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION) &&
> + (changed_flags & MDB_PG_FLAGS_OFFLOAD_FAILED);
> +}
> #else
> static inline int br_multicast_rcv(struct net_bridge_mcast **brmctx,
> struct net_bridge_mcast_port **pmctx,
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 40f0b16e4df8..9b5005d0742a 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -504,6 +504,7 @@ 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;
> + u8 old_flags;
>
> spin_lock_bh(&br->multicast_lock);
> mp = br_mdb_ip_get(br, &data->ip);
> @@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
> if (p->key.port != port)
> continue;
>
> + old_flags = p->flags;
> br_multicast_set_pg_offload_flags(p, !err);
> + if (br_mdb_should_notify(br, old_flags ^ p->flags))
> + br_mdb_flag_change_notify(br->dev, mp, p);
> }
> out:
> spin_unlock_bh(&br->multicast_lock);
On 4/4/2025 6:29 AM, Nikolay Aleksandrov wrote: > On 4/4/25 02:44, Joseph Huang wrote: >> Notify user space on mdb offload failure if mdb_offload_fail_notification >> is set. >> >> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com> >> --- >> net/bridge/br_mdb.c | 26 +++++++++++++++++++++----- >> net/bridge/br_private.h | 9 +++++++++ >> net/bridge/br_switchdev.c | 4 ++++ >> 3 files changed, 34 insertions(+), 5 deletions(-) >> > > The patch looks good, but one question - it seems we'll mark mdb entries with > "offload failed" when we get -EOPNOTSUPP as an error as well. Is that intended? > > That is, if the option is enabled and we have mixed bridge ports, we'll mark mdbs > to the non-switch ports as offload failed, but it is not due to a switch offload > error. Good catch. No, that was not intended. What if we short-circuit and just return like you'd suggested initially if err == -EOPNOTSUPP? >> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >> index 40f0b16e4df8..9b5005d0742a 100644 >> --- a/net/bridge/br_switchdev.c >> +++ b/net/bridge/br_switchdev.c >> @@ -504,6 +504,7 @@ 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; >> + u8 old_flags; >> + if (err == -EOPNOTSUPP) + goto notsupp; >> spin_lock_bh(&br->multicast_lock); >> mp = br_mdb_ip_get(br, &data->ip); >> @@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri >> if (p->key.port != port) >> continue; >> >> + old_flags = p->flags; >> br_multicast_set_pg_offload_flags(p, !err); >> + if (br_mdb_should_notify(br, old_flags ^ p->flags)) >> + br_mdb_flag_change_notify(br->dev, mp, p); >> } >> out: >> spin_unlock_bh(&br->multicast_lock); > + notsupp: kfree(priv);
On 4/4/25 18:25, Joseph Huang wrote: > On 4/4/2025 6:29 AM, Nikolay Aleksandrov wrote: >> On 4/4/25 02:44, Joseph Huang wrote: >>> Notify user space on mdb offload failure if mdb_offload_fail_notification >>> is set. >>> >>> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com> >>> --- >>> net/bridge/br_mdb.c | 26 +++++++++++++++++++++----- >>> net/bridge/br_private.h | 9 +++++++++ >>> net/bridge/br_switchdev.c | 4 ++++ >>> 3 files changed, 34 insertions(+), 5 deletions(-) >>> >> >> The patch looks good, but one question - it seems we'll mark mdb entries with >> "offload failed" when we get -EOPNOTSUPP as an error as well. Is that intended? >> >> That is, if the option is enabled and we have mixed bridge ports, we'll mark mdbs >> to the non-switch ports as offload failed, but it is not due to a switch offload >> error. > > Good catch. No, that was not intended. > > What if we short-circuit and just return like you'd suggested initially if err == -EOPNOTSUPP? > >>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >>> index 40f0b16e4df8..9b5005d0742a 100644 >>> --- a/net/bridge/br_switchdev.c >>> +++ b/net/bridge/br_switchdev.c >>> @@ -504,6 +504,7 @@ 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; >>> + u8 old_flags; >>> > > + if (err == -EOPNOTSUPP) > + goto notsupp; > >>> spin_lock_bh(&br->multicast_lock); >>> mp = br_mdb_ip_get(br, &data->ip); >>> @@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri >>> if (p->key.port != port) >>> continue; >>> + old_flags = p->flags; >>> br_multicast_set_pg_offload_flags(p, !err); >>> + if (br_mdb_should_notify(br, old_flags ^ p->flags)) >>> + br_mdb_flag_change_notify(br->dev, mp, p); >>> } >>> out: >>> spin_unlock_bh(&br->multicast_lock); >> > > + notsupp: > kfree(priv); Looks good to me. Thanks!
On 4/4/2025 4:04 PM, Nikolay Aleksandrov wrote: > On 4/4/25 18:25, Joseph Huang wrote: >> On 4/4/2025 6:29 AM, Nikolay Aleksandrov wrote: >>> On 4/4/25 02:44, Joseph Huang wrote: >>>> Notify user space on mdb offload failure if mdb_offload_fail_notification >>>> is set. >>>> >>>> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com> >>>> --- >>>> net/bridge/br_mdb.c | 26 +++++++++++++++++++++----- >>>> net/bridge/br_private.h | 9 +++++++++ >>>> net/bridge/br_switchdev.c | 4 ++++ >>>> 3 files changed, 34 insertions(+), 5 deletions(-) >>>> >>> >>> The patch looks good, but one question - it seems we'll mark mdb entries with >>> "offload failed" when we get -EOPNOTSUPP as an error as well. Is that intended? >>> >>> That is, if the option is enabled and we have mixed bridge ports, we'll mark mdbs >>> to the non-switch ports as offload failed, but it is not due to a switch offload >>> error. >> >> Good catch. No, that was not intended. >> >> What if we short-circuit and just return like you'd suggested initially if err == -EOPNOTSUPP? >> >>>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >>>> index 40f0b16e4df8..9b5005d0742a 100644 >>>> --- a/net/bridge/br_switchdev.c >>>> +++ b/net/bridge/br_switchdev.c >>>> @@ -504,6 +504,7 @@ 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; >>>> + u8 old_flags; >>>> >> >> + if (err == -EOPNOTSUPP) >> + goto notsupp; >> >>>> spin_lock_bh(&br->multicast_lock); >>>> mp = br_mdb_ip_get(br, &data->ip); >>>> @@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri >>>> if (p->key.port != port) >>>> continue; >>>> + old_flags = p->flags; >>>> br_multicast_set_pg_offload_flags(p, !err); >>>> + if (br_mdb_should_notify(br, old_flags ^ p->flags)) >>>> + br_mdb_flag_change_notify(br->dev, mp, p); >>>> } >>>> out: >>>> spin_unlock_bh(&br->multicast_lock); >>> >> >> + notsupp: >> kfree(priv); > > Looks good to me. Thanks! Thanks for the review! And a logistic question. Now that part 1 and part 2 are ack'd (thanks again for the review), when I send out v3, should I resend those (unmodified part 1 and part 2) with my v3 patch series, or should I break this one off and only send part 3 v3 as a separate patch? Thanks, Joseph
On 4/4/25 23:11, Joseph Huang wrote: > On 4/4/2025 4:04 PM, Nikolay Aleksandrov wrote: >> On 4/4/25 18:25, Joseph Huang wrote: >>> On 4/4/2025 6:29 AM, Nikolay Aleksandrov wrote: >>>> On 4/4/25 02:44, Joseph Huang wrote: >>>>> Notify user space on mdb offload failure if mdb_offload_fail_notification >>>>> is set. >>>>> >>>>> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com> >>>>> --- >>>>> net/bridge/br_mdb.c | 26 +++++++++++++++++++++----- >>>>> net/bridge/br_private.h | 9 +++++++++ >>>>> net/bridge/br_switchdev.c | 4 ++++ >>>>> 3 files changed, 34 insertions(+), 5 deletions(-) >>>>> >>>> >>>> The patch looks good, but one question - it seems we'll mark mdb entries with >>>> "offload failed" when we get -EOPNOTSUPP as an error as well. Is that intended? >>>> >>>> That is, if the option is enabled and we have mixed bridge ports, we'll mark mdbs >>>> to the non-switch ports as offload failed, but it is not due to a switch offload >>>> error. >>> >>> Good catch. No, that was not intended. >>> >>> What if we short-circuit and just return like you'd suggested initially if err == -EOPNOTSUPP? >>> >>>>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >>>>> index 40f0b16e4df8..9b5005d0742a 100644 >>>>> --- a/net/bridge/br_switchdev.c >>>>> +++ b/net/bridge/br_switchdev.c >>>>> @@ -504,6 +504,7 @@ 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; >>>>> + u8 old_flags; >>>>> >>> >>> + if (err == -EOPNOTSUPP) >>> + goto notsupp; >>> >>>>> spin_lock_bh(&br->multicast_lock); >>>>> mp = br_mdb_ip_get(br, &data->ip); >>>>> @@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri >>>>> if (p->key.port != port) >>>>> continue; >>>>> + old_flags = p->flags; >>>>> br_multicast_set_pg_offload_flags(p, !err); >>>>> + if (br_mdb_should_notify(br, old_flags ^ p->flags)) >>>>> + br_mdb_flag_change_notify(br->dev, mp, p); >>>>> } >>>>> out: >>>>> spin_unlock_bh(&br->multicast_lock); >>>> >>> >>> + notsupp: >>> kfree(priv); >> >> Looks good to me. Thanks! > > Thanks for the review! > > And a logistic question. Now that part 1 and part 2 are ack'd (thanks again for the review), when I send out v3, should I resend those (unmodified part 1 and part 2) with my v3 patch series, or should I break this one off and only send part 3 v3 as a separate patch? > > Thanks, > Joseph You should send the whole set again as v3, but you should keep the acked-by tags in those patches. Cheers, Nik
© 2016 - 2026 Red Hat, Inc.