Re: [PATCH v2 net-next 1/5] net: switchdev: move SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE to the blocking notifier chain

Hi Vlad,

On Thu, Aug 19, 2021 at 09:15:17PM +0300, Vlad Buslov wrote:

On Thu 19 Aug 2021 at 19:07, Vladimir Oltean [off-list ref] wrote:

quoted

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
index 0c38c2e319be..ea7c3f07f6fe 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c

@@ -276,6 +276,55 @@ mlx5_esw_bridge_port_obj_attr_set(struct net_device *dev,
 	return err;
 }

+static struct mlx5_bridge_switchdev_fdb_work *
+mlx5_esw_bridge_init_switchdev_fdb_work(struct net_device *dev, bool add,
+					struct switchdev_notifier_fdb_info *fdb_info,
+					struct mlx5_esw_bridge_offloads *br_offloads);
+
+static int
+mlx5_esw_bridge_fdb_event(struct net_device *dev, unsigned long event,
+			  struct switchdev_notifier_info *info,
+			  struct mlx5_esw_bridge_offloads *br_offloads)
+{
+	struct switchdev_notifier_fdb_info *fdb_info;
+	struct mlx5_bridge_switchdev_fdb_work *work;
+	struct mlx5_eswitch *esw = br_offloads->esw;
+	u16 vport_num, esw_owner_vhca_id;
+	struct net_device *upper, *rep;
+
+	upper = netdev_master_upper_dev_get_rcu(dev);
+	if (!upper)
+		return 0;
+	if (!netif_is_bridge_master(upper))
+		return 0;
+
+	rep = mlx5_esw_bridge_rep_vport_num_vhca_id_get(dev, esw,
+							&vport_num,
+							&esw_owner_vhca_id);
+	if (!rep)
+		return 0;
+
+	/* only handle the event on peers */
+	if (mlx5_esw_bridge_is_local(dev, rep, esw))
+		return 0;

This check is only needed for SWITCHDEV_FDB_DEL_TO_BRIDGE case. Here it
breaks the offload.

Very good point, thanks for looking. I copied the entire atomic notifier
handler and deleted the code which wasn't needed, but I actually took a
break while converting mlx5, and so I forgot to delete this part when I
came back.

quoted

+
+	fdb_info = container_of(info, struct switchdev_notifier_fdb_info, info);
+
+	work = mlx5_esw_bridge_init_switchdev_fdb_work(dev,
+						       event == SWITCHDEV_FDB_ADD_TO_DEVICE,
+						       fdb_info,

Here FDB info can already be deallocated[1] since this is now executing
asynchronously and races with fdb_rcu_free() that is scheduled to be
called after rcu grace period by fdb_delete().

I am incredibly lucky that you caught this, apparently I needed to add
an msleep(1000) to see it as well.

It is not the struct switchdev_notifier_fdb_info *fdb_info that gets
freed under RCU. It is fdb_info->addr (the MAC address), since
switchdev_deferred_enqueue only performs a shallow copy. I will address
that in v3.

quoted

@@ -415,9 +470,7 @@ static int mlx5_esw_bridge_switchdev_event(struct notifier_block *nb,
 		/* only handle the event on peers */
 		if (mlx5_esw_bridge_is_local(dev, rep, esw))
 			break;

I really like the idea of completely remove the driver wq from FDB
handling code, but I'm not yet too familiar with bridge internals to
easily determine whether same approach can be applied to
SWITCHDEV_FDB_{ADD|DEL}_TO_BRIDGE event after this series is accepted.
It seems that all current users already generate these events from
blocking context, so would it be a trivial change for me to do in your
opinion? That would allow me to get rid of mlx5_esw_bridge_offloads->wq
in our driver.

If all callers really are in blocking context (and they do appear to be)
you can even forgo the switchdev_deferred_enqueue that switchdev_fdb_add_to_device
does, and just call_switchdev_blocking_notifiers() directly. Then you
move the bridge handler from br_switchdev_event() to br_switchdev_blocking_event().
It should be even simpler than this conversion.