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

[Nikolay Aleksandrov <razor@blackwall.org>]

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