[PATCHv6,1/4] RDMA/rxe: Fix dead lock caused by __rxe_add_to_pool interrupted by rxe_pool_get_index

Commit Message

Zhu Yanjun April 22, 2022, 7:44 p.m. UTC

From: Zhu Yanjun <[e-mail]>

This is a dead lock problem.
The ah_pool xa_lock first is acquired in this:

{SOFTIRQ-ON-W} state was registered at:

  lock_acquire+0x1d2/0x5a0
  _raw_spin_lock+0x33/0x80
  __rxe_add_to_pool+0x183/0x230 [rdma_rxe]

Then ah_pool xa_lock is acquired in this:

{IN-SOFTIRQ-W}:

Call Trace:
 <TASK>
  dump_stack_lvl+0x44/0x57
  mark_lock.part.52.cold.79+0x3c/0x46
  __lock_acquire+0x1565/0x34a0
  lock_acquire+0x1d2/0x5a0
  _raw_spin_lock_irqsave+0x42/0x90
  rxe_pool_get_index+0x72/0x1d0 [rdma_rxe]
  rxe_get_av+0x168/0x2a0 [rdma_rxe]
</TASK>

From the above, in the function __rxe_add_to_pool,
xa_lock is acquired. Then the function __rxe_add_to_pool
is interrupted by softirq. The function
rxe_pool_get_index will also acquire xa_lock.

Finally, the dead lock appears.

        CPU0
        ----
   lock(&xa->xa_lock#15);  <----- __rxe_add_to_pool
   <Interrupt>
     lock(&xa->xa_lock#15); <---- rxe_pool_get_index

                 *** DEADLOCK ***

Fixes: 3225717f6dfa ("RDMA/rxe: Replace red-black trees by carrays")
Reported-and-tested-by: Yi Zhang <[e-mail]>
Signed-off-by: Zhu Yanjun <[e-mail]>
---
V5->V6: One dead lock fix in one commit
V4->V5: Commit logs are changed.
V3->V4: xa_lock_irq locks are used.
V2->V3: __rxe_add_to_pool is between spin_lock and spin_unlock, so
        GFP_ATOMIC is used in __rxe_add_to_pool.
V1->V2: Replace GFP_KERNEL with GFP_ATOMIC
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Pearson, Robert B April 22, 2022, 3:57 p.m. UTC | #1

Use of rcu_read_lock solves this problem. Rcu_read_lock and spinlock on same data can
Co-exist at the same time. That is the whole point. All this is going away soon.

Bob

-----Original Message-----
From: [e-mail] <[e-mail]> 
Sent: Friday, April 22, 2022 2:44 PM
To: [e-mail]; [e-mail]; [e-mail]; [e-mail]
Cc: Yi Zhang <[e-mail]>
Subject: [PATCHv6 1/4] RDMA/rxe: Fix dead lock caused by __rxe_add_to_pool interrupted by rxe_pool_get_index

From: Zhu Yanjun <[e-mail]>

This is a dead lock problem.
The ah_pool xa_lock first is acquired in this:

{SOFTIRQ-ON-W} state was registered at:

  lock_acquire+0x1d2/0x5a0
  _raw_spin_lock+0x33/0x80
  __rxe_add_to_pool+0x183/0x230 [rdma_rxe]

Then ah_pool xa_lock is acquired in this:

{IN-SOFTIRQ-W}:

Call Trace:
 <TASK>
  dump_stack_lvl+0x44/0x57
  mark_lock.part.52.cold.79+0x3c/0x46
  __lock_acquire+0x1565/0x34a0
  lock_acquire+0x1d2/0x5a0
  _raw_spin_lock_irqsave+0x42/0x90
  rxe_pool_get_index+0x72/0x1d0 [rdma_rxe]
  rxe_get_av+0x168/0x2a0 [rdma_rxe]
</TASK>

From the above, in the function __rxe_add_to_pool, xa_lock is acquired. Then the function __rxe_add_to_pool is interrupted by softirq. The function rxe_pool_get_index will also acquire xa_lock.

Finally, the dead lock appears.

        CPU0
        ----
   lock(&xa->xa_lock#15);  <----- __rxe_add_to_pool
   <Interrupt>
     lock(&xa->xa_lock#15); <---- rxe_pool_get_index

                 *** DEADLOCK ***

Fixes: 3225717f6dfa ("RDMA/rxe: Replace red-black trees by carrays")
Reported-and-tested-by: Yi Zhang <[e-mail]>
Signed-off-by: Zhu Yanjun <[e-mail]>
---
V5->V6: One dead lock fix in one commit
V4->V5: Commit logs are changed.
V3->V4: xa_lock_irq locks are used.
V2->V3: __rxe_add_to_pool is between spin_lock and spin_unlock, so
        GFP_ATOMIC is used in __rxe_add_to_pool.
V1->V2: Replace GFP_KERNEL with GFP_ATOMIC
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 87066d04ed18..67f1d4733682 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -106,7 +106,7 @@ void rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
 
 	atomic_set(&pool->num_elem, 0);
 
-	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
+	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
 	pool->limit.min = info->min_index;
 	pool->limit.max = info->max_index;
 }
@@ -155,6 +155,7 @@ void *rxe_alloc(struct rxe_pool *pool)  int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)  {
 	int err;
+	unsigned long flags;
 
 	if (WARN_ON(pool->flags & RXE_POOL_ALLOC))
 		return -EINVAL;
@@ -166,8 +167,10 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
 	elem->obj = (u8 *)elem - pool->elem_offset;
 	kref_init(&elem->ref_cnt);
 
-	err = xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
-			      &pool->next, GFP_KERNEL);
+	xa_lock_irqsave(&pool->xa, flags);
+	err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
+				&pool->next, GFP_ATOMIC);
+	xa_unlock_irqrestore(&pool->xa, flags);
 	if (err)
 		goto err_cnt;
 
@@ -201,7 +204,7 @@ static void rxe_elem_release(struct kref *kref)
 	struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
 	struct rxe_pool *pool = elem->pool;
 
-	xa_erase(&pool->xa, elem->index);
+	xa_erase_irq(&pool->xa, elem->index);
 
 	if (pool->cleanup)
 		pool->cleanup(elem);
--
2.27.0

Zhu Yanjun April 24, 2022, 11:47 p.m. UTC | #2

在 2022/4/22 23:57, Pearson, Robert B 写道:
> Use of rcu_read_lock solves this problem. Rcu_read_lock and spinlock on same data can
> Co-exist at the same time. That is the whole point. All this is going away soon.

This is based on your unproved assumption.

Zhu Yanjun

> 
> Bob
> 
> -----Original Message-----
> From: [e-mail] <[e-mail]>
> Sent: Friday, April 22, 2022 2:44 PM
> To: [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Cc: Yi Zhang <[e-mail]>
> Subject: [PATCHv6 1/4] RDMA/rxe: Fix dead lock caused by __rxe_add_to_pool interrupted by rxe_pool_get_index
> 
> From: Zhu Yanjun <[e-mail]>
> 
> This is a dead lock problem.
> The ah_pool xa_lock first is acquired in this:
> 
> {SOFTIRQ-ON-W} state was registered at:
> 
>    lock_acquire+0x1d2/0x5a0
>    _raw_spin_lock+0x33/0x80
>    __rxe_add_to_pool+0x183/0x230 [rdma_rxe]
> 
> Then ah_pool xa_lock is acquired in this:
> 
> {IN-SOFTIRQ-W}:
> 
> Call Trace:
>   <TASK>
>    dump_stack_lvl+0x44/0x57
>    mark_lock.part.52.cold.79+0x3c/0x46
>    __lock_acquire+0x1565/0x34a0
>    lock_acquire+0x1d2/0x5a0
>    _raw_spin_lock_irqsave+0x42/0x90
>    rxe_pool_get_index+0x72/0x1d0 [rdma_rxe]
>    rxe_get_av+0x168/0x2a0 [rdma_rxe]
> </TASK>
> 
>  From the above, in the function __rxe_add_to_pool, xa_lock is acquired. Then the function __rxe_add_to_pool is interrupted by softirq. The function rxe_pool_get_index will also acquire xa_lock.
> 
> Finally, the dead lock appears.
> 
>          CPU0
>          ----
>     lock(&xa->xa_lock#15);  <----- __rxe_add_to_pool
>     <Interrupt>
>       lock(&xa->xa_lock#15); <---- rxe_pool_get_index
> 
>                   *** DEADLOCK ***
> 
> Fixes: 3225717f6dfa ("RDMA/rxe: Replace red-black trees by carrays")
> Reported-and-tested-by: Yi Zhang <[e-mail]>
> Signed-off-by: Zhu Yanjun <[e-mail]>
> ---
> V5->V6: One dead lock fix in one commit
> V4->V5: Commit logs are changed.
> V3->V4: xa_lock_irq locks are used.
> V2->V3: __rxe_add_to_pool is between spin_lock and spin_unlock, so
>          GFP_ATOMIC is used in __rxe_add_to_pool.
> V1->V2: Replace GFP_KERNEL with GFP_ATOMIC
> ---
>   drivers/infiniband/sw/rxe/rxe_pool.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> index 87066d04ed18..67f1d4733682 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> @@ -106,7 +106,7 @@ void rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
>   
>   	atomic_set(&pool->num_elem, 0);
>   
> -	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
> +	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
>   	pool->limit.min = info->min_index;
>   	pool->limit.max = info->max_index;
>   }
> @@ -155,6 +155,7 @@ void *rxe_alloc(struct rxe_pool *pool)  int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)  {
>   	int err;
> +	unsigned long flags;
>   
>   	if (WARN_ON(pool->flags & RXE_POOL_ALLOC))
>   		return -EINVAL;
> @@ -166,8 +167,10 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
>   	elem->obj = (u8 *)elem - pool->elem_offset;
>   	kref_init(&elem->ref_cnt);
>   
> -	err = xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
> -			      &pool->next, GFP_KERNEL);
> +	xa_lock_irqsave(&pool->xa, flags);
> +	err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
> +				&pool->next, GFP_ATOMIC);
> +	xa_unlock_irqrestore(&pool->xa, flags);
>   	if (err)
>   		goto err_cnt;
>   
> @@ -201,7 +204,7 @@ static void rxe_elem_release(struct kref *kref)
>   	struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
>   	struct rxe_pool *pool = elem->pool;
>   
> -	xa_erase(&pool->xa, elem->index);
> +	xa_erase_irq(&pool->xa, elem->index);
>   
>   	if (pool->cleanup)
>   		pool->cleanup(elem);
> --
> 2.27.0
>

Bob Pearson April 25, 2022, 5:32 p.m. UTC | #3

On 4/24/22 18:47, Yanjun Zhu wrote:
> 在 2022/4/22 23:57, Pearson, Robert B 写道:
>> Use of rcu_read_lock solves this problem. Rcu_read_lock and spinlock on same data can
>> Co-exist at the same time. That is the whole point. All this is going away soon.
> 
> This is based on your unproved assumption.
We are running the same tests (pyverbs, rping, blktests, perftest, etc.) with the same
configurations set (lockdep) and I am using rcu_read_lock for rxe_pool_get_index.
I do not see any deadlocks or lockdep warnings. The idea of RCU is to separate accesses
to a shared data structure into write operations and read operations. For write operations
a normal spinlock is used to allow only one writer at a time to access the data.
For RCU, unlike rwlocks, the reader is *not* locked at all and the writers are written so
that they can change the data structure underneath the readers and the reader will either
see the old data or the new data and not some combination of the two. This requires
some care but there is library support for this usually denoted by xxx_rcu(). In the case
of xarrays the whole library is RCU enabled so one can safely use xa_load() without
holding a spinlock. The rcu_read_lock() API marks the critical section so the writers can
make sure to wait long enough that there are no readers in the critical section before
freeing the data. The rcu_read_lock() is also recursive. It just expands the critical section.
In the case of rxe_pool.c the only rcu reader is rxe_pool_get_index() which looks up an
object from its index by calling xa_load(). This normally runs in a tasklet but sometimes
could run in process context. The only writers (as we have discussed) run in process context
in a create or destroy verbs call. The writers sometimes call while holding a _saveirq
spinlock from ib_create_ah() so lockdep issues warnings unless the spinlock in rxe_add_to_pool()
is also an _irq spinlock. However it does not issue this warning when RCU is used because
the reader *doesn't take a lock and therefore can't deadlock*. Thus the default xa_alloc_xxx()
which takes a spin_lock() it OK and the sequence xa_lock_irq(); __xa_alloc_xxx(); xa_unlock_irq()
is not needed.
> 
> Zhu Yanjun
> 
>>
>> Bob
>>
>> -----Original Message-----
>> From: [e-mail] <[e-mail]>
>> Sent: Friday, April 22, 2022 2:44 PM
>> To: [e-mail]; [e-mail]; [e-mail]; [e-mail]
>> Cc: Yi Zhang <[e-mail]>
>> Subject: [PATCHv6 1/4] RDMA/rxe: Fix dead lock caused by __rxe_add_to_pool interrupted by rxe_pool_get_index
>>
>> From: Zhu Yanjun <[e-mail]>
>>
>> This is a dead lock problem.
>> The ah_pool xa_lock first is acquired in this:
>>
>> {SOFTIRQ-ON-W} state was registered at:
>>
>>    lock_acquire+0x1d2/0x5a0
>>    _raw_spin_lock+0x33/0x80
>>    __rxe_add_to_pool+0x183/0x230 [rdma_rxe]
>>
>> Then ah_pool xa_lock is acquired in this:
>>
>> {IN-SOFTIRQ-W}:
>>
>> Call Trace:
>>   <TASK>
>>    dump_stack_lvl+0x44/0x57
>>    mark_lock.part.52.cold.79+0x3c/0x46
>>    __lock_acquire+0x1565/0x34a0
>>    lock_acquire+0x1d2/0x5a0
>>    _raw_spin_lock_irqsave+0x42/0x90
>>    rxe_pool_get_index+0x72/0x1d0 [rdma_rxe]
>>    rxe_get_av+0x168/0x2a0 [rdma_rxe]
>> </TASK>
>>
>>  From the above, in the function __rxe_add_to_pool, xa_lock is acquired. Then the function __rxe_add_to_pool is interrupted by softirq. The function rxe_pool_get_index will also acquire xa_lock.
>>
>> Finally, the dead lock appears.
>>
>>          CPU0
>>          ----
>>     lock(&xa->xa_lock#15);  <----- __rxe_add_to_pool
>>     <Interrupt>
>>       lock(&xa->xa_lock#15); <---- rxe_pool_get_index
>>
>>                   *** DEADLOCK ***
>>
>> Fixes: 3225717f6dfa ("RDMA/rxe: Replace red-black trees by carrays")
>> Reported-and-tested-by: Yi Zhang <[e-mail]>
>> Signed-off-by: Zhu Yanjun <[e-mail]>
>> ---
>> V5->V6: One dead lock fix in one commit
>> V4->V5: Commit logs are changed.
>> V3->V4: xa_lock_irq locks are used.
>> V2->V3: __rxe_add_to_pool is between spin_lock and spin_unlock, so
>>          GFP_ATOMIC is used in __rxe_add_to_pool.
>> V1->V2: Replace GFP_KERNEL with GFP_ATOMIC
>> ---
>>   drivers/infiniband/sw/rxe/rxe_pool.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
>> index 87066d04ed18..67f1d4733682 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
>> @@ -106,7 +106,7 @@ void rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
>>         atomic_set(&pool->num_elem, 0);
>>   -    xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
>> +    xa_init_flags(&pool->xa, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
>>       pool->limit.min = info->min_index;
>>       pool->limit.max = info->max_index;
>>   }
>> @@ -155,6 +155,7 @@ void *rxe_alloc(struct rxe_pool *pool)  int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)  {
>>       int err;
>> +    unsigned long flags;
>>         if (WARN_ON(pool->flags & RXE_POOL_ALLOC))
>>           return -EINVAL;
>> @@ -166,8 +167,10 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
>>       elem->obj = (u8 *)elem - pool->elem_offset;
>>       kref_init(&elem->ref_cnt);
>>   -    err = xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
>> -                  &pool->next, GFP_KERNEL);
>> +    xa_lock_irqsave(&pool->xa, flags);
>> +    err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
>> +                &pool->next, GFP_ATOMIC);
>> +    xa_unlock_irqrestore(&pool->xa, flags);
>>       if (err)
>>           goto err_cnt;
>>   @@ -201,7 +204,7 @@ static void rxe_elem_release(struct kref *kref)
>>       struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
>>       struct rxe_pool *pool = elem->pool;
>>   -    xa_erase(&pool->xa, elem->index);
>> +    xa_erase_irq(&pool->xa, elem->index);
>>         if (pool->cleanup)
>>           pool->cleanup(elem);
>> -- 
>> 2.27.0
>>
>

Jason Gunthorpe April 25, 2022, 7:02 p.m. UTC | #4

On Mon, Apr 25, 2022 at 07:47:23AM +0800, Yanjun Zhu wrote:
> 在 2022/4/22 23:57, Pearson, Robert B 写道:
> > Use of rcu_read_lock solves this problem. Rcu_read_lock and spinlock on same data can
> > Co-exist at the same time. That is the whole point. All this is going away soon.
> 
> This is based on your unproved assumption.

No, Bob is right, RCU avoids the need for a BH lock on this XA, the
only remaining issue is the AH creation atomic call path.

Jason

Zhu Yanjun April 25, 2022, 10:01 p.m. UTC | #5

在 2022/4/26 3:02, Jason Gunthorpe 写道:
> On Mon, Apr 25, 2022 at 07:47:23AM +0800, Yanjun Zhu wrote:
>> 在 2022/4/22 23:57, Pearson, Robert B 写道:
>>> Use of rcu_read_lock solves this problem. Rcu_read_lock and spinlock on same data can
>>> Co-exist at the same time. That is the whole point. All this is going away soon.
>> This is based on your unproved assumption.
> No, Bob is right, RCU avoids the need for a BH lock on this XA, the
> only remaining issue is the AH creation atomic call path.

If RCU is used, the similar issues like AH creation atomic call path 
will become more.

Zhu Yanjun

>
> Jason

Jason Gunthorpe April 25, 2022, 11:16 p.m. UTC | #6

On Tue, Apr 26, 2022 at 06:01:51AM +0800, Yanjun Zhu wrote:
> 
> 在 2022/4/26 3:02, Jason Gunthorpe 写道:
> > On Mon, Apr 25, 2022 at 07:47:23AM +0800, Yanjun Zhu wrote:
> > > 在 2022/4/22 23:57, Pearson, Robert B 写道:
> > > > Use of rcu_read_lock solves this problem. Rcu_read_lock and spinlock on same data can
> > > > Co-exist at the same time. That is the whole point. All this is going away soon.
> > > This is based on your unproved assumption.
> > No, Bob is right, RCU avoids the need for a BH lock on this XA, the
> > only remaining issue is the AH creation atomic call path.
> 
> If RCU is used, the similar issues like AH creation atomic call path will
> become more.

AH creation is unique, there will not be more cases like it.

Jason

Xiao Yang July 22, 2022, 6:51 a.m. UTC | #7

Hi Yanjun, Bob

Could you tell me if the dead lock issue has been fixed by the following 
issue:
[PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu

Best Regards,
Xiao Yang

On 2022/4/23 3:44, [e-mail] 写道:
> From: Zhu Yanjun <[e-mail]>
> 
> This is a dead lock problem.
> The ah_pool xa_lock first is acquired in this:
> 
> {SOFTIRQ-ON-W} state was registered at:
> 
>    lock_acquire+0x1d2/0x5a0
>    _raw_spin_lock+0x33/0x80
>    __rxe_add_to_pool+0x183/0x230 [rdma_rxe]
> 
> Then ah_pool xa_lock is acquired in this:
> 
> {IN-SOFTIRQ-W}:
> 
> Call Trace:
>   <TASK>
>    dump_stack_lvl+0x44/0x57
>    mark_lock.part.52.cold.79+0x3c/0x46
>    __lock_acquire+0x1565/0x34a0
>    lock_acquire+0x1d2/0x5a0
>    _raw_spin_lock_irqsave+0x42/0x90
>    rxe_pool_get_index+0x72/0x1d0 [rdma_rxe]
>    rxe_get_av+0x168/0x2a0 [rdma_rxe]
> </TASK>
> 
>  From the above, in the function __rxe_add_to_pool,
> xa_lock is acquired. Then the function __rxe_add_to_pool
> is interrupted by softirq. The function
> rxe_pool_get_index will also acquire xa_lock.
> 
> Finally, the dead lock appears.
> 
>          CPU0
>          ----
>     lock(&xa->xa_lock#15);  <----- __rxe_add_to_pool
>     <Interrupt>
>       lock(&xa->xa_lock#15); <---- rxe_pool_get_index
> 
>                   *** DEADLOCK ***
> 
> Fixes: 3225717f6dfa ("RDMA/rxe: Replace red-black trees by carrays")
> Reported-and-tested-by: Yi Zhang <[e-mail]>
> Signed-off-by: Zhu Yanjun <[e-mail]>
> ---
> V5->V6: One dead lock fix in one commit
> V4->V5: Commit logs are changed.
> V3->V4: xa_lock_irq locks are used.
> V2->V3: __rxe_add_to_pool is between spin_lock and spin_unlock, so
>          GFP_ATOMIC is used in __rxe_add_to_pool.
> V1->V2: Replace GFP_KERNEL with GFP_ATOMIC
> ---
>   drivers/infiniband/sw/rxe/rxe_pool.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
> index 87066d04ed18..67f1d4733682 100644
> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
> @@ -106,7 +106,7 @@ void rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
>   
>   	atomic_set(&pool->num_elem, 0);
>   
> -	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
> +	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
>   	pool->limit.min = info->min_index;
>   	pool->limit.max = info->max_index;
>   }
> @@ -155,6 +155,7 @@ void *rxe_alloc(struct rxe_pool *pool)
>   int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
>   {
>   	int err;
> +	unsigned long flags;
>   
>   	if (WARN_ON(pool->flags & RXE_POOL_ALLOC))
>   		return -EINVAL;
> @@ -166,8 +167,10 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
>   	elem->obj = (u8 *)elem - pool->elem_offset;
>   	kref_init(&elem->ref_cnt);
>   
> -	err = xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
> -			      &pool->next, GFP_KERNEL);
> +	xa_lock_irqsave(&pool->xa, flags);
> +	err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
> +				&pool->next, GFP_ATOMIC);
> +	xa_unlock_irqrestore(&pool->xa, flags);
>   	if (err)
>   		goto err_cnt;
>   
> @@ -201,7 +204,7 @@ static void rxe_elem_release(struct kref *kref)
>   	struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
>   	struct rxe_pool *pool = elem->pool;
>   
> -	xa_erase(&pool->xa, elem->index);
> +	xa_erase_irq(&pool->xa, elem->index);
>   
>   	if (pool->cleanup)
>   		pool->cleanup(elem);

Zhu Yanjun July 22, 2022, 1:43 p.m. UTC | #8

在 2022/7/22 14:51, [e-mail] 写道:
> Hi Yanjun, Bob
>
> Could you tell me if the dead lock issue has been fixed by the following
> issue:
> [PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu

Hi， Xiao

Normally I applied this "RDMA/rxe: Fix dead lock caused by 
__rxe_add_to_pool interrupted by rxe_pool_get_index" patch

series to fix this problem. And I am not sure if this problem is fixed 
by "[PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu".

Zhu Yanjun

>
> Best Regards,
> Xiao Yang
>
> On 2022/4/23 3:44, [e-mail] 写道:
>> From: Zhu Yanjun <[e-mail]>
>>
>> This is a dead lock problem.
>> The ah_pool xa_lock first is acquired in this:
>>
>> {SOFTIRQ-ON-W} state was registered at:
>>
>>     lock_acquire+0x1d2/0x5a0
>>     _raw_spin_lock+0x33/0x80
>>     __rxe_add_to_pool+0x183/0x230 [rdma_rxe]
>>
>> Then ah_pool xa_lock is acquired in this:
>>
>> {IN-SOFTIRQ-W}:
>>
>> Call Trace:
>>    <TASK>
>>     dump_stack_lvl+0x44/0x57
>>     mark_lock.part.52.cold.79+0x3c/0x46
>>     __lock_acquire+0x1565/0x34a0
>>     lock_acquire+0x1d2/0x5a0
>>     _raw_spin_lock_irqsave+0x42/0x90
>>     rxe_pool_get_index+0x72/0x1d0 [rdma_rxe]
>>     rxe_get_av+0x168/0x2a0 [rdma_rxe]
>> </TASK>
>>
>>   From the above, in the function __rxe_add_to_pool,
>> xa_lock is acquired. Then the function __rxe_add_to_pool
>> is interrupted by softirq. The function
>> rxe_pool_get_index will also acquire xa_lock.
>>
>> Finally, the dead lock appears.
>>
>>           CPU0
>>           ----
>>      lock(&xa->xa_lock#15);  <----- __rxe_add_to_pool
>>      <Interrupt>
>>        lock(&xa->xa_lock#15); <---- rxe_pool_get_index
>>
>>                    *** DEADLOCK ***
>>
>> Fixes: 3225717f6dfa ("RDMA/rxe: Replace red-black trees by carrays")
>> Reported-and-tested-by: Yi Zhang <[e-mail]>
>> Signed-off-by: Zhu Yanjun <[e-mail]>
>> ---
>> V5->V6: One dead lock fix in one commit
>> V4->V5: Commit logs are changed.
>> V3->V4: xa_lock_irq locks are used.
>> V2->V3: __rxe_add_to_pool is between spin_lock and spin_unlock, so
>>           GFP_ATOMIC is used in __rxe_add_to_pool.
>> V1->V2: Replace GFP_KERNEL with GFP_ATOMIC
>> ---
>>    drivers/infiniband/sw/rxe/rxe_pool.c | 11 +++++++----
>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
>> index 87066d04ed18..67f1d4733682 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_pool.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
>> @@ -106,7 +106,7 @@ void rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
>>    
>>    	atomic_set(&pool->num_elem, 0);
>>    
>> -	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
>> +	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
>>    	pool->limit.min = info->min_index;
>>    	pool->limit.max = info->max_index;
>>    }
>> @@ -155,6 +155,7 @@ void *rxe_alloc(struct rxe_pool *pool)
>>    int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
>>    {
>>    	int err;
>> +	unsigned long flags;
>>    
>>    	if (WARN_ON(pool->flags & RXE_POOL_ALLOC))
>>    		return -EINVAL;
>> @@ -166,8 +167,10 @@ int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
>>    	elem->obj = (u8 *)elem - pool->elem_offset;
>>    	kref_init(&elem->ref_cnt);
>>    
>> -	err = xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
>> -			      &pool->next, GFP_KERNEL);
>> +	xa_lock_irqsave(&pool->xa, flags);
>> +	err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
>> +				&pool->next, GFP_ATOMIC);
>> +	xa_unlock_irqrestore(&pool->xa, flags);
>>    	if (err)
>>    		goto err_cnt;
>>    
>> @@ -201,7 +204,7 @@ static void rxe_elem_release(struct kref *kref)
>>    	struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
>>    	struct rxe_pool *pool = elem->pool;
>>    
>> -	xa_erase(&pool->xa, elem->index);
>> +	xa_erase_irq(&pool->xa, elem->index);
>>    
>>    	if (pool->cleanup)
>>    		pool->cleanup(elem);

Xiao Yang July 22, 2022, 3:14 p.m. UTC | #9

On 2022/7/22 21:43, Yanjun Zhu wrote:
> Hi， Xiao
> 
> Normally I applied this "RDMA/rxe: Fix dead lock caused by 
> __rxe_add_to_pool interrupted by rxe_pool_get_index" patch
> 
> series to fix this problem. And I am not sure if this problem is fixed 
> by "[PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu".
> 
> Zhu Yanjun
Hi Yanjun,

I have confirmed that the problem has been fixed by:
[PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu

Best Regards,
Xiao Yang

Jason Gunthorpe July 22, 2022, 3:20 p.m. UTC | #10

On Fri, Jul 22, 2022 at 03:14:25PM +0000, [e-mail] wrote:
> On 2022/7/22 21:43, Yanjun Zhu wrote:
> > Hi， Xiao
> > 
> > Normally I applied this "RDMA/rxe: Fix dead lock caused by 
> > __rxe_add_to_pool interrupted by rxe_pool_get_index" patch
> > 
> > series to fix this problem. And I am not sure if this problem is fixed 
> > by "[PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu".
> > 
> > Zhu Yanjun
> Hi Yanjun,
> 
> I have confirmed that the problem has been fixed by:
> [PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu

Thanks!

Jason

Zhu Yanjun July 23, 2022, 12:35 a.m. UTC | #11

在 2022/7/22 23:14, [e-mail] 写道:
> On 2022/7/22 21:43, Yanjun Zhu wrote:
>> Hi， Xiao
>>
>> Normally I applied this "RDMA/rxe: Fix dead lock caused by
>> __rxe_add_to_pool interrupted by rxe_pool_get_index" patch
>>
>> series to fix this problem. And I am not sure if this problem is fixed
>> by "[PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu".
>>
>> Zhu Yanjun
> Hi Yanjun,
>
> I have confirmed that the problem has been fixed by:
> [PATCH v16 2/2] RDMA/rxe: Convert read side locking to rcu

Thanks.

Zhu Yanjun

>
> Best Regards,
> Xiao Yang

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 87066d04ed18..67f1d4733682 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ b/drivers/infiniband/sw/rxe/rxe_pool.c
@@ -106,7 +106,7 @@  void rxe_pool_init(struct rxe_dev *rxe, struct rxe_pool *pool,
 
 	atomic_set(&pool->num_elem, 0);
 
-	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
+	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
 	pool->limit.min = info->min_index;
 	pool->limit.max = info->max_index;
 }
@@ -155,6 +155,7 @@  void *rxe_alloc(struct rxe_pool *pool)
 int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
 {
 	int err;
+	unsigned long flags;
 
 	if (WARN_ON(pool->flags & RXE_POOL_ALLOC))
 		return -EINVAL;
@@ -166,8 +167,10 @@  int __rxe_add_to_pool(struct rxe_pool *pool, struct rxe_pool_elem *elem)
 	elem->obj = (u8 *)elem - pool->elem_offset;
 	kref_init(&elem->ref_cnt);
 
-	err = xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
-			      &pool->next, GFP_KERNEL);
+	xa_lock_irqsave(&pool->xa, flags);
+	err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
+				&pool->next, GFP_ATOMIC);
+	xa_unlock_irqrestore(&pool->xa, flags);
 	if (err)
 		goto err_cnt;
 
@@ -201,7 +204,7 @@  static void rxe_elem_release(struct kref *kref)
 	struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
 	struct rxe_pool *pool = elem->pool;
 
-	xa_erase(&pool->xa, elem->index);
+	xa_erase_irq(&pool->xa, elem->index);
 
 	if (pool->cleanup)
 		pool->cleanup(elem);