Re: Timing of host-joined bridge multicast groups with switchdev

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Feb 23, 2021 at 09:29:09PM +0200, Ido Schimmel wrote:
> On Tue, Feb 23, 2021 at 08:02:36PM +0200, Vladimir Oltean wrote:
> > On Tue, Feb 23, 2021 at 07:56:22PM +0200, Ido Schimmel wrote:
> > > For route offload you get a dump of all the existing routes when you
> > > register your notifier. It's a bit different with bridge because you
> > > don't care about existing bridges when you just initialize your driver.
> > >
> > > We had a similar issue with VXLAN because its FDB can be populated and
> > > only then attached to a bridge that you offload. Check
> > > vxlan_fdb_replay(). Probably need to introduce something similar for
> > > FDB/MDB entries.
> >
> > So you would be in favor of a driver-voluntary 'pull' type of approach
> > at bridge join, instead of the bridge 'pushing' the addresses?
> >
> > That's all fine, except when we'll have more than 3 switchdev drivers,
> > how do we expect to manage all this complexity duplicated in many places
> > in the kernel, instead of having it in a central place? Are there corner
> > cases I'm missing which make the 'push' approach impractical?
>
> Not sure. It needs to be scheduled when the driver is ready to handle
> it. In br_add_if() after netdev_master_upper_dev_link() is probably a
> good place.
>
> It also needs to be done once and not every time another port joins the
> bridge. This can be done using the port's parent ID, similar to what we
> are already doing with the offload forward mark in
> nbp_switchdev_mark_set().
>
> But I'm not sure how we replay it only for a single notifier block. I'm
> not familiar with setups where you have more than one listener let alone
> more than one that is interested in notifications from a specific
> bridge, so maybe it is OK to just replay it for all the listeners. But I
> would prefer to avoid it if we can.

At least with a driver-initiated pull, this seems to work:

-----------------------------[ cut here ]-----------------------------
From 13cb5ccbe35f64cfabe7dea3f76c8bc778cff9dc Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 23 Feb 2021 21:45:08 +0200
Subject: [PATCH] net: bridge: add a function that replays port and host-joined
 mdb entries

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/if_bridge.h |   3 +
 include/net/switchdev.h   |   1 +
 net/bridge/br_mdb.c       | 115 ++++++++++++++++++++++++++++++++++++++
 net/dsa/slave.c           |   1 +
 4 files changed, 120 insertions(+)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index b979005ea39c..d1190e2984bc 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -95,6 +95,9 @@ static inline bool br_multicast_router(const struct net_device *dev)
 }
 #endif
 
+int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,
+		  struct notifier_block *nb);
+
 #if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING)
 bool br_vlan_enabled(const struct net_device *dev);
 int br_vlan_get_pvid(const struct net_device *dev, u16 *p_pvid);
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index b7fc7d0f54e2..8c3218177136 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -68,6 +68,7 @@ enum switchdev_obj_id {
 };
 
 struct switchdev_obj {
+	struct list_head list;
 	struct net_device *orig_dev;
 	enum switchdev_obj_id id;
 	u32 flags;
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 8846c5bcd075..72978c881e11 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -235,6 +235,121 @@ static int __mdb_fill_info(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
+static int br_mdb_replay_one(struct notifier_block *nb, struct net_device *dev,
+			     struct switchdev_obj_port_mdb *mdb)
+{
+	struct switchdev_notifier_port_obj_info obj_info = {
+		.info = {
+			.dev = dev,
+		},
+		.obj = &mdb->obj,
+	};
+	int err;
+
+	err = nb->notifier_call(nb, SWITCHDEV_PORT_OBJ_ADD, &obj_info);
+	return notifier_to_errno(err);
+}
+
+static int br_mdb_queue_one(struct list_head *mdb_list,
+			    enum switchdev_obj_id id,
+			    struct net_bridge_mdb_entry *mp,
+			    struct net_device *orig_dev)
+{
+	struct switchdev_obj_port_mdb *mdb;
+
+	mdb = kzalloc(sizeof(*mdb), GFP_ATOMIC);
+	if (!mdb)
+		return -ENOMEM;
+
+	mdb->obj.id = id;
+	mdb->obj.orig_dev = orig_dev;
+	mdb->vid = mp->addr.vid;
+
+	if (mp->addr.proto == htons(ETH_P_IP))
+		ip_eth_mc_map(mp->addr.dst.ip4, mdb->addr);
+#if IS_ENABLED(CONFIG_IPV6)
+	else
+		ipv6_eth_mc_map(&mp->addr.dst.ip6, mdb->addr);
+#endif
+
+	list_add_tail(&mdb->obj.list, mdb_list);
+
+	return 0;
+}
+
+int br_mdb_replay(struct net_device *br_dev, struct net_device *dev,
+		  struct notifier_block *nb)
+{
+	struct net_bridge_mdb_entry *mp;
+	struct switchdev_obj *obj, *tmp;
+	struct list_head mdb_list;
+	struct net_bridge *br;
+	int err = 0;
+
+	ASSERT_RTNL();
+
+	INIT_LIST_HEAD(&mdb_list);
+
+	if (!netif_is_bridge_master(br_dev))
+		return -EINVAL;
+
+	if (!netif_is_bridge_port(dev))
+		return -EINVAL;
+
+	br = netdev_priv(br_dev);
+
+	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
+		return 0;
+
+	rcu_read_lock();
+
+	hlist_for_each_entry_rcu(mp, &br->mdb_list, mdb_node) {
+		struct net_bridge_port_group __rcu **pp;
+		struct net_bridge_port_group *p;
+
+		if (mp->host_joined) {
+			err = br_mdb_queue_one(&mdb_list,
+					       SWITCHDEV_OBJ_ID_HOST_MDB,
+					       mp, br_dev);
+			if (err) {
+				rcu_read_unlock();
+				goto out_free_mdb;
+			}
+		}
+
+		for (pp = &mp->ports; (p = rcu_dereference(*pp)) != NULL;
+		     pp = &p->next) {
+			if (p->key.port->dev != dev)
+				continue;
+
+			err = br_mdb_queue_one(&mdb_list,
+					       SWITCHDEV_OBJ_ID_PORT_MDB,
+					       mp, dev);
+			if (err) {
+				rcu_read_unlock();
+				goto out_free_mdb;
+			}
+		}
+	}
+
+	rcu_read_unlock();
+
+	list_for_each_entry(obj, &mdb_list, list) {
+		err = br_mdb_replay_one(nb, dev, SWITCHDEV_OBJ_PORT_MDB(obj));
+		if (err)
+			goto out_free_mdb;
+	}
+
+out_free_mdb:
+	list_for_each_entry_safe(obj, tmp, &mdb_list, list) {
+		list_del(&obj->list);
+		kfree(SWITCHDEV_OBJ_PORT_MDB(obj));
+	}
+
+	return err;
+}
+EXPORT_SYMBOL(br_mdb_replay);
+
 static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,
 			    struct net_device *dev)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 43e403ac70d5..9052ff5efab7 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2070,6 +2070,7 @@ static int dsa_slave_changeupper(struct net_device *dev,
 			err = dsa_port_bridge_join(dp, info->upper_dev);
 			if (!err)
 				dsa_bridge_mtu_normalization(dp);
+			err = br_mdb_replay(info->upper_dev, dev, &dsa_slave_switchdev_blocking_notifier);
 			err = notifier_from_errno(err);
 		} else {
 			dsa_port_bridge_leave(dp, info->upper_dev);
-- 
2.25.1

-----------------------------[ cut here ]-----------------------------

I am just not sure why I need to emit the notification only once per
ASIC. Currently, SWITCHDEV_OBJ_ID_HOST_MDB is emitted for all bridge
ports, so the callers need to reference-count it anyway. As for the port
mdb entries, I am filtering the entries towards just a single port when
that joins. So I think this is okay.