Re: [Patch v2 net-next 3/3] net: bridge: mcast: Notify on mdb offload failure

[Nikolay Aleksandrov]

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@xxxxxxxxxx>
> ---
>  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);