[PATCH V2 00/20] Cleanup for siw

[PATCH V2 00/20] Cleanup for siw

[Guoqing Jiang]

V2 changes:
1. address W=1 warning in patch 12 and 19 per the report from lkp.
2. add one more patch (20th).

Hi,

This series aim to cleanup siw code, please review and comment!

Thanks,
Guoqing

Guoqing Jiang (20):
  RDMA/siw: Introduce siw_get_page
  RDMA/siw: Introduce siw_srx_update_skb
  RDMA/siw: Use iov.iov_len in kernel_sendmsg
  RDMA/siw: Remove goto lable in siw_mmap
  RDMA/siw: Remove rcu from siw_qp
  RDMA/siw: No need to check term_info.valid before call
    siw_send_terminate
  RDMA/siw: Also goto out_sem_up if pin_user_pages returns 0
  RDMA/siw: Factor out siw_generic_rx helper
  RDMA/siw: Introduce SIW_STAG_MAX_INDEX
  RDMA/siw: Add one parameter to siw_destroy_cpulist
  RDMA/siw: Introduce siw_cep_set_free_and_put
  RDMA/siw: Introduce siw_free_cm_id
  RDMA/siw: Simplify siw_qp_id2obj
  RDMA/siw: Simplify siw_mem_id2obj
  RDMA/siw: Cleanup siw_accept
  RDMA/siw: Remove siw_sk_assign_cm_upcalls
  RDMA/siw: Fix typo
  RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp
  RDMA/siw: Introduce siw_destroy_cep_sock
  RDMA/siw: Update comments for siw_qp_sq_process

 drivers/infiniband/sw/siw/siw.h       |   9 +-
 drivers/infiniband/sw/siw/siw_cm.c    | 154 +++++++++++---------------
 drivers/infiniband/sw/siw/siw_main.c  |  30 +++--
 drivers/infiniband/sw/siw/siw_mem.c   |  22 ++--
 drivers/infiniband/sw/siw/siw_qp.c    |   2 +-
 drivers/infiniband/sw/siw/siw_qp_rx.c |  84 ++++++--------
 drivers/infiniband/sw/siw/siw_qp_tx.c |  39 +++----
 drivers/infiniband/sw/siw/siw_verbs.c |  23 +---
 8 files changed, 144 insertions(+), 219 deletions(-)


base-commit: 964168970cef5f5b738fae047e6de2107842feb7
-- 
2.35.3

[PATCH V2 01/20] RDMA/siw: Introduce siw_get_page

[Guoqing Jiang]

Add the wrapper function to get either pbl page or umem page.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 31 +++++++++++----------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index b2c06100cf01..6a24e08356e9 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -34,6 +34,15 @@ static struct page *siw_get_pblpage(struct siw_mem *mem, u64 addr, int *idx)
 	return NULL;
 }
 
+static struct page *siw_get_page(struct siw_mem *mem, struct siw_sge *sge,
+				 unsigned long offset, int *pbl_idx)
+{
+	if (!mem->is_pbl)
+		return siw_get_upage(mem->umem, sge->laddr + offset);
+	else
+		return siw_get_pblpage(mem, sge->laddr + offset, pbl_idx);
+}
+
 /*
  * Copy short payload at provided destination payload address
  */
@@ -67,11 +76,7 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 			char *buffer;
 			int pbl_idx = 0;
 
-			if (!mem->is_pbl)
-				p = siw_get_upage(mem->umem, sge->laddr);
-			else
-				p = siw_get_pblpage(mem, sge->laddr, &pbl_idx);
-
+			p = siw_get_page(mem, sge, 0, &pbl_idx);
 			if (unlikely(!p))
 				return -EFAULT;
 
@@ -85,13 +90,7 @@ static int siw_try_1seg(struct siw_iwarp_tx *c_tx, void *paddr)
 				memcpy(paddr, buffer + off, part);
 				kunmap_local(buffer);
 
-				if (!mem->is_pbl)
-					p = siw_get_upage(mem->umem,
-							  sge->laddr + part);
-				else
-					p = siw_get_pblpage(mem,
-							    sge->laddr + part,
-							    &pbl_idx);
+				p = siw_get_page(mem, sge, part, &pbl_idx);
 				if (unlikely(!p))
 					return -EFAULT;
 
@@ -502,13 +501,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx, struct socket *s)
 			if (!is_kva) {
 				struct page *p;
 
-				if (mem->is_pbl)
-					p = siw_get_pblpage(
-						mem, sge->laddr + sge_off,
-						&pbl_idx);
-				else
-					p = siw_get_upage(mem->umem,
-							  sge->laddr + sge_off);
+				p = siw_get_page(mem, sge, sge_off, &pbl_idx);
 				if (unlikely(!p)) {
 					siw_unmap_pages(iov, kmap_mask, seg);
 					wqe->processed -= c_tx->bytes_unsent;
-- 
2.35.3

[PATCH V2 02/20] RDMA/siw: Introduce siw_srx_update_skb

[Guoqing Jiang]

There are some places share the same logic, factor a common
helper for it.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_qp_rx.c | 31 +++++++++++----------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
index 33e0fdb362ff..aa7b680452fb 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -881,6 +881,13 @@ int siw_proc_rresp(struct siw_qp *qp)
 	return rv;
 }
 
+static void siw_srx_update_skb(struct siw_rx_stream *srx, u16 length)
+{
+	srx->skb_offset += length;
+	srx->skb_new -= length;
+	srx->skb_copied += length;
+}
+
 int siw_proc_terminate(struct siw_qp *qp)
 {
 	struct siw_rx_stream *srx = &qp->rx_stream;
@@ -925,9 +932,7 @@ int siw_proc_terminate(struct siw_qp *qp)
 		goto out;
 
 	infop += to_copy;
-	srx->skb_offset += to_copy;
-	srx->skb_new -= to_copy;
-	srx->skb_copied += to_copy;
+	siw_srx_update_skb(srx, to_copy);
 	srx->fpdu_part_rcvd += to_copy;
 	srx->fpdu_part_rem -= to_copy;
 
@@ -949,9 +954,7 @@ int siw_proc_terminate(struct siw_qp *qp)
 			   term->flag_m ? "valid" : "invalid");
 	}
 out:
-	srx->skb_new -= to_copy;
-	srx->skb_offset += to_copy;
-	srx->skb_copied += to_copy;
+	siw_srx_update_skb(srx, to_copy);
 	srx->fpdu_part_rcvd += to_copy;
 	srx->fpdu_part_rem -= to_copy;
 
@@ -970,9 +973,7 @@ static int siw_get_trailer(struct siw_qp *qp, struct siw_rx_stream *srx)
 
 	skb_copy_bits(skb, srx->skb_offset, tbuf, avail);
 
-	srx->skb_new -= avail;
-	srx->skb_offset += avail;
-	srx->skb_copied += avail;
+	siw_srx_update_skb(srx, avail);
 	srx->fpdu_part_rem -= avail;
 
 	if (srx->fpdu_part_rem)
@@ -1023,12 +1024,8 @@ static int siw_get_hdr(struct siw_rx_stream *srx)
 		skb_copy_bits(skb, srx->skb_offset,
 			      (char *)c_hdr + srx->fpdu_part_rcvd, bytes);
 
+		siw_srx_update_skb(srx, bytes);
 		srx->fpdu_part_rcvd += bytes;
-
-		srx->skb_new -= bytes;
-		srx->skb_offset += bytes;
-		srx->skb_copied += bytes;
-
 		if (srx->fpdu_part_rcvd < MIN_DDP_HDR)
 			return -EAGAIN;
 
@@ -1091,12 +1088,8 @@ static int siw_get_hdr(struct siw_rx_stream *srx)
 		skb_copy_bits(skb, srx->skb_offset,
 			      (char *)c_hdr + srx->fpdu_part_rcvd, bytes);
 
+		siw_srx_update_skb(srx, bytes);
 		srx->fpdu_part_rcvd += bytes;
-
-		srx->skb_new -= bytes;
-		srx->skb_offset += bytes;
-		srx->skb_copied += bytes;
-
 		if (srx->fpdu_part_rcvd < hdrlen)
 			return -EAGAIN;
 	}
-- 
2.35.3

[PATCH V2 03/20] RDMA/siw: Use iov.iov_len in kernel_sendmsg

[Guoqing Jiang]

We can pass iov.iov_len here.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 6a24e08356e9..2e055b6dcd42 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -296,8 +296,7 @@ static int siw_tx_ctrl(struct siw_iwarp_tx *c_tx, struct socket *s,
 				    (char *)&c_tx->pkt.ctrl + c_tx->ctrl_sent,
 			    .iov_len = c_tx->ctrl_len - c_tx->ctrl_sent };
 
-	int rv = kernel_sendmsg(s, &msg, &iov, 1,
-				c_tx->ctrl_len - c_tx->ctrl_sent);
+	int rv = kernel_sendmsg(s, &msg, &iov, 1, iov.iov_len);
 
 	if (rv >= 0) {
 		c_tx->ctrl_sent += rv;
-- 
2.35.3

[PATCH V2 04/20] RDMA/siw: Remove goto lable in siw_mmap

[Guoqing Jiang]

Remove unnecesary label since the failure case only need to
print warning message.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_verbs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index c5c27db9c2fe..dcd69fc01176 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -66,12 +66,9 @@ int siw_mmap(struct ib_ucontext *ctx, struct vm_area_struct *vma)
 	entry = to_siw_mmap_entry(rdma_entry);
 
 	rv = remap_vmalloc_range(vma, entry->address, 0);
-	if (rv) {
+	if (rv)
 		pr_warn("remap_vmalloc_range failed: %lu, %zu\n", vma->vm_pgoff,
 			size);
-		goto out;
-	}
-out:
 	rdma_user_mmap_entry_put(rdma_entry);
 
 	return rv;
-- 
2.35.3

[PATCH V2 05/20] RDMA/siw: Remove rcu from siw_qp

[Guoqing Jiang]

Remove it since it is not used.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index cec5cccd2e75..44684b74550f 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -466,7 +466,6 @@ struct siw_qp {
 	} term_info;
 	struct rdma_user_mmap_entry *sq_entry; /* mmap info for SQE array */
 	struct rdma_user_mmap_entry *rq_entry; /* mmap info for RQE array */
-	struct rcu_head rcu;
 };
 
 /* helper macros */
-- 
2.35.3

[PATCH V2 06/20] RDMA/siw: No need to check term_info.valid before call siw_send_terminate

[Guoqing Jiang]

Remove the redundate checking since siw_send_terminate check it inside.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_cm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 0a1525d76ba1..c8a9118677d7 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -393,8 +393,7 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 		}
 		siw_dbg_cep(cep, "immediate close, state %d\n", cep->state);
 
-		if (qp->term_info.valid)
-			siw_send_terminate(qp);
+		siw_send_terminate(qp);
 
 		if (cep->cm_id) {
 			switch (cep->state) {
@@ -1060,7 +1059,7 @@ static void siw_cm_work_handler(struct work_struct *w)
 		/*
 		 * QP scheduled LLP close
 		 */
-		if (cep->qp && cep->qp->term_info.valid)
+		if (cep->qp)
 			siw_send_terminate(cep->qp);
 
 		if (cep->cm_id)
-- 
2.35.3

[PATCH V2 07/20] RDMA/siw: Also goto out_sem_up if pin_user_pages returns 0

[Guoqing Jiang]

Since it is legitimate for pin_user_pages returns 0, which
means it might be dead loop here.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index c5f7f1669d09..92c5776a9eed 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -423,7 +423,7 @@ struct siw_umem *siw_umem_get(u64 start, u64 len, bool writable)
 		while (nents) {
 			rv = pin_user_pages(first_page_va, nents, foll_flags,
 					    plist);
-			if (rv < 0)
+			if (rv <= 0)
 				goto out_sem_up;
 
 			umem->num_pages += rv;
-- 
2.35.3

[PATCH V2 08/20] RDMA/siw: Factor out siw_generic_rx helper

[Guoqing Jiang]

Remove the redundant code given they share the same logic.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_qp_rx.c | 53 ++++++++++-----------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
index aa7b680452fb..4931c0c57df0 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -405,6 +405,20 @@ static struct siw_wqe *siw_rqe_get(struct siw_qp *qp)
 	return wqe;
 }
 
+static int siw_generic_rx(struct siw_mem *mem_p, struct siw_rx_stream *srx,
+			  unsigned int *pbl_idx, u64 addr, int bytes)
+{
+	int rv;
+
+	if (mem_p->mem_obj == NULL)
+		rv = siw_rx_kva(srx, ib_virt_dma_to_ptr(addr), bytes);
+	else if (!mem_p->is_pbl)
+		rv = siw_rx_umem(srx, mem_p->umem, addr, bytes);
+	else
+		rv = siw_rx_pbl(srx, pbl_idx, mem_p, addr, bytes);
+	return rv;
+}
+
 /*
  * siw_proc_send:
  *
@@ -485,17 +499,8 @@ int siw_proc_send(struct siw_qp *qp)
 			break;
 		}
 		mem_p = *mem;
-		if (mem_p->mem_obj == NULL)
-			rv = siw_rx_kva(srx,
-				ib_virt_dma_to_ptr(sge->laddr + frx->sge_off),
-				sge_bytes);
-		else if (!mem_p->is_pbl)
-			rv = siw_rx_umem(srx, mem_p->umem,
-					 sge->laddr + frx->sge_off, sge_bytes);
-		else
-			rv = siw_rx_pbl(srx, &frx->pbl_idx, mem_p,
-					sge->laddr + frx->sge_off, sge_bytes);
-
+		rv = siw_generic_rx(mem_p, srx, &frx->pbl_idx,
+				    sge->laddr + frx->sge_off, sge_bytes);
 		if (unlikely(rv != sge_bytes)) {
 			wqe->processed += rcvd_bytes;
 
@@ -598,17 +603,8 @@ int siw_proc_write(struct siw_qp *qp)
 		return -EINVAL;
 	}
 
-	if (mem->mem_obj == NULL)
-		rv = siw_rx_kva(srx,
-			(void *)(uintptr_t)(srx->ddp_to + srx->fpdu_part_rcvd),
-			bytes);
-	else if (!mem->is_pbl)
-		rv = siw_rx_umem(srx, mem->umem,
-				 srx->ddp_to + srx->fpdu_part_rcvd, bytes);
-	else
-		rv = siw_rx_pbl(srx, &frx->pbl_idx, mem,
-				srx->ddp_to + srx->fpdu_part_rcvd, bytes);
-
+	rv = siw_generic_rx(mem, srx, &frx->pbl_idx,
+			    srx->ddp_to + srx->fpdu_part_rcvd, bytes);
 	if (unlikely(rv != bytes)) {
 		siw_init_terminate(qp, TERM_ERROR_LAYER_DDP,
 				   DDP_ETYPE_CATASTROPHIC,
@@ -849,17 +845,8 @@ int siw_proc_rresp(struct siw_qp *qp)
 	mem_p = *mem;
 
 	bytes = min(srx->fpdu_part_rem, srx->skb_new);
-
-	if (mem_p->mem_obj == NULL)
-		rv = siw_rx_kva(srx,
-			ib_virt_dma_to_ptr(sge->laddr + wqe->processed),
-			bytes);
-	else if (!mem_p->is_pbl)
-		rv = siw_rx_umem(srx, mem_p->umem, sge->laddr + wqe->processed,
-				 bytes);
-	else
-		rv = siw_rx_pbl(srx, &frx->pbl_idx, mem_p,
-				sge->laddr + wqe->processed, bytes);
+	rv = siw_generic_rx(mem_p, srx, &frx->pbl_idx,
+			    sge->laddr + wqe->processed, bytes);
 	if (rv != bytes) {
 		wqe->wc_status = SIW_WC_GENERAL_ERR;
 		rv = -EINVAL;
-- 
2.35.3

[PATCH V2 09/20] RDMA/siw: Introduce SIW_STAG_MAX_INDEX

[Guoqing Jiang]

Add the macro to remove magic number in the code.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_mem.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index 92c5776a9eed..ac4502fb0a96 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -13,18 +13,20 @@
 #include "siw.h"
 #include "siw_mem.h"
 
+/* Stag lookup is based on its index part only (24 bits). */
+#define SIW_STAG_MAX_INDEX	0x00ffffff
+
 /*
- * Stag lookup is based on its index part only (24 bits).
  * The code avoids special Stag of zero and tries to randomize
  * STag values between 1 and SIW_STAG_MAX_INDEX.
  */
 int siw_mem_add(struct siw_device *sdev, struct siw_mem *m)
 {
-	struct xa_limit limit = XA_LIMIT(1, 0x00ffffff);
+	struct xa_limit limit = XA_LIMIT(1, SIW_STAG_MAX_INDEX);
 	u32 id, next;
 
 	get_random_bytes(&next, 4);
-	next &= 0x00ffffff;
+	next &= SIW_STAG_MAX_INDEX;
 
 	if (xa_alloc_cyclic(&sdev->mem_xa, &id, m, limit, &next,
 	    GFP_KERNEL) < 0)
@@ -91,7 +93,7 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
 {
 	struct siw_device *sdev = to_siw_dev(pd->device);
 	struct siw_mem *mem = kzalloc(sizeof(*mem), GFP_KERNEL);
-	struct xa_limit limit = XA_LIMIT(1, 0x00ffffff);
+	struct xa_limit limit = XA_LIMIT(1, SIW_STAG_MAX_INDEX);
 	u32 id, next;
 
 	if (!mem)
@@ -107,7 +109,7 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
 	kref_init(&mem->ref);
 
 	get_random_bytes(&next, 4);
-	next &= 0x00ffffff;
+	next &= SIW_STAG_MAX_INDEX;
 
 	if (xa_alloc_cyclic(&sdev->mem_xa, &id, mem, limit, &next,
 	    GFP_KERNEL) < 0) {
-- 
2.35.3

[PATCH V2 10/20] RDMA/siw: Add one parameter to siw_destroy_cpulist

[Guoqing Jiang]

With that we can reuse it in siw_init_cpulist.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_main.c | 30 +++++++++++++---------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
index 1ab62982df74..61ad8ca3d1a2 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -109,6 +109,17 @@ static struct {
 	int num_nodes;
 } siw_cpu_info;
 
+static void siw_destroy_cpulist(int number)
+{
+	int i = 0;
+
+	while (i < number)
+		kfree(siw_cpu_info.tx_valid_cpus[i++]);
+
+	kfree(siw_cpu_info.tx_valid_cpus);
+	siw_cpu_info.tx_valid_cpus = NULL;
+}
+
 static int siw_init_cpulist(void)
 {
 	int i, num_nodes = nr_node_ids;
@@ -138,24 +149,11 @@ static int siw_init_cpulist(void)
 
 out_err:
 	siw_cpu_info.num_nodes = 0;
-	while (--i >= 0)
-		kfree(siw_cpu_info.tx_valid_cpus[i]);
-	kfree(siw_cpu_info.tx_valid_cpus);
-	siw_cpu_info.tx_valid_cpus = NULL;
+	siw_destroy_cpulist(i);
 
 	return -ENOMEM;
 }
 
-static void siw_destroy_cpulist(void)
-{
-	int i = 0;
-
-	while (i < siw_cpu_info.num_nodes)
-		kfree(siw_cpu_info.tx_valid_cpus[i++]);
-
-	kfree(siw_cpu_info.tx_valid_cpus);
-}
-
 /*
  * Choose CPU with least number of active QP's from NUMA node of
  * TX interface.
@@ -558,7 +556,7 @@ static __init int siw_init_module(void)
 	pr_info("SoftIWARP attach failed. Error: %d\n", rv);
 
 	siw_cm_exit();
-	siw_destroy_cpulist();
+	siw_destroy_cpulist(siw_cpu_info.num_nodes);
 
 	return rv;
 }
@@ -573,7 +571,7 @@ static void __exit siw_exit_module(void)
 
 	siw_cm_exit();
 
-	siw_destroy_cpulist();
+	siw_destroy_cpulist(siw_cpu_info.num_nodes);
 
 	if (siw_crypto_shash)
 		crypto_free_shash(siw_crypto_shash);
-- 
2.35.3

[PATCH V2 11/20] RDMA/siw: Introduce siw_cep_set_free_and_put

[Guoqing Jiang]

Add the helper which can be used in some places.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_cm.c | 31 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index c8a9118677d7..2f338bb3a24c 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -444,6 +444,12 @@ void siw_cep_put(struct siw_cep *cep)
 	kref_put(&cep->ref, __siw_cep_dealloc);
 }
 
+static void siw_cep_set_free_and_put(struct siw_cep *cep)
+{
+	siw_cep_set_free(cep);
+	siw_cep_put(cep);
+}
+
 void siw_cep_get(struct siw_cep *cep)
 {
 	kref_get(&cep->ref);
@@ -1506,9 +1512,7 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 
 		cep->state = SIW_EPSTATE_CLOSED;
 
-		siw_cep_set_free(cep);
-
-		siw_cep_put(cep);
+		siw_cep_set_free_and_put(cep);
 
 	} else if (s) {
 		sock_release(s);
@@ -1556,16 +1560,14 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	if (cep->state != SIW_EPSTATE_RECVD_MPAREQ) {
 		siw_dbg_cep(cep, "out of state\n");
 
-		siw_cep_set_free(cep);
-		siw_cep_put(cep);
+		siw_cep_set_free_and_put(cep);
 
 		return -ECONNRESET;
 	}
 	qp = siw_qp_id2obj(sdev, params->qpn);
 	if (!qp) {
 		WARN(1, "[QP %d] does not exist\n", params->qpn);
-		siw_cep_set_free(cep);
-		siw_cep_put(cep);
+		siw_cep_set_free_and_put(cep);
 
 		return -EINVAL;
 	}
@@ -1711,8 +1713,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	cep->qp = NULL;
 	siw_qp_put(qp);
 
-	siw_cep_set_free(cep);
-	siw_cep_put(cep);
+	siw_cep_set_free_and_put(cep);
 
 	return rv;
 }
@@ -1735,8 +1736,7 @@ int siw_reject(struct iw_cm_id *id, const void *pdata, u8 pd_len)
 	if (cep->state != SIW_EPSTATE_RECVD_MPAREQ) {
 		siw_dbg_cep(cep, "out of state\n");
 
-		siw_cep_set_free(cep);
-		siw_cep_put(cep); /* put last reference */
+		siw_cep_set_free_and_put(cep); /* put last reference */
 
 		return -ECONNRESET;
 	}
@@ -1753,8 +1753,7 @@ int siw_reject(struct iw_cm_id *id, const void *pdata, u8 pd_len)
 
 	cep->state = SIW_EPSTATE_CLOSED;
 
-	siw_cep_set_free(cep);
-	siw_cep_put(cep);
+	siw_cep_set_free_and_put(cep);
 
 	return 0;
 }
@@ -1889,8 +1888,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 		siw_socket_disassoc(s);
 		cep->state = SIW_EPSTATE_CLOSED;
 
-		siw_cep_set_free(cep);
-		siw_cep_put(cep);
+		siw_cep_set_free_and_put(cep);
 	}
 	sock_release(s);
 
@@ -1924,8 +1922,7 @@ static void siw_drop_listeners(struct iw_cm_id *id)
 			cep->sock = NULL;
 		}
 		cep->state = SIW_EPSTATE_CLOSED;
-		siw_cep_set_free(cep);
-		siw_cep_put(cep);
+		siw_cep_set_free_and_put(cep);
 	}
 }
 
-- 
2.35.3

[PATCH V2 12/20] RDMA/siw: Introduce siw_free_cm_id

[Guoqing Jiang]

Factor out a helper to simplify code.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202310091656.JlrmcNXB-lkp@intel.com/
Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_cm.c | 36 +++++++++++++-----------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 2f338bb3a24c..21303bad1281 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -364,6 +364,17 @@ static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type reason,
 	return id->event_handler(id, &event);
 }
 
+static void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
+{
+	if (!cep->cm_id)
+		return;
+
+	cep->cm_id->rem_ref(cep->cm_id);
+	cep->cm_id = NULL;
+	if (put_cep)
+		siw_cep_put(cep);
+}
+
 /*
  * siw_qp_cm_drop()
  *
@@ -415,9 +426,7 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 			default:
 				break;
 			}
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-			siw_cep_put(cep);
+			siw_free_cm_id(cep, true);
 		}
 		cep->state = SIW_EPSTATE_CLOSED;
 
@@ -1175,11 +1184,7 @@ static void siw_cm_work_handler(struct work_struct *w)
 			sock_release(cep->sock);
 			cep->sock = NULL;
 		}
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-			siw_cep_put(cep);
-		}
+		siw_free_cm_id(cep, true);
 	}
 	siw_cep_set_free(cep);
 	siw_put_work(work);
@@ -1702,10 +1707,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 
 	cep->state = SIW_EPSTATE_CLOSED;
 
-	if (cep->cm_id) {
-		cep->cm_id->rem_ref(id);
-		cep->cm_id = NULL;
-	}
+	siw_free_cm_id(cep, false);
 	if (qp->cep) {
 		siw_cep_put(cep);
 		qp->cep = NULL;
@@ -1880,10 +1882,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 	if (cep) {
 		siw_cep_set_inuse(cep);
 
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-		}
+		siw_free_cm_id(cep, false);
 		cep->sock = NULL;
 		siw_socket_disassoc(s);
 		cep->state = SIW_EPSTATE_CLOSED;
@@ -1912,10 +1911,7 @@ static void siw_drop_listeners(struct iw_cm_id *id)
 
 		siw_cep_set_inuse(cep);
 
-		if (cep->cm_id) {
-			cep->cm_id->rem_ref(cep->cm_id);
-			cep->cm_id = NULL;
-		}
+		siw_free_cm_id(cep, false);
 		if (cep->sock) {
 			siw_socket_disassoc(cep->sock);
 			sock_release(cep->sock);
-- 
2.35.3

[PATCH V2 13/20] RDMA/siw: Simplify siw_qp_id2obj

[Guoqing Jiang]

Let's set qp and return it.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index 44684b74550f..e127ef493296 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -601,12 +601,10 @@ static inline struct siw_qp *siw_qp_id2obj(struct siw_device *sdev, int id)
 
 	rcu_read_lock();
 	qp = xa_load(&sdev->qp_xa, id);
-	if (likely(qp && kref_get_unless_zero(&qp->ref))) {
-		rcu_read_unlock();
-		return qp;
-	}
+	if (likely(qp && !kref_get_unless_zero(&qp->ref)))
+		qp = NULL;
 	rcu_read_unlock();
-	return NULL;
+	return qp;
 }
 
 static inline u32 qp_id(struct siw_qp *qp)
-- 
2.35.3

[PATCH V2 14/20] RDMA/siw: Simplify siw_mem_id2obj

[Guoqing Jiang]

We can set mm then return it.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_mem.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index ac4502fb0a96..2d62f947d330 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -53,13 +53,11 @@ struct siw_mem *siw_mem_id2obj(struct siw_device *sdev, int stag_index)
 
 	rcu_read_lock();
 	mem = xa_load(&sdev->mem_xa, stag_index);
-	if (likely(mem && kref_get_unless_zero(&mem->ref))) {
-		rcu_read_unlock();
-		return mem;
-	}
+	if (likely(mem && !kref_get_unless_zero(&mem->ref)))
+		mem = NULL;
 	rcu_read_unlock();
 
-	return NULL;
+	return mem;
 }
 
 static void siw_free_plist(struct siw_page_chunk *chunk, int num_pages,
-- 
2.35.3

[PATCH V2 15/20] RDMA/siw: Cleanup siw_accept

[Guoqing Jiang]

With the initialization of rv and the two added label, we can
simplifiy code a bit.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_cm.c | 41 ++++++++++--------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 21303bad1281..4dbdcae46a78 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1548,7 +1548,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	struct siw_cep *cep = (struct siw_cep *)id->provider_data;
 	struct siw_qp *qp;
 	struct siw_qp_attrs qp_attrs;
-	int rv, max_priv_data = MPA_MAX_PRIVDATA;
+	int rv = -EINVAL, max_priv_data = MPA_MAX_PRIVDATA;
 	bool wait_for_peer_rts = false;
 
 	siw_cep_set_inuse(cep);
@@ -1564,24 +1564,17 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 
 	if (cep->state != SIW_EPSTATE_RECVD_MPAREQ) {
 		siw_dbg_cep(cep, "out of state\n");
-
-		siw_cep_set_free_and_put(cep);
-
-		return -ECONNRESET;
+		rv = -ECONNRESET;
+		goto free_cep;
 	}
 	qp = siw_qp_id2obj(sdev, params->qpn);
 	if (!qp) {
 		WARN(1, "[QP %d] does not exist\n", params->qpn);
-		siw_cep_set_free_and_put(cep);
-
-		return -EINVAL;
+		goto free_cep;
 	}
 	down_write(&qp->state_lock);
-	if (qp->attrs.state > SIW_QP_STATE_RTR) {
-		rv = -EINVAL;
-		up_write(&qp->state_lock);
-		goto error;
-	}
+	if (qp->attrs.state > SIW_QP_STATE_RTR)
+		goto error_unlock;
 	siw_dbg_cep(cep, "[QP %d]\n", params->qpn);
 
 	if (try_gso && cep->mpa.hdr.params.bits & MPA_RR_FLAG_GSO_EXP) {
@@ -1595,9 +1588,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 			"[QP %u]: ord %d (max %d), ird %d (max %d)\n",
 			qp_id(qp), params->ord, sdev->attrs.max_ord,
 			params->ird, sdev->attrs.max_ird);
-		rv = -EINVAL;
-		up_write(&qp->state_lock);
-		goto error;
+		goto error_unlock;
 	}
 	if (cep->enhanced_rdma_conn_est)
 		max_priv_data -= sizeof(struct mpa_v2_data);
@@ -1607,9 +1598,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 			cep,
 			"[QP %u]: private data length: %d (max %d)\n",
 			qp_id(qp), params->private_data_len, max_priv_data);
-		rv = -EINVAL;
-		up_write(&qp->state_lock);
-		goto error;
+		goto error_unlock;
 	}
 	if (cep->enhanced_rdma_conn_est) {
 		if (params->ord > cep->ord) {
@@ -1618,9 +1607,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 			} else {
 				cep->ird = params->ird;
 				cep->ord = params->ord;
-				rv = -EINVAL;
-				up_write(&qp->state_lock);
-				goto error;
+				goto error_unlock;
 			}
 		}
 		if (params->ird < cep->ird) {
@@ -1629,8 +1616,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 				params->ird = cep->ird;
 			else {
 				rv = -ENOMEM;
-				up_write(&qp->state_lock);
-				goto error;
+				goto error_unlock;
 			}
 		}
 		if (cep->mpa.v2_ctrl.ord &
@@ -1677,7 +1663,6 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 				   SIW_QP_ATTR_ORD | SIW_QP_ATTR_IRD |
 				   SIW_QP_ATTR_MPA);
 	up_write(&qp->state_lock);
-
 	if (rv)
 		goto error;
 
@@ -1700,6 +1685,9 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	siw_cep_set_free(cep);
 
 	return 0;
+
+error_unlock:
+	up_write(&qp->state_lock);
 error:
 	siw_socket_disassoc(cep->sock);
 	sock_release(cep->sock);
@@ -1714,9 +1702,8 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 	}
 	cep->qp = NULL;
 	siw_qp_put(qp);
-
+free_cep:
 	siw_cep_set_free_and_put(cep);
-
 	return rv;
 }
 
-- 
2.35.3

[PATCH V2 16/20] RDMA/siw: Remove siw_sk_assign_cm_upcalls

[Guoqing Jiang]

Let's move it into siw_sk_save_upcalls, then we only need to
get sk_callback_lock once. Also rename siw_sk_save_upcalls
to better fitting the new code.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_cm.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 4dbdcae46a78..be0d09d18a4f 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -39,17 +39,7 @@ static void siw_cm_llp_error_report(struct sock *s);
 static int siw_cm_upcall(struct siw_cep *cep, enum iw_cm_event_type reason,
 			 int status);
 
-static void siw_sk_assign_cm_upcalls(struct sock *sk)
-{
-	write_lock_bh(&sk->sk_callback_lock);
-	sk->sk_state_change = siw_cm_llp_state_change;
-	sk->sk_data_ready = siw_cm_llp_data_ready;
-	sk->sk_write_space = siw_cm_llp_write_space;
-	sk->sk_error_report = siw_cm_llp_error_report;
-	write_unlock_bh(&sk->sk_callback_lock);
-}
-
-static void siw_sk_save_upcalls(struct sock *sk)
+static void siw_sk_save_and_assign_upcalls(struct sock *sk)
 {
 	struct siw_cep *cep = sk_to_cep(sk);
 
@@ -58,6 +48,10 @@ static void siw_sk_save_upcalls(struct sock *sk)
 	cep->sk_data_ready = sk->sk_data_ready;
 	cep->sk_write_space = sk->sk_write_space;
 	cep->sk_error_report = sk->sk_error_report;
+	sk->sk_state_change = siw_cm_llp_state_change;
+	sk->sk_data_ready = siw_cm_llp_data_ready;
+	sk->sk_write_space = siw_cm_llp_write_space;
+	sk->sk_error_report = siw_cm_llp_error_report;
 	write_unlock_bh(&sk->sk_callback_lock);
 }
 
@@ -156,8 +150,7 @@ static void siw_cep_socket_assoc(struct siw_cep *cep, struct socket *s)
 	siw_cep_get(cep);
 	s->sk->sk_user_data = cep;
 
-	siw_sk_save_upcalls(s->sk);
-	siw_sk_assign_cm_upcalls(s->sk);
+	siw_sk_save_and_assign_upcalls(s->sk);
 }
 
 static struct siw_cep *siw_cep_alloc(struct siw_device *sdev)
-- 
2.35.3

[PATCH V2 17/20] RDMA/siw: Fix typo

[Guoqing Jiang]

Replace ORRQ with ORQ.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_qp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp.c b/drivers/infiniband/sw/siw/siw_qp.c
index 26e3904d2f41..da92cfa2073d 100644
--- a/drivers/infiniband/sw/siw/siw_qp.c
+++ b/drivers/infiniband/sw/siw/siw_qp.c
@@ -1183,7 +1183,7 @@ int siw_rqe_complete(struct siw_qp *qp, struct siw_rqe *rqe, u32 bytes,
 /*
  * siw_sq_flush()
  *
- * Flush SQ and ORRQ entries to CQ.
+ * Flush SQ and ORQ entries to CQ.
  *
  * Must be called with QP state write lock held.
  * Therefore, SQ and ORQ lock must not be taken.
-- 
2.35.3

[PATCH V2 18/20] RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp

[Guoqing Jiang]

We can just check max_send_wr here given both max_send_wr and
max_recv_wr are defined as u32 type, and we also need to ensure
num_sqe (derived from max_send_wr) shouldn't be zero.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index dcd69fc01176..ef149ed98946 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
 		goto err_atomic;
 	}
 	/*
-	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
-	 * but not for a QP unable to hold any WQE (SQ + RQ)
+	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
 	 */
-	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
-		siw_dbg(base_dev, "QP must have send or receive queue\n");
+	if (attrs->cap.max_send_wr == 0) {
+		siw_dbg(base_dev, "QP must have send queue\n");
 		rv = -EINVAL;
 		goto err_atomic;
 	}
@@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
 	if (rv)
 		goto err_atomic;
 
-	num_sqe = attrs->cap.max_send_wr;
-	num_rqe = attrs->cap.max_recv_wr;
 
 	/* All queue indices are derived from modulo operations
 	 * on a free running 'get' (consumer) and 'put' (producer)
 	 * unsigned counter. Having queue sizes at power of two
 	 * avoids handling counter wrap around.
 	 */
-	if (num_sqe)
-		num_sqe = roundup_pow_of_two(num_sqe);
-	else {
-		/* Zero sized SQ is not supported */
-		rv = -EINVAL;
-		goto err_out_xa;
-	}
+	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
+	num_rqe = attrs->cap.max_recv_wr;
 	if (num_rqe)
 		num_rqe = roundup_pow_of_two(num_rqe);
 
-- 
2.35.3

[PATCH V2 19/20] RDMA/siw: Introduce siw_destroy_cep_sock

[Guoqing Jiang]

Add one helper to simplify code a bit.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202310091735.oG7bTvLR-lkp@intel.com/`
Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_cm.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index be0d09d18a4f..4b3fde6ca9ca 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -368,6 +368,15 @@ static void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
 		siw_cep_put(cep);
 }
 
+static void siw_destroy_cep_sock(struct siw_cep *cep)
+{
+	if (cep->sock) {
+		siw_socket_disassoc(cep->sock);
+		sock_release(cep->sock);
+		cep->sock = NULL;
+	}
+}
+
 /*
  * siw_qp_cm_drop()
  *
@@ -423,14 +432,7 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
 		}
 		cep->state = SIW_EPSTATE_CLOSED;
 
-		if (cep->sock) {
-			siw_socket_disassoc(cep->sock);
-			/*
-			 * Immediately close socket
-			 */
-			sock_release(cep->sock);
-			cep->sock = NULL;
-		}
+		siw_destroy_cep_sock(cep);
 		if (cep->qp) {
 			cep->qp = NULL;
 			siw_qp_put(qp);
@@ -1682,9 +1684,7 @@ int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 error_unlock:
 	up_write(&qp->state_lock);
 error:
-	siw_socket_disassoc(cep->sock);
-	sock_release(cep->sock);
-	cep->sock = NULL;
+	siw_destroy_cep_sock(cep);
 
 	cep->state = SIW_EPSTATE_CLOSED;
 
@@ -1729,9 +1729,7 @@ int siw_reject(struct iw_cm_id *id, const void *pdata, u8 pd_len)
 		cep->mpa.hdr.params.bits |= MPA_RR_FLAG_REJECT; /* reject */
 		siw_send_mpareqrep(cep, pdata, pd_len);
 	}
-	siw_socket_disassoc(cep->sock);
-	sock_release(cep->sock);
-	cep->sock = NULL;
+	siw_destroy_cep_sock(cep);
 
 	cep->state = SIW_EPSTATE_CLOSED;
 
-- 
2.35.3

[PATCH V2 20/20] RDMA/siw: Update comments for siw_qp_sq_process

[Guoqing Jiang]

There is no siw_sq_work_handler in code, change it with siw_tx_thread
since siw_run_sq -> siw_sq_resume -> siw_qp_sq_process.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_qp_tx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index 2e055b6dcd42..553d85cb6dbb 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -1001,13 +1001,12 @@ static int siw_qp_sq_proc_local(struct siw_qp *qp, struct siw_wqe *wqe)
  * MPA FPDUs, each containing a DDP segment.
  *
  * SQ processing may occur in user context as a result of posting
- * new WQE's or from siw_sq_work_handler() context. Processing in
+ * new WQE's or from siw_tx_thread context. Processing in
  * user context is limited to non-kernel verbs users.
  *
  * SQ processing may get paused anytime, possibly in the middle of a WR
  * or FPDU, if insufficient send space is available. SQ processing
- * gets resumed from siw_sq_work_handler(), if send space becomes
- * available again.
+ * gets resumed from siw_tx_thread, if send space becomes available again.
  *
  * Must be called with the QP state read-locked.
  *
-- 
2.35.3

Re: [PATCH V2 00/20] Cleanup for siw

[Leon Romanovsky]

On Fri, Oct 13, 2023 at 10:00:33AM +0800, Guoqing Jiang wrote:
> V2 changes:
> 1. address W=1 warning in patch 12 and 19 per the report from lkp.
> 2. add one more patch (20th).
> 
> Hi,
> 
> This series aim to cleanup siw code, please review and comment!
> 
> Thanks,
> Guoqing
> 
> Guoqing Jiang (20):

Bernard, did you return from the vacation?

Thanks

RE: [PATCH V2 00/20] Cleanup for siw

[Bernard Metzler]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Tuesday, October 24, 2023 4:10 PM
> To: Guoqing Jiang <guoqing.jiang@linux.dev>; Bernard Metzler
> <BMT@zurich.ibm.com>
> Cc: jgg@ziepe.ca; linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH V2 00/20] Cleanup for siw
> 
> On Fri, Oct 13, 2023 at 10:00:33AM +0800, Guoqing Jiang wrote:
> > V2 changes:
> > 1. address W=1 warning in patch 12 and 19 per the report from lkp.
> > 2. add one more patch (20th).
> >
> > Hi,
> >
> > This series aim to cleanup siw code, please review and comment!
> >
> > Thanks,
> > Guoqing
> >
> > Guoqing Jiang (20):
> 
> Bernard, did you return from the vacation?
> 

Hi Leon, yes. I have it on my list. I hope to get to it tomorrow. 
Sorry for the delay!

Bernard.

Re: [PATCH V2 00/20] Cleanup for siw

[Leon Romanovsky]

On Tue, Oct 24, 2023 at 05:22:38PM +0000, Bernard Metzler wrote:
> 
> 
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Tuesday, October 24, 2023 4:10 PM
> > To: Guoqing Jiang <guoqing.jiang@linux.dev>; Bernard Metzler
> > <BMT@zurich.ibm.com>
> > Cc: jgg@ziepe.ca; linux-rdma@vger.kernel.org
> > Subject: [EXTERNAL] Re: [PATCH V2 00/20] Cleanup for siw
> > 
> > On Fri, Oct 13, 2023 at 10:00:33AM +0800, Guoqing Jiang wrote:
> > > V2 changes:
> > > 1. address W=1 warning in patch 12 and 19 per the report from lkp.
> > > 2. add one more patch (20th).
> > >
> > > Hi,
> > >
> > > This series aim to cleanup siw code, please review and comment!
> > >
> > > Thanks,
> > > Guoqing
> > >
> > > Guoqing Jiang (20):
> > 
> > Bernard, did you return from the vacation?
> > 
> 
> Hi Leon, yes. I have it on my list. I hope to get to it tomorrow.
> Sorry for the delay!

Thanks

> 
> Bernard.

RE: [PATCH V2 17/20] RDMA/siw: Fix typo

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 13, 2023 4:01 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V2 17/20] RDMA/siw: Fix typo
> 
> Replace ORRQ with ORQ.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/sw/siw/siw_qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_qp.c
> b/drivers/infiniband/sw/siw/siw_qp.c
> index 26e3904d2f41..da92cfa2073d 100644
> --- a/drivers/infiniband/sw/siw/siw_qp.c
> +++ b/drivers/infiniband/sw/siw/siw_qp.c
> @@ -1183,7 +1183,7 @@ int siw_rqe_complete(struct siw_qp *qp, struct
> siw_rqe *rqe, u32 bytes,
>  /*
>   * siw_sq_flush()
>   *
> - * Flush SQ and ORRQ entries to CQ.
> + * Flush SQ and ORQ entries to CQ.
>   *
>   * Must be called with QP state write lock held.
>   * Therefore, SQ and ORQ lock must not be taken.
> --
> 2.35.3

Acked-by: Bernard Metzler <bmt@zurich.ibm.com>

RE: [PATCH V2 18/20] RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 13, 2023 4:01 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >cap.max_send_wr in siw_create_qp
> 
> We can just check max_send_wr here given both max_send_wr and
> max_recv_wr are defined as u32 type, and we also need to ensure
> num_sqe (derived from max_send_wr) shouldn't be zero.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> b/drivers/infiniband/sw/siw/siw_verbs.c
> index dcd69fc01176..ef149ed98946 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> ib_qp_init_attr *attrs,
>  		goto err_atomic;
>  	}
>  	/*
> -	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
> -	 * but not for a QP unable to hold any WQE (SQ + RQ)
> +	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
>  	 */
> -	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
> -		siw_dbg(base_dev, "QP must have send or receive queue\n");
> +	if (attrs->cap.max_send_wr == 0) {
> +		siw_dbg(base_dev, "QP must have send queue\n");
>  		rv = -EINVAL;
>  		goto err_atomic;
>  	}
> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> ib_qp_init_attr *attrs,
>  	if (rv)
>  		goto err_atomic;
> 
> -	num_sqe = attrs->cap.max_send_wr;
> -	num_rqe = attrs->cap.max_recv_wr;
> 
>  	/* All queue indices are derived from modulo operations
>  	 * on a free running 'get' (consumer) and 'put' (producer)
>  	 * unsigned counter. Having queue sizes at power of two
>  	 * avoids handling counter wrap around.
>  	 */
> -	if (num_sqe)
> -		num_sqe = roundup_pow_of_two(num_sqe);
> -	else {
> -		/* Zero sized SQ is not supported */
> -		rv = -EINVAL;
> -		goto err_out_xa;
> -	}
> +	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
> +	num_rqe = attrs->cap.max_recv_wr;
>  	if (num_rqe)
>  		num_rqe = roundup_pow_of_two(num_rqe);
> 
> --
> 2.35.3

No the original code allows for a QP which cannot send but
just receive or vice versa. A QP which cannot send should be allowed.
Only a QP which cannot do anything should be refused to be created.
Keep it as is.

RE: [PATCH V2 19/20] RDMA/siw: Introduce siw_destroy_cep_sock

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 13, 2023 4:01 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V2 19/20] RDMA/siw: Introduce
> siw_destroy_cep_sock
> 
> Add one helper to simplify code a bit.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: INVALID URI REMOVED
> 3A__lore.kernel.org_oe-2Dkbuild-2Dall_202310091735.oG7bTvLR-2Dlkp-
> 40intel.com_&d=DwIDAg&c=jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-
> r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&m=glgYr0ZTX68mQQ5KqLKVgtMwCeBUa2g6CMcsI-
> atXWhya5f5qFEHlXxYa8pfeW7n&s=1mAM2DAKVpNKr6zJQofHUOMQ7rXtPvPUmZDLwpNySZ4&e=
> `
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> b/drivers/infiniband/sw/siw/siw_cm.c
> index be0d09d18a4f..4b3fde6ca9ca 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -368,6 +368,15 @@ static void siw_free_cm_id(struct siw_cep *cep, bool
> put_cep)
>  		siw_cep_put(cep);
>  }
> 
> +static void siw_destroy_cep_sock(struct siw_cep *cep)
> +{
> +	if (cep->sock) {
> +		siw_socket_disassoc(cep->sock);
> +		sock_release(cep->sock);
> +		cep->sock = NULL;
> +	}
> +}
> +
>  /*
>   * siw_qp_cm_drop()
>   *
> @@ -423,14 +432,7 @@ void siw_qp_cm_drop(struct siw_qp *qp, int schedule)
>  		}
>  		cep->state = SIW_EPSTATE_CLOSED;
> 
> -		if (cep->sock) {
> -			siw_socket_disassoc(cep->sock);
> -			/*
> -			 * Immediately close socket
> -			 */
> -			sock_release(cep->sock);
> -			cep->sock = NULL;
> -		}
> +		siw_destroy_cep_sock(cep);
>  		if (cep->qp) {
>  			cep->qp = NULL;
>  			siw_qp_put(qp);
> @@ -1682,9 +1684,7 @@ int siw_accept(struct iw_cm_id *id, struct
> iw_cm_conn_param *params)
>  error_unlock:
>  	up_write(&qp->state_lock);
>  error:
> -	siw_socket_disassoc(cep->sock);
> -	sock_release(cep->sock);
> -	cep->sock = NULL;
> +	siw_destroy_cep_sock(cep);
> 
>  	cep->state = SIW_EPSTATE_CLOSED;
> 
> @@ -1729,9 +1729,7 @@ int siw_reject(struct iw_cm_id *id, const void
> *pdata, u8 pd_len)
>  		cep->mpa.hdr.params.bits |= MPA_RR_FLAG_REJECT; /* reject */
>  		siw_send_mpareqrep(cep, pdata, pd_len);
>  	}
> -	siw_socket_disassoc(cep->sock);
> -	sock_release(cep->sock);
> -	cep->sock = NULL;
> +	siw_destroy_cep_sock(cep);
> 
>  	cep->state = SIW_EPSTATE_CLOSED;
> 
> --
> 2.35.3

OK

Acked-by: Bernard Metzler <bmt@zurich.ibm.com>

RE: [PATCH V2 20/20] RDMA/siw: Update comments for siw_qp_sq_process

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 13, 2023 4:01 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V2 20/20] RDMA/siw: Update comments for
> siw_qp_sq_process
> 
> There is no siw_sq_work_handler in code, change it with siw_tx_thread
> since siw_run_sq -> siw_sq_resume -> siw_qp_sq_process.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/sw/siw/siw_qp_tx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
> b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index 2e055b6dcd42..553d85cb6dbb 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_tx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -1001,13 +1001,12 @@ static int siw_qp_sq_proc_local(struct siw_qp *qp,
> struct siw_wqe *wqe)
>   * MPA FPDUs, each containing a DDP segment.
>   *
>   * SQ processing may occur in user context as a result of posting
> - * new WQE's or from siw_sq_work_handler() context. Processing in
> + * new WQE's or from siw_tx_thread context. Processing in
>   * user context is limited to non-kernel verbs users.
>   *
>   * SQ processing may get paused anytime, possibly in the middle of a WR
>   * or FPDU, if insufficient send space is available. SQ processing
> - * gets resumed from siw_sq_work_handler(), if send space becomes
> - * available again.
> + * gets resumed from siw_tx_thread, if send space becomes available again.
>   *
>   * Must be called with the QP state read-locked.
>   *
> --
> 2.35.3


Thanks, makes sense!

Acked-by: Bernard Metzler <bmt@zurich.ibm.com>

RE: [PATCH V2 00/20] Cleanup for siw

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 13, 2023 4:01 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V2 00/20] Cleanup for siw
> 
> V2 changes:
> 1. address W=1 warning in patch 12 and 19 per the report from lkp.
> 2. add one more patch (20th).
> 
> Hi,
> 
> This series aim to cleanup siw code, please review and comment!
> 
> Thanks,
> Guoqing
> 
> Guoqing Jiang (20):
>   RDMA/siw: Introduce siw_get_page
>   RDMA/siw: Introduce siw_srx_update_skb
>   RDMA/siw: Use iov.iov_len in kernel_sendmsg
>   RDMA/siw: Remove goto lable in siw_mmap
>   RDMA/siw: Remove rcu from siw_qp
>   RDMA/siw: No need to check term_info.valid before call
>     siw_send_terminate
>   RDMA/siw: Also goto out_sem_up if pin_user_pages returns 0
>   RDMA/siw: Factor out siw_generic_rx helper
>   RDMA/siw: Introduce SIW_STAG_MAX_INDEX
>   RDMA/siw: Add one parameter to siw_destroy_cpulist
>   RDMA/siw: Introduce siw_cep_set_free_and_put
>   RDMA/siw: Introduce siw_free_cm_id
>   RDMA/siw: Simplify siw_qp_id2obj
>   RDMA/siw: Simplify siw_mem_id2obj
>   RDMA/siw: Cleanup siw_accept
>   RDMA/siw: Remove siw_sk_assign_cm_upcalls
>   RDMA/siw: Fix typo
>   RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp
>   RDMA/siw: Introduce siw_destroy_cep_sock
>   RDMA/siw: Update comments for siw_qp_sq_process
> 
>  drivers/infiniband/sw/siw/siw.h       |   9 +-
>  drivers/infiniband/sw/siw/siw_cm.c    | 154 +++++++++++---------------
>  drivers/infiniband/sw/siw/siw_main.c  |  30 +++--
>  drivers/infiniband/sw/siw/siw_mem.c   |  22 ++--
>  drivers/infiniband/sw/siw/siw_qp.c    |   2 +-
>  drivers/infiniband/sw/siw/siw_qp_rx.c |  84 ++++++--------
>  drivers/infiniband/sw/siw/siw_qp_tx.c |  39 +++----
>  drivers/infiniband/sw/siw/siw_verbs.c |  23 +---
>  8 files changed, 144 insertions(+), 219 deletions(-)
> 
> 
> base-commit: 964168970cef5f5b738fae047e6de2107842feb7
> --
> 2.35.3

Hi Guoqing,
Thanks for the effort! I like most of it.

And, sorry, I saw I started my review with version 1 of your
patches. Luckily it does not have functional differences to
v2.

But I expect a version 3 anyway.

I currently do not have access to physical machines to check and
run the more complex patches. I hope to do that (patch 08,10,12,15)
tomorrow when back in office.

Thanks,
Bernard.

Re: [PATCH V2 18/20] RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp

[Guoqing Jiang]

On 10/25/23 21:27, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Friday, October 13, 2023 4:01 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
>>> cap.max_send_wr in siw_create_qp
>> We can just check max_send_wr here given both max_send_wr and
>> max_recv_wr are defined as u32 type, and we also need to ensure
>> num_sqe (derived from max_send_wr) shouldn't be zero.
>>
>> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
>> ---
>>   drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
>>   1 file changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>> b/drivers/infiniband/sw/siw/siw_verbs.c
>> index dcd69fc01176..ef149ed98946 100644
>> --- a/drivers/infiniband/sw/siw/siw_verbs.c
>> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
>> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
>> ib_qp_init_attr *attrs,
>>   		goto err_atomic;
>>   	}
>>   	/*
>> -	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
>> -	 * but not for a QP unable to hold any WQE (SQ + RQ)
>> +	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
>>   	 */
>> -	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
>> -		siw_dbg(base_dev, "QP must have send or receive queue\n");
>> +	if (attrs->cap.max_send_wr == 0) {
>> +		siw_dbg(base_dev, "QP must have send queue\n");
>>   		rv = -EINVAL;
>>   		goto err_atomic;
>>   	}
>> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
>> ib_qp_init_attr *attrs,
>>   	if (rv)
>>   		goto err_atomic;
>>
>> -	num_sqe = attrs->cap.max_send_wr;
>> -	num_rqe = attrs->cap.max_recv_wr;
>>
>>   	/* All queue indices are derived from modulo operations
>>   	 * on a free running 'get' (consumer) and 'put' (producer)
>>   	 * unsigned counter. Having queue sizes at power of two
>>   	 * avoids handling counter wrap around.
>>   	 */
>> -	if (num_sqe)
>> -		num_sqe = roundup_pow_of_two(num_sqe);
>> -	else {
>> -		/* Zero sized SQ is not supported */
>> -		rv = -EINVAL;
>> -		goto err_out_xa;
>> -	}
>> +	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
>> +	num_rqe = attrs->cap.max_recv_wr;
>>   	if (num_rqe)
>>   		num_rqe = roundup_pow_of_two(num_rqe);
>>
>> --
>> 2.35.3
> No the original code allows for a QP which cannot send but
> just receive or vice versa. A QP which cannot send should be allowed.
> Only a QP which cannot do anything should be refused to be created.
> Keep it as is.

Seems it is not consistent with the original code, because Zero sized SQ
(num_seq = 0) is not supported, also num_seq is got from max_send_wr,
which means a QP without sq is not permitted here.

But I probably misunderstood something.

Thanks,
Guoqing

Re: [PATCH V2 00/20] Cleanup for siw

[Guoqing Jiang]

Hi Bernard,

On 10/25/23 21:37, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Friday, October 13, 2023 4:01 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH V2 00/20] Cleanup for siw
>>
>> V2 changes:
>> 1. address W=1 warning in patch 12 and 19 per the report from lkp.
>> 2. add one more patch (20th).
>>
>> Hi,
>>
>> This series aim to cleanup siw code, please review and comment!
>>
>> Thanks,
>> Guoqing
>>
>> Guoqing Jiang (20):
>>    RDMA/siw: Introduce siw_get_page
>>    RDMA/siw: Introduce siw_srx_update_skb
>>    RDMA/siw: Use iov.iov_len in kernel_sendmsg
>>    RDMA/siw: Remove goto lable in siw_mmap
>>    RDMA/siw: Remove rcu from siw_qp
>>    RDMA/siw: No need to check term_info.valid before call
>>      siw_send_terminate
>>    RDMA/siw: Also goto out_sem_up if pin_user_pages returns 0
>>    RDMA/siw: Factor out siw_generic_rx helper
>>    RDMA/siw: Introduce SIW_STAG_MAX_INDEX
>>    RDMA/siw: Add one parameter to siw_destroy_cpulist
>>    RDMA/siw: Introduce siw_cep_set_free_and_put
>>    RDMA/siw: Introduce siw_free_cm_id
>>    RDMA/siw: Simplify siw_qp_id2obj
>>    RDMA/siw: Simplify siw_mem_id2obj
>>    RDMA/siw: Cleanup siw_accept
>>    RDMA/siw: Remove siw_sk_assign_cm_upcalls
>>    RDMA/siw: Fix typo
>>    RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp
>>    RDMA/siw: Introduce siw_destroy_cep_sock
>>    RDMA/siw: Update comments for siw_qp_sq_process
>>
>>   drivers/infiniband/sw/siw/siw.h       |   9 +-
>>   drivers/infiniband/sw/siw/siw_cm.c    | 154 +++++++++++---------------
>>   drivers/infiniband/sw/siw/siw_main.c  |  30 +++--
>>   drivers/infiniband/sw/siw/siw_mem.c   |  22 ++--
>>   drivers/infiniband/sw/siw/siw_qp.c    |   2 +-
>>   drivers/infiniband/sw/siw/siw_qp_rx.c |  84 ++++++--------
>>   drivers/infiniband/sw/siw/siw_qp_tx.c |  39 +++----
>>   drivers/infiniband/sw/siw/siw_verbs.c |  23 +---
>>   8 files changed, 144 insertions(+), 219 deletions(-)
>>
>>
>> base-commit: 964168970cef5f5b738fae047e6de2107842feb7
>> --
>> 2.35.3
> Hi Guoqing,
> Thanks for the effort! I like most of it.
>
> And, sorry, I saw I started my review with version 1 of your
> patches. Luckily it does not have functional differences to
> v2.

Thanks a lot for your review! I have replied  them separately.

> But I expect a version 3 anyway.

No problem, will do it.

> I currently do not have access to physical machines to check and
> run the more complex patches. I hope to do that (patch 08,10,12,15)
> tomorrow when back in office.

More check and running test with physical machines would be great :).
I have run some tests (blktests and xfstests) in VM against nvme host
which connected to nvme target through siw.

Thanks,
Guoqing

RE: [PATCH V2 18/20] RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Thursday, October 26, 2023 8:55 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >cap.max_send_wr in siw_create_qp
> 
> 
> 
> On 10/25/23 21:27, Bernard Metzler wrote:
> >> -----Original Message-----
> >> From: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> Sent: Friday, October 13, 2023 4:01 AM
> >> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
> >> Cc:linux-rdma@vger.kernel.org
> >> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >>> cap.max_send_wr in siw_create_qp
> >> We can just check max_send_wr here given both max_send_wr and
> >> max_recv_wr are defined as u32 type, and we also need to ensure
> >> num_sqe (derived from max_send_wr) shouldn't be zero.
> >>
> >> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> ---
> >>   drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
> >>   1 file changed, 5 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> >> b/drivers/infiniband/sw/siw/siw_verbs.c
> >> index dcd69fc01176..ef149ed98946 100644
> >> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> >> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> >> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> >> ib_qp_init_attr *attrs,
> >>   		goto err_atomic;
> >>   	}
> >>   	/*
> >> -	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
> >> -	 * but not for a QP unable to hold any WQE (SQ + RQ)
> >> +	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
> >>   	 */
> >> -	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
> >> -		siw_dbg(base_dev, "QP must have send or receive queue\n");
> >> +	if (attrs->cap.max_send_wr == 0) {
> >> +		siw_dbg(base_dev, "QP must have send queue\n");
> >>   		rv = -EINVAL;
> >>   		goto err_atomic;
> >>   	}
> >> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> >> ib_qp_init_attr *attrs,
> >>   	if (rv)
> >>   		goto err_atomic;
> >>
> >> -	num_sqe = attrs->cap.max_send_wr;
> >> -	num_rqe = attrs->cap.max_recv_wr;
> >>
> >>   	/* All queue indices are derived from modulo operations
> >>   	 * on a free running 'get' (consumer) and 'put' (producer)
> >>   	 * unsigned counter. Having queue sizes at power of two
> >>   	 * avoids handling counter wrap around.
> >>   	 */
> >> -	if (num_sqe)
> >> -		num_sqe = roundup_pow_of_two(num_sqe);
> >> -	else {
> >> -		/* Zero sized SQ is not supported */
> >> -		rv = -EINVAL;
> >> -		goto err_out_xa;
> >> -	}
> >> +	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
> >> +	num_rqe = attrs->cap.max_recv_wr;
> >>   	if (num_rqe)
> >>   		num_rqe = roundup_pow_of_two(num_rqe);
> >>
> >> --
> >> 2.35.3
> > No the original code allows for a QP which cannot send but
> > just receive or vice versa. A QP which cannot send should be allowed.
> > Only a QP which cannot do anything should be refused to be created.
> > Keep it as is.
> 
> Seems it is not consistent with the original code, because Zero sized SQ
> (num_seq = 0) is not supported, also num_seq is got from max_send_wr,
> which means a QP without sq is not permitted here.
> 

You are right. The RQ can be zero especially if a shared receive queue
is provided. So your patch looks good.


> But I probably misunderstood something.
> 
> Thanks,
> Guoqing