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

[Vladimir Oltean <olteanv@gmail.com>]

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 <vladimir.oltean@nxp.com> wrote:
> > 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.

> > +
> > +	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.

> > @@ -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.