| Message ID | 0-v1-808b6da3bd3f+1857-cm_xarray_no_irq_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/cm: Make the local_id_table xarray non-irq | expand |
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
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
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
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
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);
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(-)