* [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks
@ 2020-11-04 2:32 xiaofeng.yan
2020-11-04 2:32 ` [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed xiaofeng.yan
2020-11-04 2:36 ` [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks Matthew Wilcox
0 siblings, 2 replies; 6+ messages in thread
From: xiaofeng.yan @ 2020-11-04 2:32 UTC (permalink / raw)
To: willy, linux-fsdevel, linux-kernel, dledford, jgg, oulijun,
yanxiaofeng7, xiaofeng.yan2012
From: "xiaofeng.yan" <yanxiaofeng7@jd.com>
function xa_store_irq() has a spinlock as follows:
xa_lock_irq()
-->spin_lock_irq(&(xa)->xa_lock)
GFP_KERNEL flag could cause sleep.
So change GFP_KERNEL to GFP_ATOMIC and Romve "gfp_t gfp" in function
static inline void *xa_store_irq(struct xarray *xa, unsigned long index,
void *entry, gfp_t gfp)
Signed-off-by: xiaofeng.yan <yanxiaofeng7@jd.com>
---
include/linux/xarray.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index 92c0160b3352..aeaf97d5642f 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -595,7 +595,6 @@ static inline void *xa_store_bh(struct xarray *xa, unsigned long index,
* @xa: XArray.
* @index: Index into array.
* @entry: New entry.
- * @gfp: Memory allocation flags.
*
* This function is like calling xa_store() except it disables interrupts
* while holding the array lock.
@@ -605,12 +604,12 @@ static inline void *xa_store_bh(struct xarray *xa, unsigned long index,
* Return: The old entry at this index or xa_err() if an error happened.
*/
static inline void *xa_store_irq(struct xarray *xa, unsigned long index,
- void *entry, gfp_t gfp)
+ void *entry)
{
void *curr;
xa_lock_irq(xa);
- curr = __xa_store(xa, index, entry, gfp);
+ curr = __xa_store(xa, index, entry, GFP_ATOMIC);
xa_unlock_irq(xa);
return curr;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed 2020-11-04 2:32 [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks xiaofeng.yan @ 2020-11-04 2:32 ` xiaofeng.yan 2020-11-04 18:58 ` Jason Gunthorpe 2020-11-04 2:36 ` [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks Matthew Wilcox 1 sibling, 1 reply; 6+ messages in thread From: xiaofeng.yan @ 2020-11-04 2:32 UTC (permalink / raw) To: willy, linux-fsdevel, linux-kernel, dledford, jgg, oulijun, yanxiaofeng7, xiaofeng.yan2012 From: "xiaofeng.yan" <yanxiaofeng7@jd.com> function xa_store_irq() has three parameters because of removing patameter "gfp_t gfp" Signed-off-by: xiaofeng.yan <yanxiaofeng7@jd.com> --- drivers/infiniband/core/cm.c | 2 +- drivers/infiniband/hw/hns/hns_roce_qp.c | 2 +- drivers/infiniband/hw/mlx5/srq_cmd.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index 5740d1ba3568..afcb5711270b 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -879,7 +879,7 @@ 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); + cm_id_priv); } struct ib_cm_id *ib_create_cm_id(struct ib_device *device, diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c index 6c081dd985fc..1876a51f9e08 100644 --- a/drivers/infiniband/hw/hns/hns_roce_qp.c +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c @@ -237,7 +237,7 @@ static int hns_roce_qp_store(struct hns_roce_dev *hr_dev, if (!hr_qp->qpn) return -EINVAL; - ret = xa_err(xa_store_irq(xa, hr_qp->qpn, hr_qp, GFP_KERNEL)); + ret = xa_err(xa_store_irq(xa, hr_qp->qpn, hr_qp)); if (ret) dev_err(hr_dev->dev, "Failed to xa store for QPC\n"); else diff --git a/drivers/infiniband/hw/mlx5/srq_cmd.c b/drivers/infiniband/hw/mlx5/srq_cmd.c index db889ec3fd48..f277e264ceab 100644 --- a/drivers/infiniband/hw/mlx5/srq_cmd.c +++ b/drivers/infiniband/hw/mlx5/srq_cmd.c @@ -578,7 +578,7 @@ int mlx5_cmd_create_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq, refcount_set(&srq->common.refcount, 1); init_completion(&srq->common.free); - err = xa_err(xa_store_irq(&table->array, srq->srqn, srq, GFP_KERNEL)); + err = xa_err(xa_store_irq(&table->array, srq->srqn, srq)); if (err) goto err_destroy_srq_split; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed 2020-11-04 2:32 ` [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed xiaofeng.yan @ 2020-11-04 18:58 ` Jason Gunthorpe 2020-11-04 19:30 ` Matthew Wilcox 0 siblings, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2020-11-04 18:58 UTC (permalink / raw) To: xiaofeng.yan Cc: willy, linux-fsdevel, linux-kernel, dledford, oulijun, yanxiaofeng7 On Wed, Nov 04, 2020 at 10:32:13AM +0800, xiaofeng.yan wrote: > From: "xiaofeng.yan" <yanxiaofeng7@jd.com> > > function xa_store_irq() has three parameters because of removing > patameter "gfp_t gfp" > > Signed-off-by: xiaofeng.yan <yanxiaofeng7@jd.com> > drivers/infiniband/core/cm.c | 2 +- > drivers/infiniband/hw/hns/hns_roce_qp.c | 2 +- > drivers/infiniband/hw/mlx5/srq_cmd.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 5740d1ba3568..afcb5711270b 100644 > +++ b/drivers/infiniband/core/cm.c > @@ -879,7 +879,7 @@ 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); > + cm_id_priv); > } This one is almost a bug, the entry is preallocated with NULL though: ret = xa_alloc_cyclic_irq(&cm.local_id_table, &id, NULL, xa_limit_32b, &cm.local_id_next, GFP_KERNEL); so it should never allocate here: static int cm_req_handler(struct cm_work *work) { spin_lock_irq(&cm_id_priv->lock); cm_finalize_id(cm_id_priv); Still, woops. Matt, maybe a might_sleep is deserved in here someplace? @@ -1534,6 +1534,8 @@ void *__xa_store(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp) XA_STATE(xas, xa, index); void *curr; + might_sleep_if(gfpflags_allow_blocking(gfp)); + if (WARN_ON_ONCE(xa_is_advanced(entry))) return XA_ERROR(-EINVAL); if (xa_track_free(xa) && !entry) And similar in the other places that conditionally call __xas_nomem() ? I also still wish there was a proper 'xa store in already allocated but null' idiom - I remember you thought about using gfp flags == 0 at one point. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed 2020-11-04 18:58 ` Jason Gunthorpe @ 2020-11-04 19:30 ` Matthew Wilcox 2020-11-04 21:34 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Matthew Wilcox @ 2020-11-04 19:30 UTC (permalink / raw) To: Jason Gunthorpe Cc: xiaofeng.yan, linux-fsdevel, linux-kernel, dledford, oulijun, yanxiaofeng7 On Wed, Nov 04, 2020 at 02:58:43PM -0400, Jason Gunthorpe wrote: > > 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); > > + cm_id_priv); > > } > > This one is almost a bug, the entry is preallocated with NULL though: > > ret = xa_alloc_cyclic_irq(&cm.local_id_table, &id, NULL, xa_limit_32b, > &cm.local_id_next, GFP_KERNEL); > > so it should never allocate here: > > static int cm_req_handler(struct cm_work *work) > { > spin_lock_irq(&cm_id_priv->lock); > cm_finalize_id(cm_id_priv); Uhm. I think you want a different debugging check from this. The actual bug here is that you'll get back from calling cm_finalize_id() with interrupts enabled. Can you switch to xa_store(), or do we need an xa_store_irqsave()? > Still, woops. > > Matt, maybe a might_sleep is deserved in here someplace? > > @@ -1534,6 +1534,8 @@ void *__xa_store(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp) > XA_STATE(xas, xa, index); > void *curr; > > + might_sleep_if(gfpflags_allow_blocking(gfp)); > + > if (WARN_ON_ONCE(xa_is_advanced(entry))) > return XA_ERROR(-EINVAL); > if (xa_track_free(xa) && !entry) > > And similar in the other places that conditionally call __xas_nomem() > ? > > I also still wish there was a proper 'xa store in already allocated > but null' idiom - I remember you thought about using gfp flags == 0 at > one point. An xa_replace(), perhaps? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed 2020-11-04 19:30 ` Matthew Wilcox @ 2020-11-04 21:34 ` Jason Gunthorpe 0 siblings, 0 replies; 6+ messages in thread From: Jason Gunthorpe @ 2020-11-04 21:34 UTC (permalink / raw) To: Matthew Wilcox Cc: xiaofeng.yan, linux-fsdevel, linux-kernel, dledford, oulijun, yanxiaofeng7 On Wed, Nov 04, 2020 at 07:30:36PM +0000, Matthew Wilcox wrote: > On Wed, Nov 04, 2020 at 02:58:43PM -0400, Jason Gunthorpe wrote: > > > 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); > > > + cm_id_priv); > > > } > > > > This one is almost a bug, the entry is preallocated with NULL though: > > > > ret = xa_alloc_cyclic_irq(&cm.local_id_table, &id, NULL, xa_limit_32b, > > &cm.local_id_next, GFP_KERNEL); > > > > so it should never allocate here: > > > > static int cm_req_handler(struct cm_work *work) > > { > > spin_lock_irq(&cm_id_priv->lock); > > cm_finalize_id(cm_id_priv); > > Uhm. I think you want a different debugging check from this. The actual > bug here is that you'll get back from calling cm_finalize_id() with > interrupts enabled. Ooh, that is just no fun too :\ Again surprised some lockdep didn't catch wrongly nesting irq locks > Can you switch to xa_store(), or do we need an > xa_store_irqsave()? Yes, it looks like there is no reason for this, all users of the xarray are from sleeping contexts, so it shouldn't need the IRQ version.. I made a patch for this thanks The cm_id_priv->lock is probably also not needing to be irq either, but that is much harder to tell for sure > > Still, woops. > > > > Matt, maybe a might_sleep is deserved in here someplace? > > > > @@ -1534,6 +1534,8 @@ void *__xa_store(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp) > > XA_STATE(xas, xa, index); > > void *curr; > > > > + might_sleep_if(gfpflags_allow_blocking(gfp)); > > + > > if (WARN_ON_ONCE(xa_is_advanced(entry))) > > return XA_ERROR(-EINVAL); > > if (xa_track_free(xa) && !entry) > > > > And similar in the other places that conditionally call __xas_nomem() > > ? But this debugging would still catch the wrong nesting of a GFP_KERNEL inside a spinlock, you don't like it? > > I also still wish there was a proper 'xa store in already allocated > > but null' idiom - I remember you thought about using gfp flags == 0 at > > one point. > > An xa_replace(), perhaps? Make sense.. But I've also done this with cmpxchg. A magic GFP flag, as you tried to do with 0, is appealing in many ways Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks 2020-11-04 2:32 [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks xiaofeng.yan 2020-11-04 2:32 ` [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed xiaofeng.yan @ 2020-11-04 2:36 ` Matthew Wilcox 1 sibling, 0 replies; 6+ messages in thread From: Matthew Wilcox @ 2020-11-04 2:36 UTC (permalink / raw) To: xiaofeng.yan Cc: linux-fsdevel, linux-kernel, dledford, jgg, oulijun, yanxiaofeng7 On Wed, Nov 04, 2020 at 10:32:12AM +0800, xiaofeng.yan wrote: > xa_lock_irq(xa); > - curr = __xa_store(xa, index, entry, gfp); > + curr = __xa_store(xa, index, entry, GFP_ATOMIC); > xa_unlock_irq(xa); You haven't actually seen a bug, have you? You just read the code wrongly. void *__xa_store(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp) ... } while (__xas_nomem(&xas, gfp)); ... if (gfpflags_allow_blocking(gfp)) { xas_unlock_type(xas, lock_type); xas->xa_alloc = kmem_cache_alloc(radix_tree_node_cachep, gfp); xas_lock_type(xas, lock_type); ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-04 21:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-04 2:32 [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks xiaofeng.yan 2020-11-04 2:32 ` [PATCH 2/2] infiniband: Modify the reference to xa_store_irq() because the parameter of this function has changed xiaofeng.yan 2020-11-04 18:58 ` Jason Gunthorpe 2020-11-04 19:30 ` Matthew Wilcox 2020-11-04 21:34 ` Jason Gunthorpe 2020-11-04 2:36 ` [PATCH 1/2] [xarry]:Fixed an issue with memory allocated using the GFP_KERNEL flag in spinlocks Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).