Re: [PATCH rdma-rc v1] RDMA/mlx5: Use xa_lock_irq when access to SRQ table

[Leon Romanovsky <leon@kernel.org>]

On Tue, Jul 07, 2020 at 02:42:58PM +0100, Matthew Wilcox wrote:
> On Tue, Jul 07, 2020 at 04:15:51PM +0300, Leon Romanovsky wrote:
> > +++ b/drivers/infiniband/hw/mlx5/srq_cmd.c
> > @@ -83,11 +83,11 @@ struct mlx5_core_srq *mlx5_cmd_get_srq(struct mlx5_ib_dev *dev, u32 srqn)
> >  	struct mlx5_srq_table *table = &dev->srq_table;
> >  	struct mlx5_core_srq *srq;
> >
> > -	xa_lock(&table->array);
> > +	xa_lock_irq(&table->array);
> >  	srq = xa_load(&table->array, srqn);
> >  	if (srq)
> >  		refcount_inc(&srq->common.refcount);
> > -	xa_unlock(&table->array);
> > +	xa_unlock_irq(&table->array);
>
> This one is correct.
>
> > @@ -655,11 +655,11 @@ static int srq_event_notifier(struct notifier_block *nb,
> >  	eqe = data;
> >  	srqn = be32_to_cpu(eqe->data.qp_srq.qp_srq_n) & 0xffffff;
> >
> > -	xa_lock(&table->array);
> > +	xa_lock_irq(&table->array);
> >  	srq = xa_load(&table->array, srqn);
> >  	if (srq)
> >  		refcount_inc(&srq->common.refcount);
> > -	xa_unlock(&table->array);
> > +	xa_unlock_irq(&table->array);
>
> This one is not.  srq_event_notifier() is called in irq context, always,
> so leave it as xa_lock() / xa_unlock().
>
> You could switch to a less-locked model, which would look something like this:
>
> 	rcu_read_lock();
> 	srq = xa_load(&table->array, srqn);
> 	if (srq && !refcount_inc_not_zero(&srq->common.refcount))
> 		srq = NULL;
> 	rcu_read_unlock();
>
> Then you wouldn't need to disable irqs while accessing the array.
> But you would need to rcu-free the srqs.

Thanks for your feedback,
At this point of time, we don't need rcu_* optimization, it is not
bottleneck yet :).

Thanks