* [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 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.