[PATCH] RDMA/cm: Make the local_id_table xarray non-irq

[PATCH] RDMA/cm: Make the local_id_table xarray non-irq

[Jason Gunthorpe]

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@infradead.org>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 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
--- a/drivers/infiniband/core/cm.c
+++ 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);
 }
 
 struct ib_cm_id *ib_create_cm_id(struct ib_device *device,
@@ -1169,7 +1169,7 @@ static void cm_destroy_id(struct ib_cm_id *cm_id, int err)
 	spin_unlock(&cm.lock);
 	spin_unlock_irq(&cm_id_priv->lock);
 
-	xa_erase_irq(&cm.local_id_table, cm_local_id(cm_id->local_id));
+	xa_erase(&cm.local_id_table, cm_local_id(cm_id->local_id));
 	cm_deref_id(cm_id_priv);
 	wait_for_completion(&cm_id_priv->comp);
 	while ((work = cm_dequeue_work(cm_id_priv)) != NULL)
@@ -4482,7 +4482,7 @@ static int __init ib_cm_init(void)
 	cm.remote_id_table = RB_ROOT;
 	cm.remote_qp_table = RB_ROOT;
 	cm.remote_sidr_table = RB_ROOT;
-	xa_init_flags(&cm.local_id_table, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
+	xa_init_flags(&cm.local_id_table, XA_FLAGS_ALLOC);
 	get_random_bytes(&cm.random_id_operand, sizeof cm.random_id_operand);
 	INIT_LIST_HEAD(&cm.timewait_list);
 
-- 
2.28.0

Re: [PATCH] RDMA/cm: Make the local_id_table xarray non-irq

[Leon Romanovsky]

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@infradead.org>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  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
> --- a/drivers/infiniband/core/cm.c
> +++ 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?

Thanks

Re: [PATCH] RDMA/cm: Make the local_id_table xarray non-irq

[Jason Gunthorpe]

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@infradead.org>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  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.

Jason

Re: [PATCH] RDMA/cm: Make the local_id_table xarray non-irq

[Leon Romanovsky]

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@infradead.org>
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  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

Re: [PATCH] RDMA/cm: Make the local_id_table xarray non-irq

[Jason Gunthorpe]

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@infradead.org>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/infiniband/core/cm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Applied to for-rc, thanks

Jason