All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] iSER fixes and cleanups for kernel-5.17
@ 2021-12-15 13:57 Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 1/6] IB/iser: remove deprecated pi_guard module param Max Gurtovoy
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

Hi Jason and Sagi,
This series is the first fixes and cleanups for iSER intiator that we
aim for kernel-5.17 edition.

It starts with removing deprecated module parameter from ib_iser driver
(patch 1/6).

The series continues with a patch (SergeyG) that fixes RNR messages sent
to the target HCA since there are not enough credits/space in the receive
queue (patch 2/6).

Patch 3/6 is a simple renaming patch.

Patch 4/6 is a preparation patch to eventually guarantee that the HCA
will never perform an access violation when retrying a send operation
(same as done in NVMe-oF).

Patches 5/6 and 6/6 are some cleanups and coding style fixes to help
maintaining the driver.

Max Gurtovoy (5):
  IB/iser: remove deprecated pi_guard module param
  IB/iser: rename ib_ret local variable
  IB/iser: don't suppress send completions
  IB/iser: remove un-needed casting to/from void pointer
  IB/iser: align coding style accross driver

Sergey Gorenko (1):
  IB/iser: fix RNR errors

 drivers/infiniband/ulp/iser/iscsi_iser.c     |  78 ++++-------
 drivers/infiniband/ulp/iser/iscsi_iser.h     |  23 +---
 drivers/infiniband/ulp/iser/iser_initiator.c | 106 ++++++---------
 drivers/infiniband/ulp/iser/iser_memory.c    |  60 ++++----
 drivers/infiniband/ulp/iser/iser_verbs.c     | 136 ++++++++-----------
 5 files changed, 156 insertions(+), 247 deletions(-)

-- 
2.18.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/6] IB/iser: remove deprecated pi_guard module param
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
@ 2021-12-15 13:57 ` Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 2/6] IB/iser: fix RNR errors Max Gurtovoy
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

No need for this dead code. This commit doesn't change any
functionality since one can still run "modprobe ib_iser
pi_guard=<type>".

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 4 ----
 drivers/infiniband/ulp/iser/iscsi_iser.h | 1 -
 2 files changed, 5 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 776e46ee95da..410df19bdfb5 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -113,10 +113,6 @@ bool iser_pi_enable = false;
 module_param_named(pi_enable, iser_pi_enable, bool, S_IRUGO);
 MODULE_PARM_DESC(pi_enable, "Enable T10-PI offload support (default:disabled)");
 
-int iser_pi_guard;
-module_param_named(pi_guard, iser_pi_guard, int, S_IRUGO);
-MODULE_PARM_DESC(pi_guard, "T10-PI guard_type [deprecated]");
-
 static int iscsi_iser_set(const char *val, const struct kernel_param *kp)
 {
 	int ret;
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 9f6ac0a09a78..22d2586b08cd 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -486,7 +486,6 @@ struct iser_global {
 extern struct iser_global ig;
 extern int iser_debug_level;
 extern bool iser_pi_enable;
-extern int iser_pi_guard;
 extern unsigned int iser_max_sectors;
 extern bool iser_always_reg;
 
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/6] IB/iser: fix RNR errors
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 1/6] IB/iser: remove deprecated pi_guard module param Max Gurtovoy
@ 2021-12-15 13:57 ` Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 3/6] IB/iser: rename ib_ret local variable Max Gurtovoy
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

From: Sergey Gorenko <sergeygo@nvidia.com>

Some users complain about RNR errors on the target, when heavy
high-priority tasks run on the initiator. After the investigation, we
found out that the receive WRs were exhausted, because the initiator
could not post them on time.

Receive work reqeusts are posted in chunks to reduce the number of hits
to the HCA. The WRs are posted in the receive completion handler when
the number of free receive buffers reaches the threshold. But on a
high-loaded host, receive CQEs processing can be delayed and all receive
WRs will be exhausted. In this case, the target will get an RNR error.

To avoid this, we post receive WR, as soon as possible and not in a
batch. This increases the number of hits to the HCA, but also the common
implementation in most of Linux ULPs (e.g. NVMe-oF/RDMA). As a rule of
thumb, performance improvements and heuristics are being added to the
RDMA core layer or vendors low level drivers and it's about time to
align iSER as well.

Signed-off-by: Sergey Gorenko <sergeygo@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.h     | 15 +----
 drivers/infiniband/ulp/iser/iser_initiator.c | 64 +++++++++-----------
 drivers/infiniband/ulp/iser/iser_verbs.c     | 41 ++++---------
 3 files changed, 42 insertions(+), 78 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 22d2586b08cd..05a95d5b25f0 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -119,8 +119,6 @@
 
 #define ISER_QP_MAX_RECV_DTOS		(ISER_DEF_XMIT_CMDS_MAX)
 
-#define ISER_MIN_POSTED_RX		(ISER_DEF_XMIT_CMDS_MAX >> 2)
-
 /* the max TX (send) WR supported by the iSER QP is defined by                 *
  * max_send_wr = T * (1 + D) + C ; D is how many inflight dataouts we expect   *
  * to have at max for SCSI command. The tx posting & completion handling code  *
@@ -366,9 +364,7 @@ struct iser_fr_pool {
  * @qp:                  Connection Queue-pair
  * @cq:                  Connection completion queue
  * @cq_size:             The number of max outstanding completions
- * @post_recv_buf_count: post receive counter
  * @sig_count:           send work request signal count
- * @rx_wr:               receive work request for batch posts
  * @device:              reference to iser device
  * @fr_pool:             connection fast registration poool
  * @pi_support:          Indicate device T10-PI support
@@ -379,9 +375,7 @@ struct ib_conn {
 	struct ib_qp	            *qp;
 	struct ib_cq		    *cq;
 	u32			    cq_size;
-	int                          post_recv_buf_count;
 	u8                           sig_count;
-	struct ib_recv_wr	     rx_wr[ISER_MIN_POSTED_RX];
 	struct iser_device          *device;
 	struct iser_fr_pool          fr_pool;
 	bool			     pi_support;
@@ -397,8 +391,6 @@ struct ib_conn {
  * @state:            connection logical state
  * @qp_max_recv_dtos: maximum number of data outs, corresponds
  *                    to max number of post recvs
- * @qp_max_recv_dtos_mask: (qp_max_recv_dtos - 1)
- * @min_posted_rx:    (qp_max_recv_dtos >> 2)
  * @max_cmds:         maximum cmds allowed for this connection
  * @name:             connection peer portal
  * @release_work:     deffered work for release job
@@ -409,7 +401,6 @@ struct ib_conn {
  *                    (state is ISER_CONN_UP)
  * @conn_list:        entry in ig conn list
  * @login_desc:       login descriptor
- * @rx_desc_head:     head of rx_descs cyclic buffer
  * @rx_descs:         rx buffers array (cyclic buffer)
  * @num_rx_descs:     number of rx descriptors
  * @scsi_sg_tablesize: scsi host sg_tablesize
@@ -422,8 +413,6 @@ struct iser_conn {
 	struct iscsi_endpoint	     *ep;
 	enum iser_conn_state	     state;
 	unsigned		     qp_max_recv_dtos;
-	unsigned		     qp_max_recv_dtos_mask;
-	unsigned		     min_posted_rx;
 	u16                          max_cmds;
 	char 			     name[ISER_OBJECT_NAME_SIZE];
 	struct work_struct	     release_work;
@@ -433,7 +422,6 @@ struct iser_conn {
 	struct completion	     up_completion;
 	struct list_head	     conn_list;
 	struct iser_login_desc       login_desc;
-	unsigned int 		     rx_desc_head;
 	struct iser_rx_desc	     *rx_descs;
 	u32                          num_rx_descs;
 	unsigned short               scsi_sg_tablesize;
@@ -542,7 +530,8 @@ int  iser_connect(struct iser_conn *iser_conn,
 		  int non_blocking);
 
 int  iser_post_recvl(struct iser_conn *iser_conn);
-int  iser_post_recvm(struct iser_conn *iser_conn, int count);
+int  iser_post_recvm(struct iser_conn *iser_conn,
+		     struct iser_rx_desc *rx_desc);
 int  iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc,
 		    bool signal);
 
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 27a6f75a9912..ca22b6d1f5e3 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -247,8 +247,6 @@ int iser_alloc_rx_descriptors(struct iser_conn *iser_conn,
 	struct iser_device *device = ib_conn->device;
 
 	iser_conn->qp_max_recv_dtos = session->cmds_max;
-	iser_conn->qp_max_recv_dtos_mask = session->cmds_max - 1; /* cmds_max is 2^N */
-	iser_conn->min_posted_rx = iser_conn->qp_max_recv_dtos >> 2;
 
 	if (iser_alloc_fastreg_pool(ib_conn, session->scsi_cmds_max,
 				    iser_conn->pages_per_mr))
@@ -280,7 +278,6 @@ int iser_alloc_rx_descriptors(struct iser_conn *iser_conn,
 		rx_sg->lkey = device->pd->local_dma_lkey;
 	}
 
-	iser_conn->rx_desc_head = 0;
 	return 0;
 
 rx_desc_dma_map_failed:
@@ -322,32 +319,35 @@ void iser_free_rx_descriptors(struct iser_conn *iser_conn)
 static int iser_post_rx_bufs(struct iscsi_conn *conn, struct iscsi_hdr *req)
 {
 	struct iser_conn *iser_conn = conn->dd_data;
-	struct ib_conn *ib_conn = &iser_conn->ib_conn;
 	struct iscsi_session *session = conn->session;
+	int err = 0;
+	int i;
 
 	iser_dbg("req op %x flags %x\n", req->opcode, req->flags);
 	/* check if this is the last login - going to full feature phase */
 	if ((req->flags & ISCSI_FULL_FEATURE_PHASE) != ISCSI_FULL_FEATURE_PHASE)
-		return 0;
-
-	/*
-	 * Check that there is one posted recv buffer
-	 * (for the last login response).
-	 */
-	WARN_ON(ib_conn->post_recv_buf_count != 1);
+		goto out;
 
 	if (session->discovery_sess) {
 		iser_info("Discovery session, re-using login RX buffer\n");
-		return 0;
-	} else
-		iser_info("Normal session, posting batch of RX %d buffers\n",
-			  iser_conn->min_posted_rx);
+		goto out;
+	}
 
-	/* Initial post receive buffers */
-	if (iser_post_recvm(iser_conn, iser_conn->min_posted_rx))
-		return -ENOMEM;
+	iser_info("Normal session, posting batch of RX %d buffers\n",
+		  iser_conn->qp_max_recv_dtos - 1);
 
-	return 0;
+	/*
+	 * Initial post receive buffers.
+	 * There is one already posted recv buffer (for the last login
+	 * response). Therefore, the first recv buffer is skipped here.
+	 */
+	for (i = 1; i < iser_conn->qp_max_recv_dtos; i++) {
+		err = iser_post_recvm(iser_conn, &iser_conn->rx_descs[i]);
+		if (err)
+			goto out;
+	}
+out:
+	return err;
 }
 
 static inline bool iser_signal_comp(u8 sig_count)
@@ -590,7 +590,11 @@ void iser_login_rsp(struct ib_cq *cq, struct ib_wc *wc)
 				      desc->rsp_dma, ISER_RX_LOGIN_SIZE,
 				      DMA_FROM_DEVICE);
 
-	ib_conn->post_recv_buf_count--;
+	if (iser_conn->iscsi_conn->session->discovery_sess)
+		return;
+
+	/* Post the first RX buffer that is skipped in iser_post_rx_bufs() */
+	iser_post_recvm(iser_conn, iser_conn->rx_descs);
 }
 
 static inline int
@@ -657,8 +661,7 @@ void iser_task_rsp(struct ib_cq *cq, struct ib_wc *wc)
 	struct iser_conn *iser_conn = to_iser_conn(ib_conn);
 	struct iser_rx_desc *desc = iser_rx(wc->wr_cqe);
 	struct iscsi_hdr *hdr;
-	int length;
-	int outstanding, count, err;
+	int length, err;
 
 	if (unlikely(wc->status != IB_WC_SUCCESS)) {
 		iser_err_comp(wc, "task_rsp");
@@ -687,20 +690,9 @@ void iser_task_rsp(struct ib_cq *cq, struct ib_wc *wc)
 				      desc->dma_addr, ISER_RX_PAYLOAD_SIZE,
 				      DMA_FROM_DEVICE);
 
-	/* decrementing conn->post_recv_buf_count only --after-- freeing the   *
-	 * task eliminates the need to worry on tasks which are completed in   *
-	 * parallel to the execution of iser_conn_term. So the code that waits *
-	 * for the posted rx bufs refcount to become zero handles everything   */
-	ib_conn->post_recv_buf_count--;
-
-	outstanding = ib_conn->post_recv_buf_count;
-	if (outstanding + iser_conn->min_posted_rx <= iser_conn->qp_max_recv_dtos) {
-		count = min(iser_conn->qp_max_recv_dtos - outstanding,
-			    iser_conn->min_posted_rx);
-		err = iser_post_recvm(iser_conn, count);
-		if (err)
-			iser_err("posting %d rx bufs err %d\n", count, err);
-	}
+	err = iser_post_recvm(iser_conn, desc);
+	if (err)
+		iser_err("posting rx buffer err %d\n", err);
 }
 
 void iser_cmd_comp(struct ib_cq *cq, struct ib_wc *wc)
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index b566f7cb7797..e272390bc492 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -757,7 +757,6 @@ void iser_conn_init(struct iser_conn *iser_conn)
 	INIT_LIST_HEAD(&iser_conn->conn_list);
 	mutex_init(&iser_conn->state_mutex);
 
-	ib_conn->post_recv_buf_count = 0;
 	ib_conn->reg_cqe.done = iser_reg_comp;
 }
 
@@ -841,44 +840,28 @@ int iser_post_recvl(struct iser_conn *iser_conn)
 	wr.num_sge = 1;
 	wr.next = NULL;
 
-	ib_conn->post_recv_buf_count++;
 	ib_ret = ib_post_recv(ib_conn->qp, &wr, NULL);
-	if (ib_ret) {
-		iser_err("ib_post_recv failed ret=%d\n", ib_ret);
-		ib_conn->post_recv_buf_count--;
-	}
+	if (unlikely(ib_ret))
+		iser_err("ib_post_recv login failed ret=%d\n", ib_ret);
 
 	return ib_ret;
 }
 
-int iser_post_recvm(struct iser_conn *iser_conn, int count)
+int iser_post_recvm(struct iser_conn *iser_conn, struct iser_rx_desc *rx_desc)
 {
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
-	unsigned int my_rx_head = iser_conn->rx_desc_head;
-	struct iser_rx_desc *rx_desc;
-	struct ib_recv_wr *wr;
-	int i, ib_ret;
-
-	for (wr = ib_conn->rx_wr, i = 0; i < count; i++, wr++) {
-		rx_desc = &iser_conn->rx_descs[my_rx_head];
-		rx_desc->cqe.done = iser_task_rsp;
-		wr->wr_cqe = &rx_desc->cqe;
-		wr->sg_list = &rx_desc->rx_sg;
-		wr->num_sge = 1;
-		wr->next = wr + 1;
-		my_rx_head = (my_rx_head + 1) & iser_conn->qp_max_recv_dtos_mask;
-	}
+	struct ib_recv_wr wr;
+	int ib_ret;
 
-	wr--;
-	wr->next = NULL; /* mark end of work requests list */
+	rx_desc->cqe.done = iser_task_rsp;
+	wr.wr_cqe = &rx_desc->cqe;
+	wr.sg_list = &rx_desc->rx_sg;
+	wr.num_sge = 1;
+	wr.next = NULL;
 
-	ib_conn->post_recv_buf_count += count;
-	ib_ret = ib_post_recv(ib_conn->qp, ib_conn->rx_wr, NULL);
-	if (unlikely(ib_ret)) {
+	ib_ret = ib_post_recv(ib_conn->qp, &wr, NULL);
+	if (unlikely(ib_ret))
 		iser_err("ib_post_recv failed ret=%d\n", ib_ret);
-		ib_conn->post_recv_buf_count -= count;
-	} else
-		iser_conn->rx_desc_head = my_rx_head;
 
 	return ib_ret;
 }
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/6] IB/iser: rename ib_ret local variable
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 1/6] IB/iser: remove deprecated pi_guard module param Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 2/6] IB/iser: fix RNR errors Max Gurtovoy
@ 2021-12-15 13:57 ` Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 4/6] IB/iser: don't suppress send completions Max Gurtovoy
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

Use more common name for return values ("ret"). This commit doesn't
change any logic.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 30 ++++++++++++------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index e272390bc492..e0d7119a2c40 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -828,7 +828,7 @@ int iser_post_recvl(struct iser_conn *iser_conn)
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
 	struct iser_login_desc *desc = &iser_conn->login_desc;
 	struct ib_recv_wr wr;
-	int ib_ret;
+	int ret;
 
 	desc->sge.addr = desc->rsp_dma;
 	desc->sge.length = ISER_RX_LOGIN_SIZE;
@@ -840,18 +840,18 @@ int iser_post_recvl(struct iser_conn *iser_conn)
 	wr.num_sge = 1;
 	wr.next = NULL;
 
-	ib_ret = ib_post_recv(ib_conn->qp, &wr, NULL);
-	if (unlikely(ib_ret))
-		iser_err("ib_post_recv login failed ret=%d\n", ib_ret);
+	ret = ib_post_recv(ib_conn->qp, &wr, NULL);
+	if (unlikely(ret))
+		iser_err("ib_post_recv login failed ret=%d\n", ret);
 
-	return ib_ret;
+	return ret;
 }
 
 int iser_post_recvm(struct iser_conn *iser_conn, struct iser_rx_desc *rx_desc)
 {
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
 	struct ib_recv_wr wr;
-	int ib_ret;
+	int ret;
 
 	rx_desc->cqe.done = iser_task_rsp;
 	wr.wr_cqe = &rx_desc->cqe;
@@ -859,11 +859,11 @@ int iser_post_recvm(struct iser_conn *iser_conn, struct iser_rx_desc *rx_desc)
 	wr.num_sge = 1;
 	wr.next = NULL;
 
-	ib_ret = ib_post_recv(ib_conn->qp, &wr, NULL);
-	if (unlikely(ib_ret))
-		iser_err("ib_post_recv failed ret=%d\n", ib_ret);
+	ret = ib_post_recv(ib_conn->qp, &wr, NULL);
+	if (unlikely(ret))
+		iser_err("ib_post_recv failed ret=%d\n", ret);
 
-	return ib_ret;
+	return ret;
 }
 
 
@@ -880,7 +880,7 @@ int iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc,
 {
 	struct ib_send_wr *wr = &tx_desc->send_wr;
 	struct ib_send_wr *first_wr;
-	int ib_ret;
+	int ret;
 
 	ib_dma_sync_single_for_device(ib_conn->device->ib_device,
 				      tx_desc->dma_addr, ISER_HEADERS_LEN,
@@ -900,12 +900,12 @@ int iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc,
 	else
 		first_wr = wr;
 
-	ib_ret = ib_post_send(ib_conn->qp, first_wr, NULL);
-	if (unlikely(ib_ret))
+	ret = ib_post_send(ib_conn->qp, first_wr, NULL);
+	if (unlikely(ret))
 		iser_err("ib_post_send failed, ret:%d opcode:%d\n",
-			 ib_ret, wr->opcode);
+			 ret, wr->opcode);
 
-	return ib_ret;
+	return ret;
 }
 
 u8 iser_check_task_pi_status(struct iscsi_iser_task *iser_task,
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/6] IB/iser: don't suppress send completions
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
                   ` (2 preceding siblings ...)
  2021-12-15 13:57 ` [PATCH 3/6] IB/iser: rename ib_ret local variable Max Gurtovoy
@ 2021-12-15 13:57 ` Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 5/6] IB/iser: remove un-needed casting to/from void pointer Max Gurtovoy
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

In order to complete a scsi command and guarantee that the HCA will
never perform an access violation when retrying a send operation we must
complete a scsi request only when both send and receive completions
has arrived. This is a preparation commit that remove the send
completions suppression. Next step will be taking care of the local
invalidation mechanism and adding a reference counter for commands.
Currently, we don't do anything upon getting the send completion and
just "consume" it.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.h     |  7 +------
 drivers/infiniband/ulp/iser/iser_initiator.c | 13 +++----------
 drivers/infiniband/ulp/iser/iser_verbs.c     |  6 ++----
 3 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 05a95d5b25f0..20af46c4e954 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -146,8 +146,6 @@
 					 - ISER_MAX_RX_MISC_PDUS) /	\
 					 (1 + ISER_INFLIGHT_DATAOUTS))
 
-#define ISER_SIGNAL_CMD_COUNT 32
-
 /* Constant PDU lengths calculations */
 #define ISER_HEADERS_LEN	(sizeof(struct iser_ctrl) + sizeof(struct iscsi_hdr))
 
@@ -364,7 +362,6 @@ struct iser_fr_pool {
  * @qp:                  Connection Queue-pair
  * @cq:                  Connection completion queue
  * @cq_size:             The number of max outstanding completions
- * @sig_count:           send work request signal count
  * @device:              reference to iser device
  * @fr_pool:             connection fast registration poool
  * @pi_support:          Indicate device T10-PI support
@@ -375,7 +372,6 @@ struct ib_conn {
 	struct ib_qp	            *qp;
 	struct ib_cq		    *cq;
 	u32			    cq_size;
-	u8                           sig_count;
 	struct iser_device          *device;
 	struct iser_fr_pool          fr_pool;
 	bool			     pi_support;
@@ -532,8 +528,7 @@ int  iser_connect(struct iser_conn *iser_conn,
 int  iser_post_recvl(struct iser_conn *iser_conn);
 int  iser_post_recvm(struct iser_conn *iser_conn,
 		     struct iser_rx_desc *rx_desc);
-int  iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc,
-		    bool signal);
+int  iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc);
 
 int iser_dma_map_task_data(struct iscsi_iser_task *iser_task,
 			   struct iser_data_buf *data,
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index ca22b6d1f5e3..778835003d39 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -350,11 +350,6 @@ static int iser_post_rx_bufs(struct iscsi_conn *conn, struct iscsi_hdr *req)
 	return err;
 }
 
-static inline bool iser_signal_comp(u8 sig_count)
-{
-	return ((sig_count % ISER_SIGNAL_CMD_COUNT) == 0);
-}
-
 /**
  * iser_send_command - send command PDU
  * @conn: link to matching iscsi connection
@@ -371,7 +366,6 @@ int iser_send_command(struct iscsi_conn *conn,
 	struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)task->hdr;
 	struct scsi_cmnd *sc  =  task->sc;
 	struct iser_tx_desc *tx_desc = &iser_task->desc;
-	u8 sig_count = ++iser_conn->ib_conn.sig_count;
 
 	edtl = ntohl(hdr->data_length);
 
@@ -418,8 +412,7 @@ int iser_send_command(struct iscsi_conn *conn,
 
 	iser_task->status = ISER_TASK_STATUS_STARTED;
 
-	err = iser_post_send(&iser_conn->ib_conn, tx_desc,
-			     iser_signal_comp(sig_count));
+	err = iser_post_send(&iser_conn->ib_conn, tx_desc);
 	if (!err)
 		return 0;
 
@@ -487,7 +480,7 @@ int iser_send_data_out(struct iscsi_conn *conn,
 		 itt, buf_offset, data_seg_len);
 
 
-	err = iser_post_send(&iser_conn->ib_conn, tx_desc, true);
+	err = iser_post_send(&iser_conn->ib_conn, tx_desc);
 	if (!err)
 		return 0;
 
@@ -550,7 +543,7 @@ int iser_send_control(struct iscsi_conn *conn,
 			goto send_control_error;
 	}
 
-	err = iser_post_send(&iser_conn->ib_conn, mdesc, true);
+	err = iser_post_send(&iser_conn->ib_conn, mdesc);
 	if (!err)
 		return 0;
 
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index e0d7119a2c40..53af7f4052ec 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -871,12 +871,10 @@ int iser_post_recvm(struct iser_conn *iser_conn, struct iser_rx_desc *rx_desc)
  * iser_post_send - Initiate a Send DTO operation
  * @ib_conn: connection RDMA resources
  * @tx_desc: iSER TX descriptor
- * @signal: true to send work request as SIGNALED
  *
  * Return: 0 on success, -1 on failure
  */
-int iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc,
-		   bool signal)
+int iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc)
 {
 	struct ib_send_wr *wr = &tx_desc->send_wr;
 	struct ib_send_wr *first_wr;
@@ -891,7 +889,7 @@ int iser_post_send(struct ib_conn *ib_conn, struct iser_tx_desc *tx_desc,
 	wr->sg_list = tx_desc->tx_sg;
 	wr->num_sge = tx_desc->num_sge;
 	wr->opcode = IB_WR_SEND;
-	wr->send_flags = signal ? IB_SEND_SIGNALED : 0;
+	wr->send_flags = IB_SEND_SIGNALED;
 
 	if (tx_desc->inv_wr.next)
 		first_wr = &tx_desc->inv_wr;
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/6] IB/iser: remove un-needed casting to/from void pointer
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
                   ` (3 preceding siblings ...)
  2021-12-15 13:57 ` [PATCH 4/6] IB/iser: don't suppress send completions Max Gurtovoy
@ 2021-12-15 13:57 ` Max Gurtovoy
  2021-12-15 13:57 ` [PATCH 6/6] IB/iser: align coding style accross driver Max Gurtovoy
  2022-01-06  0:39 ` [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Jason Gunthorpe
  6 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

The void pointer can be typecasted to/from any type.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 53af7f4052ec..afef5a2a7329 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -501,7 +501,7 @@ static void iser_connect_error(struct rdma_cm_id *cma_id)
 {
 	struct iser_conn *iser_conn;
 
-	iser_conn = (struct iser_conn *)cma_id->context;
+	iser_conn = cma_id->context;
 	iser_conn->state = ISER_CONN_TERMINATING;
 }
 
@@ -549,7 +549,7 @@ static void iser_addr_handler(struct rdma_cm_id *cma_id)
 	struct ib_conn   *ib_conn;
 	int    ret;
 
-	iser_conn = (struct iser_conn *)cma_id->context;
+	iser_conn = cma_id->context;
 	if (iser_conn->state != ISER_CONN_PENDING)
 		/* bailout */
 		return;
@@ -595,7 +595,7 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
 	struct rdma_conn_param conn_param;
 	int    ret;
 	struct iser_cm_hdr req_hdr;
-	struct iser_conn *iser_conn = (struct iser_conn *)cma_id->context;
+	struct iser_conn *iser_conn = cma_id->context;
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
 	struct ib_device *ib_dev = ib_conn->device->ib_device;
 
@@ -638,7 +638,7 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id,
 	struct ib_qp_attr attr;
 	struct ib_qp_init_attr init_attr;
 
-	iser_conn = (struct iser_conn *)cma_id->context;
+	iser_conn = cma_id->context;
 	if (iser_conn->state != ISER_CONN_PENDING)
 		/* bailout */
 		return;
@@ -661,7 +661,7 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id,
 
 static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
 {
-	struct iser_conn *iser_conn = (struct iser_conn *)cma_id->context;
+	struct iser_conn *iser_conn = cma_id->context;
 
 	if (iser_conn_terminate(iser_conn)) {
 		if (iser_conn->iscsi_conn)
@@ -675,7 +675,7 @@ static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
 static void iser_cleanup_handler(struct rdma_cm_id *cma_id,
 				 bool destroy)
 {
-	struct iser_conn *iser_conn = (struct iser_conn *)cma_id->context;
+	struct iser_conn *iser_conn = cma_id->context;
 
 	/*
 	 * We are not guaranteed that we visited disconnected_handler
@@ -692,7 +692,7 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
 	struct iser_conn *iser_conn;
 	int ret = 0;
 
-	iser_conn = (struct iser_conn *)cma_id->context;
+	iser_conn = cma_id->context;
 	iser_info("%s (%d): status %d conn %p id %p\n",
 		  rdma_event_msg(event->event), event->event,
 		  event->status, cma_id->context, cma_id);
@@ -784,8 +784,7 @@ int iser_connect(struct iser_conn   *iser_conn,
 	iser_conn->state = ISER_CONN_PENDING;
 
 	ib_conn->cma_id = rdma_create_id(&init_net, iser_cma_handler,
-					 (void *)iser_conn,
-					 RDMA_PS_TCP, IB_QPT_RC);
+					 iser_conn, RDMA_PS_TCP, IB_QPT_RC);
 	if (IS_ERR(ib_conn->cma_id)) {
 		err = PTR_ERR(ib_conn->cma_id);
 		iser_err("rdma_create_id failed: %d\n", err);
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 6/6] IB/iser: align coding style accross driver
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
                   ` (4 preceding siblings ...)
  2021-12-15 13:57 ` [PATCH 5/6] IB/iser: remove un-needed casting to/from void pointer Max Gurtovoy
@ 2021-12-15 13:57 ` Max Gurtovoy
  2022-01-06  0:39 ` [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Jason Gunthorpe
  6 siblings, 0 replies; 8+ messages in thread
From: Max Gurtovoy @ 2021-12-15 13:57 UTC (permalink / raw)
  To: linux-rdma, sagi, jgg; +Cc: oren, israelr, sergeygo, Max Gurtovoy

The following changes were made:
1. Align function signatures to 80 characters per line.
2. Remove tabs for variable assignment and use 1 space instead.
3. Don't compare to NULL in "if" clause.
4. Remove strange indentations.

This will ease on the maintenance of the driver for the future.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c     | 74 ++++++++------------
 drivers/infiniband/ulp/iser/iser_initiator.c | 29 +++-----
 drivers/infiniband/ulp/iser/iser_memory.c    | 60 +++++++---------
 drivers/infiniband/ulp/iser/iser_verbs.c     | 52 +++++++-------
 4 files changed, 90 insertions(+), 125 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 410df19bdfb5..db0ca548bbd9 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -135,9 +135,8 @@ static int iscsi_iser_set(const char *val, const struct kernel_param *kp)
  * Notes: In case of data length errors or iscsi PDU completion failures
  *        this routine will signal iscsi layer of connection failure.
  */
-void
-iscsi_iser_recv(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
-		char *rx_data, int rx_data_len)
+void iscsi_iser_recv(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
+		     char *rx_data, int rx_data_len)
 {
 	int rc = 0;
 	int datalen;
@@ -172,8 +171,7 @@ iscsi_iser_recv(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
  * Netes: This routine can't fail, just assign iscsi task
  *        hdr and max hdr size.
  */
-static int
-iscsi_iser_pdu_alloc(struct iscsi_task *task, uint8_t opcode)
+static int iscsi_iser_pdu_alloc(struct iscsi_task *task, uint8_t opcode)
 {
 	struct iscsi_iser_task *iser_task = task->dd_data;
 
@@ -194,9 +192,8 @@ iscsi_iser_pdu_alloc(struct iscsi_task *task, uint8_t opcode)
  * state mutex to avoid dereferencing the IB device which
  * may have already been terminated.
  */
-int
-iser_initialize_task_headers(struct iscsi_task *task,
-			     struct iser_tx_desc *tx_desc)
+int iser_initialize_task_headers(struct iscsi_task *task,
+				 struct iser_tx_desc *tx_desc)
 {
 	struct iser_conn *iser_conn = task->conn->dd_data;
 	struct iser_device *device = iser_conn->ib_conn.device;
@@ -233,8 +230,7 @@ iser_initialize_task_headers(struct iscsi_task *task,
  * Return: Returns zero on success or -ENOMEM when failing
  *         to init task headers (dma mapping error).
  */
-static int
-iscsi_iser_task_init(struct iscsi_task *task)
+static int iscsi_iser_task_init(struct iscsi_task *task)
 {
 	struct iscsi_iser_task *iser_task = task->dd_data;
 	int ret;
@@ -268,8 +264,8 @@ iscsi_iser_task_init(struct iscsi_task *task)
  *	xmit.
  *
  **/
-static int
-iscsi_iser_mtask_xmit(struct iscsi_conn *conn, struct iscsi_task *task)
+static int iscsi_iser_mtask_xmit(struct iscsi_conn *conn,
+				 struct iscsi_task *task)
 {
 	int error = 0;
 
@@ -286,9 +282,8 @@ iscsi_iser_mtask_xmit(struct iscsi_conn *conn, struct iscsi_task *task)
 	return error;
 }
 
-static int
-iscsi_iser_task_xmit_unsol_data(struct iscsi_conn *conn,
-				 struct iscsi_task *task)
+static int iscsi_iser_task_xmit_unsol_data(struct iscsi_conn *conn,
+					   struct iscsi_task *task)
 {
 	struct iscsi_r2t_info *r2t = &task->unsol_r2t;
 	struct iscsi_data hdr;
@@ -322,8 +317,7 @@ iscsi_iser_task_xmit_unsol_data(struct iscsi_conn *conn,
  *
  * Return: zero on success or escalates $error on failure.
  */
-static int
-iscsi_iser_task_xmit(struct iscsi_task *task)
+static int iscsi_iser_task_xmit(struct iscsi_task *task)
 {
 	struct iscsi_conn *conn = task->conn;
 	struct iscsi_iser_task *iser_task = task->dd_data;
@@ -406,8 +400,8 @@ static void iscsi_iser_cleanup_task(struct iscsi_task *task)
  *
  *         In addition the error sector is marked.
  */
-static u8
-iscsi_iser_check_protection(struct iscsi_task *task, sector_t *sector)
+static u8 iscsi_iser_check_protection(struct iscsi_task *task,
+				      sector_t *sector)
 {
 	struct iscsi_iser_task *iser_task = task->dd_data;
 	enum iser_data_dir dir = iser_task->dir[ISER_DIR_IN] ?
@@ -456,11 +450,10 @@ iscsi_iser_conn_create(struct iscsi_cls_session *cls_session,
  *         -EINVAL in case end-point doesn't exsits anymore or iser connection
  *         state is not UP (teardown already started).
  */
-static int
-iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
-		     struct iscsi_cls_conn *cls_conn,
-		     uint64_t transport_eph,
-		     int is_leading)
+static int iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
+				struct iscsi_cls_conn *cls_conn,
+				uint64_t transport_eph,
+				int is_leading)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
 	struct iser_conn *iser_conn;
@@ -515,8 +508,7 @@ iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
  *        from this point iscsi must call conn_stop in session/connection
  *        teardown so iser transport must wait for it.
  */
-static int
-iscsi_iser_conn_start(struct iscsi_cls_conn *cls_conn)
+static int iscsi_iser_conn_start(struct iscsi_cls_conn *cls_conn)
 {
 	struct iscsi_conn *iscsi_conn;
 	struct iser_conn *iser_conn;
@@ -538,8 +530,7 @@ iscsi_iser_conn_start(struct iscsi_cls_conn *cls_conn)
  *        handle, so we call it under iser the state lock to protect against
  *        this kind of race.
  */
-static void
-iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
+static void iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
 	struct iser_conn *iser_conn = conn->dd_data;
@@ -574,8 +565,7 @@ iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
  *
  * Removes and free iscsi host.
  */
-static void
-iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
+static void iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
 {
 	struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
 
@@ -584,8 +574,7 @@ iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
 	iscsi_host_free(shost);
 }
 
-static inline unsigned int
-iser_dif_prot_caps(int prot_caps)
+static inline unsigned int iser_dif_prot_caps(int prot_caps)
 {
 	int ret = 0;
 
@@ -704,9 +693,8 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 	return NULL;
 }
 
-static int
-iscsi_iser_set_param(struct iscsi_cls_conn *cls_conn,
-		     enum iscsi_param param, char *buf, int buflen)
+static int iscsi_iser_set_param(struct iscsi_cls_conn *cls_conn,
+				enum iscsi_param param, char *buf, int buflen)
 {
 	int value;
 
@@ -756,8 +744,8 @@ iscsi_iser_set_param(struct iscsi_cls_conn *cls_conn,
  *
  * Output connection statistics.
  */
-static void
-iscsi_iser_conn_get_stats(struct iscsi_cls_conn *cls_conn, struct iscsi_stats *stats)
+static void iscsi_iser_conn_get_stats(struct iscsi_cls_conn *cls_conn,
+				      struct iscsi_stats *stats)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
 
@@ -808,9 +796,9 @@ static int iscsi_iser_get_ep_param(struct iscsi_endpoint *ep,
  * Return: iscsi_endpoint created by iscsi layer or ERR_PTR(error)
  *         if fails.
  */
-static struct iscsi_endpoint *
-iscsi_iser_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
-		      int non_blocking)
+static struct iscsi_endpoint *iscsi_iser_ep_connect(struct Scsi_Host *shost,
+						    struct sockaddr *dst_addr,
+						    int non_blocking)
 {
 	int err;
 	struct iser_conn *iser_conn;
@@ -853,8 +841,7 @@ iscsi_iser_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr,
  *         or more likely iser connection state transitioned to TEMINATING or
  *         DOWN during the wait period.
  */
-static int
-iscsi_iser_ep_poll(struct iscsi_endpoint *ep, int timeout_ms)
+static int iscsi_iser_ep_poll(struct iscsi_endpoint *ep, int timeout_ms)
 {
 	struct iser_conn *iser_conn = ep->dd_data;
 	int rc;
@@ -889,8 +876,7 @@ iscsi_iser_ep_poll(struct iscsi_endpoint *ep, int timeout_ms)
  * and cleanup or actually call it immediately in case we didn't pass
  * iscsi conn bind/start stage, thus it is safe.
  */
-static void
-iscsi_iser_ep_disconnect(struct iscsi_endpoint *ep)
+static void iscsi_iser_ep_disconnect(struct iscsi_endpoint *ep)
 {
 	struct iser_conn *iser_conn = ep->dd_data;
 
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 778835003d39..2490150d3085 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -95,11 +95,8 @@ static int iser_prepare_read_cmd(struct iscsi_task *task)
  *  task->data[ISER_DIR_OUT].data_len, Protection size
  *  is stored at task->prot[ISER_DIR_OUT].data_len
  */
-static int
-iser_prepare_write_cmd(struct iscsi_task *task,
-		       unsigned int imm_sz,
-		       unsigned int unsol_sz,
-		       unsigned int edtl)
+static int iser_prepare_write_cmd(struct iscsi_task *task, unsigned int imm_sz,
+				  unsigned int unsol_sz, unsigned int edtl)
 {
 	struct iscsi_iser_task *iser_task = task->dd_data;
 	struct iser_mem_reg *mem_reg;
@@ -160,8 +157,8 @@ iser_prepare_write_cmd(struct iscsi_task *task,
 }
 
 /* creates a new tx descriptor and adds header regd buffer */
-static void iser_create_send_desc(struct iser_conn	*iser_conn,
-				  struct iser_tx_desc	*tx_desc)
+static void iser_create_send_desc(struct iser_conn *iser_conn,
+				  struct iser_tx_desc *tx_desc)
 {
 	struct iser_device *device = iser_conn->ib_conn.device;
 
@@ -355,8 +352,7 @@ static int iser_post_rx_bufs(struct iscsi_conn *conn, struct iscsi_hdr *req)
  * @conn: link to matching iscsi connection
  * @task: SCSI command task
  */
-int iser_send_command(struct iscsi_conn *conn,
-		      struct iscsi_task *task)
+int iser_send_command(struct iscsi_conn *conn, struct iscsi_task *task)
 {
 	struct iser_conn *iser_conn = conn->dd_data;
 	struct iscsi_iser_task *iser_task = task->dd_data;
@@ -427,8 +423,7 @@ int iser_send_command(struct iscsi_conn *conn,
  * @task: SCSI command task
  * @hdr: pointer to the LLD's iSCSI message header
  */
-int iser_send_data_out(struct iscsi_conn *conn,
-		       struct iscsi_task *task,
+int iser_send_data_out(struct iscsi_conn *conn, struct iscsi_task *task,
 		       struct iscsi_data *hdr)
 {
 	struct iser_conn *iser_conn = conn->dd_data;
@@ -490,8 +485,7 @@ int iser_send_data_out(struct iscsi_conn *conn,
 	return err;
 }
 
-int iser_send_control(struct iscsi_conn *conn,
-		      struct iscsi_task *task)
+int iser_send_control(struct iscsi_conn *conn, struct iscsi_task *task)
 {
 	struct iser_conn *iser_conn = conn->dd_data;
 	struct iscsi_iser_task *iser_task = task->dd_data;
@@ -590,8 +584,7 @@ void iser_login_rsp(struct ib_cq *cq, struct ib_wc *wc)
 	iser_post_recvm(iser_conn, iser_conn->rx_descs);
 }
 
-static inline int
-iser_inv_desc(struct iser_fr_desc *desc, u32 rkey)
+static inline int iser_inv_desc(struct iser_fr_desc *desc, u32 rkey)
 {
 	if (unlikely((!desc->sig_protected && rkey != desc->rsc.mr->rkey) ||
 		     (desc->sig_protected && rkey != desc->rsc.sig_mr->rkey))) {
@@ -604,10 +597,8 @@ iser_inv_desc(struct iser_fr_desc *desc, u32 rkey)
 	return 0;
 }
 
-static int
-iser_check_remote_inv(struct iser_conn *iser_conn,
-		      struct ib_wc *wc,
-		      struct iscsi_hdr *hdr)
+static int iser_check_remote_inv(struct iser_conn *iser_conn, struct ib_wc *wc,
+				 struct iscsi_hdr *hdr)
 {
 	if (wc->wc_flags & IB_WC_WITH_INVALIDATE) {
 		struct iscsi_task *task;
diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index 9776b755d848..8b3fe6f7ef45 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -44,8 +44,7 @@ void iser_reg_comp(struct ib_cq *cq, struct ib_wc *wc)
 	iser_err_comp(wc, "memreg");
 }
 
-static struct iser_fr_desc *
-iser_reg_desc_get_fr(struct ib_conn *ib_conn)
+static struct iser_fr_desc *iser_reg_desc_get_fr(struct ib_conn *ib_conn)
 {
 	struct iser_fr_pool *fr_pool = &ib_conn->fr_pool;
 	struct iser_fr_desc *desc;
@@ -60,9 +59,8 @@ iser_reg_desc_get_fr(struct ib_conn *ib_conn)
 	return desc;
 }
 
-static void
-iser_reg_desc_put_fr(struct ib_conn *ib_conn,
-		     struct iser_fr_desc *desc)
+static void iser_reg_desc_put_fr(struct ib_conn *ib_conn,
+				 struct iser_fr_desc *desc)
 {
 	struct iser_fr_pool *fr_pool = &ib_conn->fr_pool;
 	unsigned long flags;
@@ -73,9 +71,9 @@ iser_reg_desc_put_fr(struct ib_conn *ib_conn,
 }
 
 int iser_dma_map_task_data(struct iscsi_iser_task *iser_task,
-			    struct iser_data_buf *data,
-			    enum iser_data_dir iser_dir,
-			    enum dma_data_direction dma_dir)
+			   struct iser_data_buf *data,
+			   enum iser_data_dir iser_dir,
+			   enum dma_data_direction dma_dir)
 {
 	struct ib_device *dev;
 
@@ -100,9 +98,8 @@ void iser_dma_unmap_task_data(struct iscsi_iser_task *iser_task,
 	ib_dma_unmap_sg(dev, data->sg, data->size, dir);
 }
 
-static int
-iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
-	     struct iser_mem_reg *reg)
+static int iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
+			struct iser_mem_reg *reg)
 {
 	struct scatterlist *sg = mem->sg;
 
@@ -154,8 +151,8 @@ void iser_unreg_mem_fastreg(struct iscsi_iser_task *iser_task,
 	reg->mem_h = NULL;
 }
 
-static void
-iser_set_dif_domain(struct scsi_cmnd *sc, struct ib_sig_domain *domain)
+static void iser_set_dif_domain(struct scsi_cmnd *sc,
+				struct ib_sig_domain *domain)
 {
 	domain->sig_type = IB_SIG_TYPE_T10_DIF;
 	domain->sig.dif.pi_interval = scsi_prot_interval(sc);
@@ -171,8 +168,8 @@ iser_set_dif_domain(struct scsi_cmnd *sc, struct ib_sig_domain *domain)
 		domain->sig.dif.ref_remap = true;
 }
 
-static int
-iser_set_sig_attrs(struct scsi_cmnd *sc, struct ib_sig_attrs *sig_attrs)
+static int iser_set_sig_attrs(struct scsi_cmnd *sc,
+			      struct ib_sig_attrs *sig_attrs)
 {
 	switch (scsi_get_prot_op(sc)) {
 	case SCSI_PROT_WRITE_INSERT:
@@ -205,8 +202,7 @@ iser_set_sig_attrs(struct scsi_cmnd *sc, struct ib_sig_attrs *sig_attrs)
 	return 0;
 }
 
-static inline void
-iser_set_prot_checks(struct scsi_cmnd *sc, u8 *mask)
+static inline void iser_set_prot_checks(struct scsi_cmnd *sc, u8 *mask)
 {
 	*mask = 0;
 	if (sc->prot_flags & SCSI_PROT_REF_CHECK)
@@ -215,11 +211,9 @@ iser_set_prot_checks(struct scsi_cmnd *sc, u8 *mask)
 		*mask |= IB_SIG_CHECK_GUARD;
 }
 
-static inline void
-iser_inv_rkey(struct ib_send_wr *inv_wr,
-	      struct ib_mr *mr,
-	      struct ib_cqe *cqe,
-	      struct ib_send_wr *next_wr)
+static inline void iser_inv_rkey(struct ib_send_wr *inv_wr, struct ib_mr *mr,
+				 struct ib_cqe *cqe,
+				 struct ib_send_wr *next_wr)
 {
 	inv_wr->opcode = IB_WR_LOCAL_INV;
 	inv_wr->wr_cqe = cqe;
@@ -229,12 +223,11 @@ iser_inv_rkey(struct ib_send_wr *inv_wr,
 	inv_wr->next = next_wr;
 }
 
-static int
-iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
-		struct iser_data_buf *mem,
-		struct iser_data_buf *sig_mem,
-		struct iser_reg_resources *rsc,
-		struct iser_mem_reg *sig_reg)
+static int iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
+			   struct iser_data_buf *mem,
+			   struct iser_data_buf *sig_mem,
+			   struct iser_reg_resources *rsc,
+			   struct iser_mem_reg *sig_reg)
 {
 	struct iser_tx_desc *tx_desc = &iser_task->desc;
 	struct ib_cqe *cqe = &iser_task->iser_conn->ib_conn.reg_cqe;
@@ -335,12 +328,11 @@ static int iser_fast_reg_mr(struct iscsi_iser_task *iser_task,
 	return 0;
 }
 
-static int
-iser_reg_data_sg(struct iscsi_iser_task *task,
-		 struct iser_data_buf *mem,
-		 struct iser_fr_desc *desc,
-		 bool use_dma_key,
-		 struct iser_mem_reg *reg)
+static int iser_reg_data_sg(struct iscsi_iser_task *task,
+			    struct iser_data_buf *mem,
+			    struct iser_fr_desc *desc,
+			    bool use_dma_key,
+			    struct iser_mem_reg *reg)
 {
 	struct iser_device *device = task->iser_conn->ib_conn.device;
 
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index afef5a2a7329..6e41b0620286 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -265,14 +265,14 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
 	memset(&init_attr, 0, sizeof(init_attr));
 
 	init_attr.event_handler = iser_qp_event_callback;
-	init_attr.qp_context	= (void *)ib_conn;
-	init_attr.send_cq	= ib_conn->cq;
-	init_attr.recv_cq	= ib_conn->cq;
-	init_attr.cap.max_recv_wr  = ISER_QP_MAX_RECV_DTOS;
+	init_attr.qp_context = (void *)ib_conn;
+	init_attr.send_cq = ib_conn->cq;
+	init_attr.recv_cq = ib_conn->cq;
+	init_attr.cap.max_recv_wr = ISER_QP_MAX_RECV_DTOS;
 	init_attr.cap.max_send_sge = 2;
 	init_attr.cap.max_recv_sge = 1;
-	init_attr.sq_sig_type	= IB_SIGNAL_REQ_WR;
-	init_attr.qp_type	= IB_QPT_RC;
+	init_attr.sq_sig_type = IB_SIGNAL_REQ_WR;
+	init_attr.qp_type = IB_QPT_RC;
 	init_attr.cap.max_send_wr = max_send_wr;
 	if (ib_conn->pi_support)
 		init_attr.create_flags |= IB_QP_CREATE_INTEGRITY_EN;
@@ -284,8 +284,7 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
 
 	ib_conn->qp = ib_conn->cma_id->qp;
 	iser_info("setting conn %p cma_id %p qp %p max_send_wr %d\n",
-		  ib_conn, ib_conn->cma_id,
-		  ib_conn->cma_id->qp, max_send_wr);
+		  ib_conn, ib_conn->cma_id, ib_conn->cma_id->qp, max_send_wr);
 	return ret;
 
 out_err:
@@ -313,7 +312,7 @@ struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
 			goto inc_refcnt;
 
 	device = kzalloc(sizeof *device, GFP_KERNEL);
-	if (device == NULL)
+	if (!device)
 		goto out;
 
 	/* assign this device to the device */
@@ -392,8 +391,7 @@ void iser_release_work(struct work_struct *work)
  * so the cm_id removal is out of here. It is Safe to
  * be invoked multiple times.
  */
-static void iser_free_ib_conn_res(struct iser_conn *iser_conn,
-				  bool destroy)
+static void iser_free_ib_conn_res(struct iser_conn *iser_conn, bool destroy)
 {
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
 	struct iser_device *device = ib_conn->device;
@@ -401,7 +399,7 @@ static void iser_free_ib_conn_res(struct iser_conn *iser_conn,
 	iser_info("freeing conn %p cma_id %p qp %p\n",
 		  iser_conn, ib_conn->cma_id, ib_conn->qp);
 
-	if (ib_conn->qp != NULL) {
+	if (ib_conn->qp) {
 		rdma_destroy_qp(ib_conn->cma_id);
 		ib_cq_pool_put(ib_conn->cq, ib_conn->cq_size);
 		ib_conn->qp = NULL;
@@ -411,7 +409,7 @@ static void iser_free_ib_conn_res(struct iser_conn *iser_conn,
 		if (iser_conn->rx_descs)
 			iser_free_rx_descriptors(iser_conn);
 
-		if (device != NULL) {
+		if (device) {
 			iser_device_try_release(device);
 			ib_conn->device = NULL;
 		}
@@ -445,7 +443,7 @@ void iser_conn_release(struct iser_conn *iser_conn)
 	iser_free_ib_conn_res(iser_conn, true);
 	mutex_unlock(&iser_conn->state_mutex);
 
-	if (ib_conn->cma_id != NULL) {
+	if (ib_conn->cma_id) {
 		rdma_destroy_id(ib_conn->cma_id);
 		ib_conn->cma_id = NULL;
 	}
@@ -505,9 +503,8 @@ static void iser_connect_error(struct rdma_cm_id *cma_id)
 	iser_conn->state = ISER_CONN_TERMINATING;
 }
 
-static void
-iser_calc_scsi_params(struct iser_conn *iser_conn,
-		      unsigned int max_sectors)
+static void iser_calc_scsi_params(struct iser_conn *iser_conn,
+				  unsigned int max_sectors)
 {
 	struct iser_device *device = iser_conn->ib_conn.device;
 	struct ib_device_attr *attr = &device->ib_device->attrs;
@@ -545,8 +542,8 @@ iser_calc_scsi_params(struct iser_conn *iser_conn,
 static void iser_addr_handler(struct rdma_cm_id *cma_id)
 {
 	struct iser_device *device;
-	struct iser_conn   *iser_conn;
-	struct ib_conn   *ib_conn;
+	struct iser_conn *iser_conn;
+	struct ib_conn *ib_conn;
 	int    ret;
 
 	iser_conn = cma_id->context;
@@ -593,7 +590,7 @@ static void iser_addr_handler(struct rdma_cm_id *cma_id)
 static void iser_route_handler(struct rdma_cm_id *cma_id)
 {
 	struct rdma_conn_param conn_param;
-	int    ret;
+	int ret;
 	struct iser_cm_hdr req_hdr;
 	struct iser_conn *iser_conn = cma_id->context;
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
@@ -609,9 +606,9 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
 
 	memset(&conn_param, 0, sizeof conn_param);
 	conn_param.responder_resources = ib_dev->attrs.max_qp_rd_atom;
-	conn_param.initiator_depth     = 1;
-	conn_param.retry_count	       = 7;
-	conn_param.rnr_retry_count     = 6;
+	conn_param.initiator_depth = 1;
+	conn_param.retry_count = 7;
+	conn_param.rnr_retry_count = 6;
 
 	memset(&req_hdr, 0, sizeof(req_hdr));
 	req_hdr.flags = ISER_ZBVA_NOT_SUP;
@@ -687,7 +684,8 @@ static void iser_cleanup_handler(struct rdma_cm_id *cma_id,
 	complete(&iser_conn->ib_completion);
 }
 
-static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
+static int iser_cma_handler(struct rdma_cm_id *cma_id,
+			    struct rdma_cm_event *event)
 {
 	struct iser_conn *iser_conn;
 	int ret = 0;
@@ -764,10 +762,8 @@ void iser_conn_init(struct iser_conn *iser_conn)
  * starts the process of connecting to the target
  * sleeps until the connection is established or rejected
  */
-int iser_connect(struct iser_conn   *iser_conn,
-		 struct sockaddr    *src_addr,
-		 struct sockaddr    *dst_addr,
-		 int                 non_blocking)
+int iser_connect(struct iser_conn *iser_conn, struct sockaddr *src_addr,
+		 struct sockaddr *dst_addr, int non_blocking)
 {
 	struct ib_conn *ib_conn = &iser_conn->ib_conn;
 	int err = 0;
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/6] iSER fixes and cleanups for kernel-5.17
  2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
                   ` (5 preceding siblings ...)
  2021-12-15 13:57 ` [PATCH 6/6] IB/iser: align coding style accross driver Max Gurtovoy
@ 2022-01-06  0:39 ` Jason Gunthorpe
  6 siblings, 0 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2022-01-06  0:39 UTC (permalink / raw)
  To: Max Gurtovoy; +Cc: linux-rdma, sagi, oren, israelr, sergeygo

On Wed, Dec 15, 2021 at 03:57:15PM +0200, Max Gurtovoy wrote:
> Hi Jason and Sagi,
> This series is the first fixes and cleanups for iSER intiator that we
> aim for kernel-5.17 edition.
> 
> It starts with removing deprecated module parameter from ib_iser driver
> (patch 1/6).
> 
> The series continues with a patch (SergeyG) that fixes RNR messages sent
> to the target HCA since there are not enough credits/space in the receive
> queue (patch 2/6).
> 
> Patch 3/6 is a simple renaming patch.
> 
> Patch 4/6 is a preparation patch to eventually guarantee that the HCA
> will never perform an access violation when retrying a send operation
> (same as done in NVMe-oF).
> 
> Patches 5/6 and 6/6 are some cleanups and coding style fixes to help
> maintaining the driver.
> 
> Max Gurtovoy (5):
>   IB/iser: remove deprecated pi_guard module param
>   IB/iser: rename ib_ret local variable
>   IB/iser: don't suppress send completions
>   IB/iser: remove un-needed casting to/from void pointer
>   IB/iser: align coding style accross driver
> 
> Sergey Gorenko (1):
>   IB/iser: fix RNR errors

Applied to for-next, thanks

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-01-06  0:39 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-15 13:57 [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Max Gurtovoy
2021-12-15 13:57 ` [PATCH 1/6] IB/iser: remove deprecated pi_guard module param Max Gurtovoy
2021-12-15 13:57 ` [PATCH 2/6] IB/iser: fix RNR errors Max Gurtovoy
2021-12-15 13:57 ` [PATCH 3/6] IB/iser: rename ib_ret local variable Max Gurtovoy
2021-12-15 13:57 ` [PATCH 4/6] IB/iser: don't suppress send completions Max Gurtovoy
2021-12-15 13:57 ` [PATCH 5/6] IB/iser: remove un-needed casting to/from void pointer Max Gurtovoy
2021-12-15 13:57 ` [PATCH 6/6] IB/iser: align coding style accross driver Max Gurtovoy
2022-01-06  0:39 ` [PATCH 0/6] iSER fixes and cleanups for kernel-5.17 Jason Gunthorpe

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.