[PATCH for-next v2 0/9] RDMA/rxe: Various fixes

[PATCH for-next v2 0/9] RDMA/rxe: Various fixes

[Bob Pearson]

This patch series is a collection of fixes to the rdma_rxe driver.
Some are new and some are replacements of eariler patches that are
rebased and improved.

These patches apply cleanly to current wip/jgg-for-next.

Bob Pearson (9):
  RDMA/rxe: Add rxe_is_fenced() subroutine
  RDMA/rxe: Convert pr_warn/err to pr_debug in pyverbs
  RDMA/rxe: Remove unnecessary include statement
  RDMA/rxe: Replace include statement
  RDMA/rxe: Fix rnr retry behavior
  RDMA/rxe: Fix deadlock in rxe_do_local_ops()
  RDMA/rxe: Make the tasklet exits the same
  RDMA/rxe: Limit the number of calls to each tasklet
  RDMA/rxe: Replace __rxe_do_task by rxe_run_task

 drivers/infiniband/sw/rxe/rxe_comp.c  |  43 ++++++----
 drivers/infiniband/sw/rxe/rxe_cq.c    |   8 +-
 drivers/infiniband/sw/rxe/rxe_param.h |   6 ++
 drivers/infiniband/sw/rxe/rxe_qp.c    |   1 +
 drivers/infiniband/sw/rxe/rxe_queue.h |   5 +-
 drivers/infiniband/sw/rxe/rxe_req.c   | 116 ++++++++++++++++++--------
 drivers/infiniband/sw/rxe/rxe_resp.c  |  25 ++++--
 drivers/infiniband/sw/rxe/rxe_task.c  |  16 +++-
 drivers/infiniband/sw/rxe/rxe_verbs.h |   2 +-
 9 files changed, 148 insertions(+), 74 deletions(-)


base-commit: e5d12406b94a4d0bb63cb4d78e568c6d9fadacfd
-- 
2.34.1

[PATCH for-next v2 1/9] RDMA/rxe: Add rxe_is_fenced() subroutine

[Bob Pearson]

The code thc that decides whether to defer execution of a wqe in
rxe_requester.c is isolated into a subroutine rxe_is_fenced()
and removed from the call to req_next_wqe(). The condition whether
a wqe should be fenced is changed to comply with the IBA. Currently
an operation is fenced if the fence bit is set in the wqe flags and
the last wqe has not completed. For normal operations the IBA
actually only requires that the last read or atomic operation is
complete. 

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
v2 replaces "RDMA/rxe: Fix incorrect fencing"

 drivers/infiniband/sw/rxe/rxe_req.c | 37 ++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 9d98237389cf..e8a1664a40eb 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -161,16 +161,36 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
 		     (wqe->state != wqe_state_processing)))
 		return NULL;
 
-	if (unlikely((wqe->wr.send_flags & IB_SEND_FENCE) &&
-						     (index != cons))) {
-		qp->req.wait_fence = 1;
-		return NULL;
-	}
-
 	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
 	return wqe;
 }
 
+/**
+ * rxe_wqe_is_fenced - check if next wqe is fenced
+ * @qp: the queue pair
+ * @wqe: the next wqe
+ *
+ * Returns: 1 if wqe needs to wait
+ *	    0 if wqe is ready to go
+ */
+static int rxe_wqe_is_fenced(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
+{
+	/* Local invalidate fence (LIF) see IBA 10.6.5.1
+	 * Requires ALL previous operations on the send queue
+	 * are complete. Make mandatory for the rxe driver.
+	 */
+	if (wqe->wr.opcode == IB_WR_LOCAL_INV)
+		return qp->req.wqe_index != queue_get_consumer(qp->sq.queue,
+						QUEUE_TYPE_FROM_CLIENT);
+
+	/* Fence see IBA 10.8.3.3
+	 * Requires that all previous read and atomic operations
+	 * are complete.
+	 */
+	return (wqe->wr.send_flags & IB_SEND_FENCE) &&
+		atomic_read(&qp->req.rd_atomic) != qp->attr.max_rd_atomic;
+}
+
 static int next_opcode_rc(struct rxe_qp *qp, u32 opcode, int fits)
 {
 	switch (opcode) {
@@ -632,6 +652,11 @@ int rxe_requester(void *arg)
 	if (unlikely(!wqe))
 		goto exit;
 
+	if (rxe_wqe_is_fenced(qp, wqe)) {
+		qp->req.wait_fence = 1;
+		goto exit;
+	}
+
 	if (wqe->mask & WR_LOCAL_OP_MASK) {
 		ret = rxe_do_local_ops(qp, wqe);
 		if (unlikely(ret))
-- 
2.34.1

Re: [PATCH for-next v2 1/9] RDMA/rxe: Add rxe_is_fenced() subroutine

[lizhijian]

On 01/07/2022 03:04, Bob Pearson wrote:
> The code thc that decides whether to defer execution of a wqe in
> rxe_requester.c is isolated into a subroutine rxe_is_fenced()
> and removed from the call to req_next_wqe(). The condition whether
> a wqe should be fenced is changed to comply with the IBA. Currently
> an operation is fenced if the fence bit is set in the wqe flags and
> the last wqe has not completed. For normal operations the IBA
> actually only requires that the last read or atomic operation is
> complete.
>
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
Looks good to me.

Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>


> ---
> v2 replaces "RDMA/rxe: Fix incorrect fencing"
>
>   drivers/infiniband/sw/rxe/rxe_req.c | 37 ++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 9d98237389cf..e8a1664a40eb 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -161,16 +161,36 @@ static struct rxe_send_wqe *req_next_wqe(struct rxe_qp *qp)
>   		     (wqe->state != wqe_state_processing)))
>   		return NULL;
>   
> -	if (unlikely((wqe->wr.send_flags & IB_SEND_FENCE) &&
> -						     (index != cons))) {
> -		qp->req.wait_fence = 1;
> -		return NULL;
> -	}
> -
>   	wqe->mask = wr_opcode_mask(wqe->wr.opcode, qp);
>   	return wqe;
>   }
>   
> +/**
> + * rxe_wqe_is_fenced - check if next wqe is fenced
> + * @qp: the queue pair
> + * @wqe: the next wqe
> + *
> + * Returns: 1 if wqe needs to wait
> + *	    0 if wqe is ready to go
> + */
> +static int rxe_wqe_is_fenced(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
> +{
> +	/* Local invalidate fence (LIF) see IBA 10.6.5.1
> +	 * Requires ALL previous operations on the send queue
> +	 * are complete. Make mandatory for the rxe driver.
> +	 */
> +	if (wqe->wr.opcode == IB_WR_LOCAL_INV)
> +		return qp->req.wqe_index != queue_get_consumer(qp->sq.queue,
> +						QUEUE_TYPE_FROM_CLIENT);
> +
> +	/* Fence see IBA 10.8.3.3
> +	 * Requires that all previous read and atomic operations
> +	 * are complete.
> +	 */
> +	return (wqe->wr.send_flags & IB_SEND_FENCE) &&
> +		atomic_read(&qp->req.rd_atomic) != qp->attr.max_rd_atomic;
> +}
> +
>   static int next_opcode_rc(struct rxe_qp *qp, u32 opcode, int fits)
>   {
>   	switch (opcode) {
> @@ -632,6 +652,11 @@ int rxe_requester(void *arg)
>   	if (unlikely(!wqe))
>   		goto exit;
>   
> +	if (rxe_wqe_is_fenced(qp, wqe)) {
> +		qp->req.wait_fence = 1;
> +		goto exit;
> +	}
> +
>   	if (wqe->mask & WR_LOCAL_OP_MASK) {
>   		ret = rxe_do_local_ops(qp, wqe);
>   		if (unlikely(ret))

[PATCH for-next v2 2/9] RDMA/rxe: Convert pr_warn/err to pr_debug in pyverbs

[Bob Pearson]

The pyverbs test suite generates a few dmesg traces from intentional
error tests. This patch replaces those messages with pr_debug()
calls which improves the usefullness of the tests.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_cq.c   | 8 ++++----
 drivers/infiniband/sw/rxe/rxe_resp.c | 8 +++++---
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_cq.c b/drivers/infiniband/sw/rxe/rxe_cq.c
index 642b52539ac3..b1a0ab3cd4bd 100644
--- a/drivers/infiniband/sw/rxe/rxe_cq.c
+++ b/drivers/infiniband/sw/rxe/rxe_cq.c
@@ -19,16 +19,16 @@ int rxe_cq_chk_attr(struct rxe_dev *rxe, struct rxe_cq *cq,
 	}
 
 	if (cqe > rxe->attr.max_cqe) {
-		pr_warn("cqe(%d) > max_cqe(%d)\n",
-			cqe, rxe->attr.max_cqe);
+		pr_debug("cqe(%d) > max_cqe(%d)\n",
+				cqe, rxe->attr.max_cqe);
 		goto err1;
 	}
 
 	if (cq) {
 		count = queue_count(cq->queue, QUEUE_TYPE_TO_CLIENT);
 		if (cqe < count) {
-			pr_warn("cqe(%d) < current # elements in queue (%d)",
-				cqe, count);
+			pr_debug("cqe(%d) < current # elements in queue (%d)",
+					cqe, count);
 			goto err1;
 		}
 	}
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index c45c9d954931..7aea5f06d061 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -448,7 +448,8 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
 	if (rkey_is_mw(rkey)) {
 		mw = rxe_lookup_mw(qp, access, rkey);
 		if (!mw) {
-			pr_err("%s: no MW matches rkey %#x\n", __func__, rkey);
+			pr_debug("%s: no MW matches rkey %#x\n",
+					__func__, rkey);
 			state = RESPST_ERR_RKEY_VIOLATION;
 			goto err;
 		}
@@ -468,7 +469,8 @@ static enum resp_states check_rkey(struct rxe_qp *qp,
 	} else {
 		mr = lookup_mr(qp->pd, access, rkey, RXE_LOOKUP_REMOTE);
 		if (!mr) {
-			pr_err("%s: no MR matches rkey %#x\n", __func__, rkey);
+			pr_debug("%s: no MR matches rkey %#x\n",
+					__func__, rkey);
 			state = RESPST_ERR_RKEY_VIOLATION;
 			goto err;
 		}
@@ -1434,7 +1436,7 @@ int rxe_responder(void *arg)
 
 		case RESPST_ERROR:
 			qp->resp.goto_error = 0;
-			pr_warn("qp#%d moved to error state\n", qp_num(qp));
+			pr_debug("qp#%d moved to error state\n", qp_num(qp));
 			rxe_qp_error(qp);
 			goto exit;
 
-- 
2.34.1

[PATCH for-next v2 3/9] RDMA/rxe: Remove unnecessary include statement

[Bob Pearson]

rxe_verbs.h includes the file <rdma/rdma_user_rxe.h>.
It should have been <uapi/rdma/rdma_user_rxe.h>,
however, it is not used and not required in this file.

This patch removes the include statement.

Reported-by: Frank Zago <frank.zago@hpe.com>
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_verbs.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index ac464e68c923..ec9a70aecc1e 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -9,7 +9,6 @@
 
 #include <linux/interrupt.h>
 #include <linux/workqueue.h>
-#include <rdma/rdma_user_rxe.h>
 #include "rxe_pool.h"
 #include "rxe_task.h"
 #include "rxe_hw_counters.h"
-- 
2.34.1

[PATCH for-next v2 4/9] RDMA/rxe: Replace include statement

[Bob Pearson]

rxe_queue.h currently includes <uapi/rdma/rdma_user_rxe.h> for
a definition of struct rxe_queue_buf. But it is only used as
a pointer so the definition is not needed.

This patch replaces the include statement with the declaration

struct rxe_queue_buf;

Reported-by: Frank Zago <frank.zago@hpe.com>
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_queue.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_queue.h b/drivers/infiniband/sw/rxe/rxe_queue.h
index 6227112ef7a2..ed44042782fa 100644
--- a/drivers/infiniband/sw/rxe/rxe_queue.h
+++ b/drivers/infiniband/sw/rxe/rxe_queue.h
@@ -7,9 +7,6 @@
 #ifndef RXE_QUEUE_H
 #define RXE_QUEUE_H
 
-/* for definition of shared struct rxe_queue_buf */
-#include <uapi/rdma/rdma_user_rxe.h>
-
 /* Implements a simple circular buffer that is shared between user
  * and the driver and can be resized. The requested element size is
  * rounded up to a power of 2 and the number of elements in the buffer
@@ -53,6 +50,8 @@ enum queue_type {
 	QUEUE_TYPE_FROM_DRIVER,
 };
 
+struct rxe_queue_buf;
+
 struct rxe_queue {
 	struct rxe_dev		*rxe;
 	struct rxe_queue_buf	*buf;
-- 
2.34.1

[PATCH for-next v2 5/9] RDMA/rxe: Fix rnr retry behavior

[Bob Pearson]

Currently the completer tasklet when retransmit timer or the rnr timer
fires the same flag (qp->req.need_retry) is set so that if either
timer fires it will attempt to perform a retry flow on the send queue.
This has the effect of responding to an RNR NAK at the first retransmit
timer event which might not allow the requested rnr timeout.

This patch adds a new flag (qp->req.wait_for_rnr_timer) which, if set,
prevents a retry flow until the rnr nak timer fires.

This patch fixes rnr retry errors which can be observed by running the
pyverbs test_rdmacm_async_traffic_external_qp multiple times. With this
patch applied they do not occur.

Link: https://lore.kernel.org/linux-rdma/a8287823-1408-4273-bc22-99a0678db640@gmail.com/
Link: https://lore.kernel.org/linux-rdma/2bafda9e-2bb6-186d-12a1-179e8f6a2678@talpey.com/
Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_comp.c  |  8 +++++++-
 drivers/infiniband/sw/rxe/rxe_qp.c    |  1 +
 drivers/infiniband/sw/rxe/rxe_req.c   | 15 +++++++++++++--
 drivers/infiniband/sw/rxe/rxe_verbs.h |  1 +
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
index da3a398053b8..4fc31bb7eee6 100644
--- a/drivers/infiniband/sw/rxe/rxe_comp.c
+++ b/drivers/infiniband/sw/rxe/rxe_comp.c
@@ -114,6 +114,8 @@ void retransmit_timer(struct timer_list *t)
 {
 	struct rxe_qp *qp = from_timer(qp, t, retrans_timer);
 
+	pr_debug("%s: fired for qp#%d\n", __func__, qp->elem.index);
+
 	if (qp->valid) {
 		qp->comp.timeout = 1;
 		rxe_run_task(&qp->comp.task, 1);
@@ -730,11 +732,15 @@ int rxe_completer(void *arg)
 			break;
 
 		case COMPST_RNR_RETRY:
+			/* we come here if we received an RNR NAK */
 			if (qp->comp.rnr_retry > 0) {
 				if (qp->comp.rnr_retry != 7)
 					qp->comp.rnr_retry--;
 
-				qp->req.need_retry = 1;
+				/* don't start a retry flow until the
+				 * rnr timer has fired
+				 */
+				qp->req.wait_for_rnr_timer = 1;
 				pr_debug("qp#%d set rnr nak timer\n",
 					 qp_num(qp));
 				mod_timer(&qp->rnr_nak_timer,
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 22e9b85344c3..edc0c34feb05 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -507,6 +507,7 @@ static void rxe_qp_reset(struct rxe_qp *qp)
 	atomic_set(&qp->ssn, 0);
 	qp->req.opcode = -1;
 	qp->req.need_retry = 0;
+	qp->req.wait_for_rnr_timer = 0;
 	qp->req.noack_pkts = 0;
 	qp->resp.msn = 0;
 	qp->resp.opcode = -1;
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index e8a1664a40eb..4d92f929d269 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -101,7 +101,11 @@ void rnr_nak_timer(struct timer_list *t)
 {
 	struct rxe_qp *qp = from_timer(qp, t, rnr_nak_timer);
 
-	pr_debug("qp#%d rnr nak timer fired\n", qp_num(qp));
+	pr_debug("%s: fired for qp#%d\n", __func__, qp_num(qp));
+
+	/* request a send queue retry */
+	qp->req.need_retry = 1;
+	qp->req.wait_for_rnr_timer = 0;
 	rxe_run_task(&qp->req.task, 1);
 }
 
@@ -640,10 +644,17 @@ int rxe_requester(void *arg)
 		qp->req.need_rd_atomic = 0;
 		qp->req.wait_psn = 0;
 		qp->req.need_retry = 0;
+		qp->req.wait_for_rnr_timer = 0;
 		goto exit;
 	}
 
-	if (unlikely(qp->req.need_retry)) {
+	/* we come here if the retransmot timer has fired
+	 * or if the rnr timer has fired. If the retransmit
+	 * timer fires while we are processing an RNR NAK wait
+	 * until the rnr timer has fired before starting the
+	 * retry flow
+	 */
+	if (unlikely(qp->req.need_retry && !qp->req.wait_for_rnr_timer)) {
 		req_retry(qp);
 		qp->req.need_retry = 0;
 	}
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h
index ec9a70aecc1e..d9f01af4050d 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.h
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.h
@@ -123,6 +123,7 @@ struct rxe_req_info {
 	int			need_rd_atomic;
 	int			wait_psn;
 	int			need_retry;
+	int			wait_for_rnr_timer;
 	int			noack_pkts;
 	struct rxe_task		task;
 };
-- 
2.34.1

[PATCH for-next v2 6/9] RDMA/rxe: Fix deadlock in rxe_do_local_ops()

[Bob Pearson]

When a local operation (invalidate mr, reg mr, bind mw) is finished
there will be no ack packet coming from a responder to cause the
wqe to be completed. This may happen anyway if a subsequent wqe
performs IO. Currently if the wqe is signalled the completer
tasklet is scheduled immediately but not otherwise.

This leads to a deadlock if the next wqe has the fence bit set in
send flags and the operation is not signalled. This patch removes
the condition that the wqe must be signalled in order to schedule
the completer tasklet which is the simplest fix for this deadlock
and is fairly low cost. This is the analog for local operations of
always setting the ackreq bit in all last or only request packets
even if the operation is not signalled.

Reported-by: Jenny Hack <jhack@hpe.com>
Fixes: c1a411268a4b1 ("RDMA/rxe: Move local ops to subroutine")
Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_req.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 4d92f929d269..81eca57b04b8 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -605,9 +605,11 @@ static int rxe_do_local_ops(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
 	wqe->status = IB_WC_SUCCESS;
 	qp->req.wqe_index = queue_next_index(qp->sq.queue, qp->req.wqe_index);
 
-	if ((wqe->wr.send_flags & IB_SEND_SIGNALED) ||
-	    qp->sq_sig_type == IB_SIGNAL_ALL_WR)
-		rxe_run_task(&qp->comp.task, 1);
+	/* There is no ack coming for local work requests
+	 * which can lead to a deadlock. So go ahead and complete
+	 * it now.
+	 */
+	rxe_run_task(&qp->comp.task, 1);
 
 	return 0;
 }
-- 
2.34.1

[PATCH for-next v2 7/9] RDMA/rxe: Make the tasklet exits the same

[Bob Pearson]

Make changes to the three tasklets so that the exit logic from
each is the same. This makes the code easier to understand.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_comp.c | 35 +++++++++---------
 drivers/infiniband/sw/rxe/rxe_req.c  | 54 ++++++++++++++++------------
 drivers/infiniband/sw/rxe/rxe_resp.c | 17 +++++----
 3 files changed, 60 insertions(+), 46 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
index 4fc31bb7eee6..bc53cad077aa 100644
--- a/drivers/infiniband/sw/rxe/rxe_comp.c
+++ b/drivers/infiniband/sw/rxe/rxe_comp.c
@@ -562,7 +562,7 @@ int rxe_completer(void *arg)
 	struct sk_buff *skb = NULL;
 	struct rxe_pkt_info *pkt = NULL;
 	enum comp_state state;
-	int ret = 0;
+	int ret;
 
 	if (!rxe_get(qp))
 		return -EAGAIN;
@@ -571,8 +571,7 @@ int rxe_completer(void *arg)
 	    qp->req.state == QP_STATE_RESET) {
 		rxe_drain_resp_pkts(qp, qp->valid &&
 				    qp->req.state == QP_STATE_ERROR);
-		ret = -EAGAIN;
-		goto done;
+		goto exit;
 	}
 
 	if (qp->comp.timeout) {
@@ -582,10 +581,8 @@ int rxe_completer(void *arg)
 		qp->comp.timeout_retry = 0;
 	}
 
-	if (qp->req.need_retry) {
-		ret = -EAGAIN;
-		goto done;
-	}
+	if (qp->req.need_retry)
+		goto exit;
 
 	state = COMPST_GET_ACK;
 
@@ -678,8 +675,7 @@ int rxe_completer(void *arg)
 			    qp->qp_timeout_jiffies)
 				mod_timer(&qp->retrans_timer,
 					  jiffies + qp->qp_timeout_jiffies);
-			ret = -EAGAIN;
-			goto done;
+			goto exit;
 
 		case COMPST_ERROR_RETRY:
 			/* we come here if the retry timer fired and we did
@@ -691,10 +687,8 @@ int rxe_completer(void *arg)
 			 */
 
 			/* there is nothing to retry in this case */
-			if (!wqe || (wqe->state == wqe_state_posted)) {
-				ret = -EAGAIN;
-				goto done;
-			}
+			if (!wqe || (wqe->state == wqe_state_posted))
+				goto exit;
 
 			/* if we've started a retry, don't start another
 			 * retry sequence, unless this is a timeout.
@@ -746,8 +740,7 @@ int rxe_completer(void *arg)
 				mod_timer(&qp->rnr_nak_timer,
 					  jiffies + rnrnak_jiffies(aeth_syn(pkt)
 						& ~AETH_TYPE_MASK));
-				ret = -EAGAIN;
-				goto done;
+				goto exit;
 			} else {
 				rxe_counter_inc(rxe,
 						RXE_CNT_RNR_RETRY_EXCEEDED);
@@ -760,12 +753,20 @@ int rxe_completer(void *arg)
 			WARN_ON_ONCE(wqe->status == IB_WC_SUCCESS);
 			do_complete(qp, wqe);
 			rxe_qp_error(qp);
-			ret = -EAGAIN;
-			goto done;
+			goto exit;
 		}
 	}
 
+	/* A non-zero return value will cause rxe_do_task to
+	 * exit its loop and end the tasklet. A zero return
+	 * will continue looping and return to rxe_completer
+	 */
 done:
+	ret = 0;
+	goto out;
+exit:
+	ret = -EAGAIN;
+out:
 	if (pkt)
 		free_pkt(pkt);
 	rxe_put(qp);
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 81eca57b04b8..008ae51c7560 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -625,6 +625,7 @@ int rxe_requester(void *arg)
 	u32 payload;
 	int mtu;
 	int opcode;
+	int err;
 	int ret;
 	struct rxe_send_wqe rollback_wqe;
 	u32 rollback_psn;
@@ -635,7 +636,6 @@ int rxe_requester(void *arg)
 	if (!rxe_get(qp))
 		return -EAGAIN;
 
-next_wqe:
 	if (unlikely(!qp->valid || qp->req.state == QP_STATE_ERROR))
 		goto exit;
 
@@ -650,7 +650,7 @@ int rxe_requester(void *arg)
 		goto exit;
 	}
 
-	/* we come here if the retransmot timer has fired
+	/* we come here if the retransmit timer has fired
 	 * or if the rnr timer has fired. If the retransmit
 	 * timer fires while we are processing an RNR NAK wait
 	 * until the rnr timer has fired before starting the
@@ -671,11 +671,11 @@ int rxe_requester(void *arg)
 	}
 
 	if (wqe->mask & WR_LOCAL_OP_MASK) {
-		ret = rxe_do_local_ops(qp, wqe);
-		if (unlikely(ret))
+		err = rxe_do_local_ops(qp, wqe);
+		if (unlikely(err))
 			goto err;
 		else
-			goto next_wqe;
+			goto done;
 	}
 
 	if (unlikely(qp_type(qp) == IB_QPT_RC &&
@@ -724,8 +724,7 @@ int rxe_requester(void *arg)
 			wqe->state = wqe_state_done;
 			wqe->status = IB_WC_SUCCESS;
 			__rxe_do_task(&qp->comp.task);
-			rxe_put(qp);
-			return 0;
+			goto done;
 		}
 		payload = mtu;
 	}
@@ -741,25 +740,29 @@ int rxe_requester(void *arg)
 	if (unlikely(!av)) {
 		pr_err("qp#%d Failed no address vector\n", qp_num(qp));
 		wqe->status = IB_WC_LOC_QP_OP_ERR;
-		goto err_drop_ah;
+		goto err;
 	}
 
 	skb = init_req_packet(qp, av, wqe, opcode, payload, &pkt);
 	if (unlikely(!skb)) {
 		pr_err("qp#%d Failed allocating skb\n", qp_num(qp));
 		wqe->status = IB_WC_LOC_QP_OP_ERR;
-		goto err_drop_ah;
+		if (ah)
+			rxe_put(ah);
+		goto err;
 	}
 
-	ret = finish_packet(qp, av, wqe, &pkt, skb, payload);
-	if (unlikely(ret)) {
+	err = finish_packet(qp, av, wqe, &pkt, skb, payload);
+	if (unlikely(err)) {
 		pr_debug("qp#%d Error during finish packet\n", qp_num(qp));
-		if (ret == -EFAULT)
+		if (err == -EFAULT)
 			wqe->status = IB_WC_LOC_PROT_ERR;
 		else
 			wqe->status = IB_WC_LOC_QP_OP_ERR;
 		kfree_skb(skb);
-		goto err_drop_ah;
+		if (ah)
+			rxe_put(ah);
+		goto err;
 	}
 
 	if (ah)
@@ -774,13 +777,14 @@ int rxe_requester(void *arg)
 	save_state(wqe, qp, &rollback_wqe, &rollback_psn);
 	update_wqe_state(qp, wqe, &pkt);
 	update_wqe_psn(qp, wqe, &pkt, payload);
-	ret = rxe_xmit_packet(qp, &pkt, skb);
-	if (ret) {
+
+	err = rxe_xmit_packet(qp, &pkt, skb);
+	if (err) {
 		qp->need_req_skb = 1;
 
 		rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
 
-		if (ret == -EAGAIN) {
+		if (err == -EAGAIN) {
 			rxe_run_task(&qp->req.task, 1);
 			goto exit;
 		}
@@ -791,16 +795,20 @@ int rxe_requester(void *arg)
 
 	update_state(qp, &pkt);
 
-	goto next_wqe;
-
-err_drop_ah:
-	if (ah)
-		rxe_put(ah);
+	/* A non-zero return value will cause rxe_do_task to
+	 * exit its loop and end the tasklet. A zero return
+	 * will continue looping and return to rxe_requester
+	 */
+done:
+	ret = 0;
+	goto out;
 err:
 	wqe->state = wqe_state_error;
 	__rxe_do_task(&qp->comp.task);
-
 exit:
+	ret = -EAGAIN;
+out:
 	rxe_put(qp);
-	return -EAGAIN;
+
+	return ret;
 }
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 7aea5f06d061..a45b1d22bc77 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -1258,17 +1258,15 @@ int rxe_responder(void *arg)
 	struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
 	enum resp_states state;
 	struct rxe_pkt_info *pkt = NULL;
-	int ret = 0;
+	int ret;
 
 	if (!rxe_get(qp))
 		return -EAGAIN;
 
 	qp->resp.aeth_syndrome = AETH_ACK_UNLIMITED;
 
-	if (!qp->valid) {
-		ret = -EINVAL;
-		goto done;
-	}
+	if (!qp->valid)
+		goto exit;
 
 	switch (qp->resp.state) {
 	case QP_STATE_RESET:
@@ -1445,9 +1443,16 @@ int rxe_responder(void *arg)
 		}
 	}
 
+	/* A non-zero return value will cause rxe_do_task to
+	 * exit its loop and end the tasklet. A zero return
+	 * will continue looping and return to rxe_responder
+	 */
+done:
+	ret = 0;
+	goto out;
 exit:
 	ret = -EAGAIN;
-done:
+out:
 	rxe_put(qp);
 	return ret;
 }
-- 
2.34.1

[PATCH for-next v2 8/9] RDMA/rxe: Limit the number of calls to each tasklet

[Bob Pearson]

Limit the maximum number of calls to each tasklet from rxe_do_task()
before yielding the cpu. When the limit is reached reschedule the
tasklet and exit the calling loop. This patch prevents one tasklet
from consuming 100% of a cpu core and causing a deadlock or soft lockup.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_param.h |  6 ++++++
 drivers/infiniband/sw/rxe/rxe_task.c  | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
index 568a7cbd13d4..9f3b036e6bde 100644
--- a/drivers/infiniband/sw/rxe/rxe_param.h
+++ b/drivers/infiniband/sw/rxe/rxe_param.h
@@ -105,6 +105,12 @@ enum rxe_device_param {
 	RXE_INFLIGHT_SKBS_PER_QP_HIGH	= 64,
 	RXE_INFLIGHT_SKBS_PER_QP_LOW	= 16,
 
+	/* Max number of interations of each tasklet
+	 * before yielding the cpu to let other
+	 * work make progress
+	 */
+	RXE_MAX_ITERATIONS		= 1024,
+
 	/* Delay before calling arbiter timer */
 	RXE_NSEC_ARB_TIMER_DELAY	= 200,
 
diff --git a/drivers/infiniband/sw/rxe/rxe_task.c b/drivers/infiniband/sw/rxe/rxe_task.c
index 0c4db5bb17d7..2248cf33d776 100644
--- a/drivers/infiniband/sw/rxe/rxe_task.c
+++ b/drivers/infiniband/sw/rxe/rxe_task.c
@@ -8,7 +8,7 @@
 #include <linux/interrupt.h>
 #include <linux/hardirq.h>
 
-#include "rxe_task.h"
+#include "rxe.h"
 
 int __rxe_do_task(struct rxe_task *task)
 
@@ -33,6 +33,7 @@ void rxe_do_task(struct tasklet_struct *t)
 	int cont;
 	int ret;
 	struct rxe_task *task = from_tasklet(task, t, tasklet);
+	unsigned int iterations = RXE_MAX_ITERATIONS;
 
 	spin_lock_bh(&task->state_lock);
 	switch (task->state) {
@@ -61,13 +62,20 @@ void rxe_do_task(struct tasklet_struct *t)
 		spin_lock_bh(&task->state_lock);
 		switch (task->state) {
 		case TASK_STATE_BUSY:
-			if (ret)
+			if (ret) {
 				task->state = TASK_STATE_START;
-			else
+			} else if (iterations--) {
 				cont = 1;
+			} else {
+				/* reschedule the tasklet and exit
+				 * the loop to give up the cpu
+				 */
+				tasklet_schedule(&task->tasklet);
+				task->state = TASK_STATE_START;
+			}
 			break;
 
-		/* soneone tried to run the task since the last time we called
+		/* someone tried to run the task since the last time we called
 		 * func, so we will call one more time regardless of the
 		 * return value
 		 */
-- 
2.34.1

[PATCH for-next v2 9/9] RDMA/rxe: Replace __rxe_do_task by rxe_run_task

[Bob Pearson]

In rxe_req.c replace calls to __rxe_do_task() by calls to
rxe_run_task(.., 0). Using __rxe_do_task is an error because
the completer tasklet is not designed to be re-entrant and
__rxe_do_task() should only be called when it is clear that
no one else could be calling the completer tasklet as is the
case in rxe_qp.c where this call is used in safe environments.

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

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 008ae51c7560..58b9f170b18b 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -723,7 +723,7 @@ int rxe_requester(void *arg)
 						       qp->req.wqe_index);
 			wqe->state = wqe_state_done;
 			wqe->status = IB_WC_SUCCESS;
-			__rxe_do_task(&qp->comp.task);
+			rxe_run_task(&qp->comp.task, 0);
 			goto done;
 		}
 		payload = mtu;
@@ -804,7 +804,7 @@ int rxe_requester(void *arg)
 	goto out;
 err:
 	wqe->state = wqe_state_error;
-	__rxe_do_task(&qp->comp.task);
+	rxe_run_task(&qp->comp.task, 0);
 exit:
 	ret = -EAGAIN;
 out:
-- 
2.34.1

Re: [PATCH for-next v2 9/9] RDMA/rxe: Replace __rxe_do_task by rxe_run_task

[lizhijian]

On 01/07/2022 03:04, Bob Pearson wrote:
> In rxe_req.c replace calls to __rxe_do_task() by calls to
> rxe_run_task(.., 0). Using __rxe_do_task is an error because
> the completer tasklet is not designed to be re-entrant and
> __rxe_do_task() should only be called when it is clear that
> no one else could be calling the completer tasklet as is the
> case in rxe_qp.c where this call is used in safe environments.
>
> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
LGTM，

Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>

> ---
>   drivers/infiniband/sw/rxe/rxe_req.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 008ae51c7560..58b9f170b18b 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -723,7 +723,7 @@ int rxe_requester(void *arg)
>   						       qp->req.wqe_index);
>   			wqe->state = wqe_state_done;
>   			wqe->status = IB_WC_SUCCESS;
> -			__rxe_do_task(&qp->comp.task);
> +			rxe_run_task(&qp->comp.task, 0);
>   			goto done;
>   		}
>   		payload = mtu;
> @@ -804,7 +804,7 @@ int rxe_requester(void *arg)
>   	goto out;
>   err:
>   	wqe->state = wqe_state_error;
> -	__rxe_do_task(&qp->comp.task);
> +	rxe_run_task(&qp->comp.task, 0);
>   exit:
>   	ret = -EAGAIN;
>   out:

Re: [PATCH for-next v2 0/9] RDMA/rxe: Various fixes

[Jason Gunthorpe]

On Thu, Jun 30, 2022 at 02:04:17PM -0500, Bob Pearson wrote:
> Bob Pearson (9):
>   RDMA/rxe: Convert pr_warn/err to pr_debug in pyverbs
>   RDMA/rxe: Remove unnecessary include statement

These two are trivial, I'm going to take them right now

Thanks,
Jason

Re: [PATCH for-next v2 0/9] RDMA/rxe: Various fixes

[Jason Gunthorpe]

On Thu, Jun 30, 2022 at 02:04:17PM -0500, Bob Pearson wrote:
> This patch series is a collection of fixes to the rdma_rxe driver.
> Some are new and some are replacements of eariler patches that are
> rebased and improved.
> 
> These patches apply cleanly to current wip/jgg-for-next.
> 
> Bob Pearson (9):
>   RDMA/rxe: Add rxe_is_fenced() subroutine
>   RDMA/rxe: Convert pr_warn/err to pr_debug in pyverbs
>   RDMA/rxe: Remove unnecessary include statement
>   RDMA/rxe: Replace include statement
>   RDMA/rxe: Fix rnr retry behavior
>   RDMA/rxe: Fix deadlock in rxe_do_local_ops()
>   RDMA/rxe: Make the tasklet exits the same
>   RDMA/rxe: Limit the number of calls to each tasklet
>   RDMA/rxe: Replace __rxe_do_task by rxe_run_task

Applied to for-next, thanks

Jason