On Thu, Nov 05, 2020 at 11:15:22AM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 05, 2020 at 10:52:31AM +0200, Leon Romanovsky wrote:
> > On Wed, Nov 04, 2020 at 05:40:59PM -0400, Jason Gunthorpe wrote:
> > > The xarray is never mutated from an IRQ handler, only from work queues
> > > under a spinlock_irq. Thus there is no reason for it be an IRQ type
> > > xarray.
> > >
> > > This was copied over from the original IDR code, but the recent rework put
> > > the xarray inside another spinlock_irq which will unbalance the unlocking.
> > >
> > > Fixes: c206f8bad15d ("RDMA/cm: Make it clearer how concurrency works in cm_req_handler()")
> > > Reported-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > drivers/infiniband/core/cm.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > > index 0201364974594f..167e436ae11ded 100644
> > > +++ b/drivers/infiniband/core/cm.c
> > > @@ -859,8 +859,8 @@ static struct cm_id_private *cm_alloc_id_priv(struct ib_device *device,
> > > atomic_set(&cm_id_priv->work_count, -1);
> > > refcount_set(&cm_id_priv->refcount, 1);
> > >
> > > - ret = xa_alloc_cyclic_irq(&cm.local_id_table, &id, NULL, xa_limit_32b,
> > > - &cm.local_id_next, GFP_KERNEL);
> > > + ret = xa_alloc_cyclic(&cm.local_id_table, &id, NULL, xa_limit_32b,
> > > + &cm.local_id_next, GFP_KERNEL);
> > > if (ret < 0)
> > > goto error;
> > > cm_id_priv->id.local_id = (__force __be32)id ^ cm.random_id_operand;
> > > @@ -878,8 +878,8 @@ static struct cm_id_private *cm_alloc_id_priv(struct ib_device *device,
> > > */
> > > static void cm_finalize_id(struct cm_id_private *cm_id_priv)
> > > {
> > > - xa_store_irq(&cm.local_id_table, cm_local_id(cm_id_priv->id.local_id),
> > > - cm_id_priv, GFP_KERNEL);
> > > + xa_store(&cm.local_id_table, cm_local_id(cm_id_priv->id.local_id),
> > > + cm_id_priv, GFP_ATOMIC);
> > > }
> >
> > I see that in the ib_create_cm_id() function, we call to cm_finalize_id(),
> > won't it be a problem to do it without irq lock?
>
> The _irq or _bh notations are only needed if some place acquires the
> internal spinlock from a bh (timer, tasklet, etc) or irq.
>
> Since all the places working with local_id_table are obviously in
> contexts that can do GFP_KERNEL allocations I conclude a normal
> spinlock is fine.
I see, Thanks
>
> Jason
