[PATCH for-next] RDMA/rxe: Convert spin_lock_irqsave to spin_lock

[PATCH for-next] RDMA/rxe: Convert spin_lock_irqsave to spin_lock

[Bob Pearson]

Currently the rxe driver uses a spin_lock_irqsave call to protect
the producer end of a completion queue from multiple QPs which may
be sharing the cq. Since all accesses to the cq are from tasklets
this is overkill and a simple spin_lock will be sufficient.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_cq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_cq.c b/drivers/infiniband/sw/rxe/rxe_cq.c
index 76534bc66cb6..408ffe2e6f14 100644
--- a/drivers/infiniband/sw/rxe/rxe_cq.c
+++ b/drivers/infiniband/sw/rxe/rxe_cq.c
@@ -104,13 +104,12 @@ int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited)
 	struct ib_event ev;
 	int full;
 	void *addr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&cq->cq_lock, flags);
+	spin_lock(&cq->cq_lock);
 
 	full = queue_full(cq->queue, QUEUE_TYPE_TO_CLIENT);
 	if (unlikely(full)) {
-		spin_unlock_irqrestore(&cq->cq_lock, flags);
+		spin_unlock(&cq->cq_lock);
 		if (cq->ibcq.event_handler) {
 			ev.device = cq->ibcq.device;
 			ev.element.cq = &cq->ibcq;
@@ -126,7 +125,7 @@ int rxe_cq_post(struct rxe_cq *cq, struct rxe_cqe *cqe, int solicited)
 
 	queue_advance_producer(cq->queue, QUEUE_TYPE_TO_CLIENT);
 
-	spin_unlock_irqrestore(&cq->cq_lock, flags);
+	spin_unlock(&cq->cq_lock);
 
 	if ((cq->notify == IB_CQ_NEXT_COMP) ||
 	    (cq->notify == IB_CQ_SOLICITED && solicited)) {

base-commit: cbdae01d8b517b81ed271981395fee8ebd08ba7d
-- 
2.34.1

Re: [PATCH for-next] RDMA/rxe: Convert spin_lock_irqsave to spin_lock

[Jason Gunthorpe]

On Thu, Oct 06, 2022 at 12:04:55PM -0500, Bob Pearson wrote:
> Currently the rxe driver uses a spin_lock_irqsave call to protect
> the producer end of a completion queue from multiple QPs which may
> be sharing the cq. Since all accesses to the cq are from tasklets
> this is overkill and a simple spin_lock will be sufficient.

A tasklet is a softirq, which means it needs to use the BH suffix for
all acquires that are in a process context if it the spinlock is
obtained from a tasklet

There is a nice table in Documentation/kernel-hacking/locking.rst that
explains this.

Jason

RE: [PATCH for-next] RDMA/rxe: Convert spin_lock_irqsave to spin_lock

[Pearson, Robert B]

Yes. Every time it turns out that you really need irqsave locks for rxe it is because some maniac is calling into the verbs API at interrupt level. I have run into this a couple of times.

Bob

-----Original Message-----
From: Jason Gunthorpe <jgg@nvidia.com> 
Sent: Friday, October 28, 2022 12:33 PM
To: Bob Pearson <rpearsonhpe@gmail.com>
Cc: zyjzyj2000@gmail.com; linux-rdma@vger.kernel.org
Subject: Re: [PATCH for-next] RDMA/rxe: Convert spin_lock_irqsave to spin_lock

On Thu, Oct 06, 2022 at 12:04:55PM -0500, Bob Pearson wrote:
> Currently the rxe driver uses a spin_lock_irqsave call to protect the 
> producer end of a completion queue from multiple QPs which may be 
> sharing the cq. Since all accesses to the cq are from tasklets this is 
> overkill and a simple spin_lock will be sufficient.

A tasklet is a softirq, which means it needs to use the BH suffix for all acquires that are in a process context if it the spinlock is obtained from a tasklet

There is a nice table in Documentation/kernel-hacking/locking.rst that explains this.

Jason

Re: [PATCH for-next] RDMA/rxe: Convert spin_lock_irqsave to spin_lock

[Jason Gunthorpe]

On Fri, Oct 28, 2022 at 05:41:29PM +0000, Pearson, Robert B wrote:

> Yes. Every time it turns out that you really need irqsave locks for
> rxe it is because some maniac is calling into the verbs API at
> interrupt level. I have run into this a couple of times.

I'm pretty sure post_send is not legal to call from an irq or
softirq..

Jason