On Tue, Feb 23, 2021 at 08:02:36PM +0200, Vladimir Oltean wrote:
quoted
On Tue, Feb 23, 2021 at 07:56:22PM +0200, Ido Schimmel wrote:
quoted
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.
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.