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

[Vlad Buslov <vladbu@nvidia.com>]

On Fri 20 Aug 2021 at 02:18, Vladimir Oltean <olteanv@gmail.com> wrote:
> 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.

Thanks for your advice! I'll start looking into it as soon as this
series is accepted.