* [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 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).