[PATCH 00/19] Cleanup for siw

[PATCH 00/19] Cleanup for siw

[Guoqing Jiang]

Hi,

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

Thanks,
Guoqing

Guoqing Jiang (19):
  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

 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 |  34 +++---
 drivers/infiniband/sw/siw/siw_verbs.c |  23 +---
 8 files changed, 142 insertions(+), 216 deletions(-)

-- 
2.35.3

[PATCH 01/19] 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 02/19] 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

RE: [PATCH 02/19] RDMA/siw: Introduce siw_srx_update_skb

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 02/19] RDMA/siw: Introduce siw_srx_update_skb
> 
> 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;
> +}
> +

better call it siw_update_skb_rcvd()?
We are not updating the skb here, but our state
referencing an skb.

>  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

Re: [PATCH 02/19] RDMA/siw: Introduce siw_srx_update_skb

[Guoqing Jiang]

On 10/25/23 20:33, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Monday, October 9, 2023 9:18 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH 02/19] RDMA/siw: Introduce siw_srx_update_skb
>>
>> 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;
>> +}
>> +
> better call it siw_update_skb_rcvd()?
> We are not updating the skb here, but our state
> referencing an skb.

Sure, thanks for the naming!

Thanks,
Guoqing

[PATCH 03/19] 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

RE: [PATCH 03/19] RDMA/siw: Use iov.iov_len in kernel_sendmsg

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 03/19] RDMA/siw: Use iov.iov_len in
> kernel_sendmsg
> 
> 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

Thanks, makes sense!

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

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

[Guoqing Jiang]

Remove unnecessary 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

RE: [PATCH 04/19] RDMA/siw: Remove goto lable in siw_mmap

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 04/19] RDMA/siw: Remove goto lable in siw_mmap
> 
> Remove unnecessary label since the failure case only need to
> print warning message.

I think you suggest removing it since the code falls through to the
useless label anyway and not since it prints a warning.

> 
> 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

Thanks, makes sense!

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

Re: [PATCH 04/19] RDMA/siw: Remove goto lable in siw_mmap

[Guoqing Jiang]

On 10/25/23 20:38, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Monday, October 9, 2023 9:18 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH 04/19] RDMA/siw: Remove goto lable in siw_mmap
>>
>> Remove unnecessary label since the failure case only need to
>> print warning message.
> I think you suggest removing it since the code falls through to the
> useless label anyway and not since it prints a warning.

Yes, exactly, sorry for the confusion. Will rephrase it to

"Let's remove it since the failure case only falls through
to the useless label."

>> 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
> Thanks, makes sense!
>
> Acked-by: Bernard Metzler<bmt@zurich.ibm.com>

Thank you!

Guoqing

[PATCH 05/19] 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

RE: [PATCH 05/19] RDMA/siw: Remove rcu from siw_qp

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 05/19] RDMA/siw: Remove rcu from siw_qp
> 
> 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

Thanks, makes sense!

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

[PATCH 06/19] 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

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

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 06/19] RDMA/siw: No need to check
> term_info.valid before call siw_send_terminate
> 
> 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

Thanks, makes sense!

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

[PATCH 07/19] 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

RE: [PATCH 07/19] RDMA/siw: Also goto out_sem_up if pin_user_pages returns 0

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 07/19] RDMA/siw: Also goto out_sem_up if
> pin_user_pages returns 0
> 
> 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
Okay, looks right.

hw/qib/qib_user_pages.c and hw/usnic/usnic_uiom.c
should be checked in that respect as well. Adding
Dennis and Christian as maintainers of those.

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

[PATCH 08/19] 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 09/19] 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

RE: [PATCH 09/19] RDMA/siw: Introduce SIW_STAG_MAX_INDEX

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 09/19] RDMA/siw: Introduce SIW_STAG_MAX_INDEX
> 
> 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

Okay.

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

[PATCH 10/19] 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 11/19] 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

RE: [PATCH 11/19] RDMA/siw: Introduce siw_cep_set_free_and_put

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 11/19] RDMA/siw: Introduce
> siw_cep_set_free_and_put
> 
> 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

Okay.

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

[PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id

[Guoqing Jiang]

Factor out a helper to simplify code.

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..987084828786 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);
 }
 
+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

Re: [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id

[kernel test robot]

Hi Guoqing,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v6.6-rc5 next-20231009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Guoqing-Jiang/RDMA-siw-Introduce-siw_get_page/20231009-152705
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
patch link:    https://lore.kernel.org/r/20231009071801.10210-13-guoqing.jiang%40linux.dev
patch subject: [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231009/202310091656.JlrmcNXB-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310091656.JlrmcNXB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310091656.JlrmcNXB-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/infiniband/sw/siw/siw_cm.c:367:6: warning: no previous prototype for 'siw_free_cm_id' [-Wmissing-prototypes]
     367 | void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
         |      ^~~~~~~~~~~~~~


vim +/siw_free_cm_id +367 drivers/infiniband/sw/siw/siw_cm.c

   366	
 > 367	void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
   368	{
   369		if (!cep->cm_id)
   370			return;
   371	
   372		cep->cm_id->rem_ref(cep->cm_id);
   373		cep->cm_id = NULL;
   374		if (put_cep)
   375			siw_cep_put(cep);
   376	}
   377	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id

[kernel test robot]

Hi Guoqing,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v6.6-rc5 next-20231009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Guoqing-Jiang/RDMA-siw-Introduce-siw_get_page/20231009-152705
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
patch link:    https://lore.kernel.org/r/20231009071801.10210-13-guoqing.jiang%40linux.dev
patch subject: [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id
config: x86_64-randconfig-071-20231009 (https://download.01.org/0day-ci/archive/20231009/202310092226.zEUwIrFa-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310092226.zEUwIrFa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310092226.zEUwIrFa-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/infiniband/sw/siw/siw_cm.c:367:6: warning: no previous declaration for 'siw_free_cm_id' [-Wmissing-declarations]
    void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
         ^~~~~~~~~~~~~~


vim +/siw_free_cm_id +367 drivers/infiniband/sw/siw/siw_cm.c

   366	
 > 367	void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
   368	{
   369		if (!cep->cm_id)
   370			return;
   371	
   372		cep->cm_id->rem_ref(cep->cm_id);
   373		cep->cm_id = NULL;
   374		if (put_cep)
   375			siw_cep_put(cep);
   376	}
   377	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id
> 
> Factor out a helper to simplify code.
> 
> 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..987084828786 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);
>  }
> 
> +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;

I suggest not including cep_put() here, but to keep
cep reference counting explicit in the code.

> +	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

Re: [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id

[Guoqing Jiang]

On 10/25/23 21:02, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Monday, October 9, 2023 9:18 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH 12/19] RDMA/siw: Introduce siw_free_cm_id
>>
>> Factor out a helper to simplify code.
>>
>> 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..987084828786 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);
>>   }
>>
>> +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;
> I suggest not including cep_put() here, but to keep
> cep reference counting explicit in the code.
>
>> +	if (put_cep)
>> +		siw_cep_put(cep);
>> +}

Ok, will remove it in next version.

Thanks,
Guoqing

[PATCH 13/19] 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

RE: [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj
> 
> 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
No let's keep it as is. It openly codes the likely case
first.

Your code makes the unlikely thing likely.

Re: [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj

[Guoqing Jiang]

On 10/25/23 21:04, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Monday, October 9, 2023 9:18 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj
>>
>> 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
> No let's keep it as is. It openly codes the likely case
> first.
>
> Your code makes the unlikely thing likely.

How about change likely to unlikely? If not, I will drop both 13 and 14.

Thanks,
Guoqing

RE: [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Thursday, October 26, 2023 8:43 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj
> 
> 
> 
> On 10/25/23 21:04, Bernard Metzler wrote:
> >> -----Original Message-----
> >> From: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> Sent: Monday, October 9, 2023 9:18 AM
> >> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
> >> Cc:linux-rdma@vger.kernel.org
> >> Subject: [EXTERNAL] [PATCH 13/19] RDMA/siw: Simplify siw_qp_id2obj
> >>
> >> 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
> > No let's keep it as is. It openly codes the likely case
> > first.
> >
> > Your code makes the unlikely thing likely.
> 
> How about change likely to unlikely? If not, I will drop both 13 and 14.

just drop these. I don't see much benefit in changing it.

[PATCH 14/19] 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

RE: [PATCH 14/19] RDMA/siw: Simplify siw_mem_id2obj

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 14/19] RDMA/siw: Simplify siw_mem_id2obj
> 
> 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
No let's keep it as is. It openly codes the likely case first.

Your code makes the unlikely thing likely.

[PATCH 15/19] 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 987084828786..c3aa5533e75d 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 16/19] 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 align with 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 c3aa5533e75d..6866ec80473c 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

RE: [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls
> 
> 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 align with 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 c3aa5533e75d..6866ec80473c 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)

To simplify, I'd suggest doing it the other way around,
so having siw_sk_assign_cm_upcalls() including the
functionality of siw_sk_save_upcalls() first.

There is another function siw_sk_assign_rtr_upcalls(),
which re-assigns the upcalls for special handling of
an explicit RTR->RTS handshake if requested later during
connection setup.


> +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

Re: [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls

[Guoqing Jiang]

On 10/25/23 21:19, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Monday, October 9, 2023 9:18 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls
>>
>> 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 align with 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 c3aa5533e75d..6866ec80473c 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)
> To simplify, I'd suggest doing it the other way around,
> so having siw_sk_assign_cm_upcalls() including the
> functionality of siw_sk_save_upcalls() first.

I guess you mean below. If so, I will update it in v3.

  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)
  {
         struct siw_cep *cep = sk_to_cep(sk);

@@ -58,6 +48,11 @@ 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);
  }

> There is another function siw_sk_assign_rtr_upcalls(),
> which re-assigns the upcalls for special handling of
> an explicit RTR->RTS handshake if requested later during
> connection setup.

Ah, one is assign rtr upcall and another is assign cm upcall.
Thanks for the explanation.

Guoqing

RE: [PATCH 16/19] RDMA/siw: Remove siw_sk_assign_cm_upcalls

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Thursday, October 26, 2023 8:46 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH 16/19] RDMA/siw: Remove
> siw_sk_assign_cm_upcalls
> 
> 
> 
> On 10/25/23 21:19, Bernard Metzler wrote:
> >> -----Original Message-----
> >> From: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> Sent: Monday, October 9, 2023 9:18 AM
> >> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
> >> Cc:linux-rdma@vger.kernel.org
> >> Subject: [EXTERNAL] [PATCH 16/19] RDMA/siw: Remove
> siw_sk_assign_cm_upcalls
> >>
> >> 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 align with 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 c3aa5533e75d..6866ec80473c 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)
> > To simplify, I'd suggest doing it the other way around,
> > so having siw_sk_assign_cm_upcalls() including the
> > functionality of siw_sk_save_upcalls() first.
> 
> I guess you mean below. If so, I will update it in v3.

right. assigning the cm upcalls is the first step and
needs the original socket callbacks saved. Later, maybe
we have to reassign the upcalls to execute the additional
RTS handshake. 
> 
>   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)
>   {
>          struct siw_cep *cep = sk_to_cep(sk);
> 
> @@ -58,6 +48,11 @@ 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);
>   }
> 
> > There is another function siw_sk_assign_rtr_upcalls(),
> > which re-assigns the upcalls for special handling of
> > an explicit RTR->RTS handshake if requested later during
> > connection setup.
> 
> Ah, one is assign rtr upcall and another is assign cm upcall.
> Thanks for the explanation.
> 
> Guoqing

[PATCH 17/19] 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 18/19] 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 19/19] RDMA/siw: Introduce siw_destroy_cep_sock

[Guoqing Jiang]

Add one helper to simplify code a bit.

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 6866ec80473c..c39a18b0bff9 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -368,6 +368,15 @@ void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
 		siw_cep_put(cep);
 }
 
+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

Re: [PATCH 19/19] RDMA/siw: Introduce siw_destroy_cep_sock

[kernel test robot]

Hi Guoqing,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rdma/for-next]
[also build test WARNING on linus/master v6.6-rc5 next-20231009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Guoqing-Jiang/RDMA-siw-Introduce-siw_get_page/20231009-152705
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
patch link:    https://lore.kernel.org/r/20231009071801.10210-20-guoqing.jiang%40linux.dev
patch subject: [PATCH 19/19] RDMA/siw: Introduce siw_destroy_cep_sock
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231009/202310091735.oG7bTvLR-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310091735.oG7bTvLR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310091735.oG7bTvLR-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/infiniband/sw/siw/siw_cm.c:360:6: warning: no previous prototype for 'siw_free_cm_id' [-Wmissing-prototypes]
     360 | void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
         |      ^~~~~~~~~~~~~~
>> drivers/infiniband/sw/siw/siw_cm.c:371:6: warning: no previous prototype for 'siw_destroy_cep_sock' [-Wmissing-prototypes]
     371 | void siw_destroy_cep_sock(struct siw_cep *cep)
         |      ^~~~~~~~~~~~~~~~~~~~


vim +/siw_destroy_cep_sock +371 drivers/infiniband/sw/siw/siw_cm.c

   370	
 > 371	void siw_destroy_cep_sock(struct siw_cep *cep)
   372	{
   373		if (cep->sock) {
   374			siw_socket_disassoc(cep->sock);
   375			sock_release(cep->sock);
   376			cep->sock = NULL;
   377		}
   378	}
   379	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 19/19] RDMA/siw: Introduce siw_destroy_cep_sock

[Guoqing Jiang]

Hi,

On 10/9/23 17:54, kernel test robot wrote:
> Hi Guoqing,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on rdma/for-next]
> [also build test WARNING on linus/master v6.6-rc5 next-20231009]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Guoqing-Jiang/RDMA-siw-Introduce-siw_get_page/20231009-152705
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
> patch link:    https://lore.kernel.org/r/20231009071801.10210-20-guoqing.jiang%40linux.dev
> patch subject: [PATCH 19/19] RDMA/siw: Introduce siw_destroy_cep_sock
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231009/202310091735.oG7bTvLR-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310091735.oG7bTvLR-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310091735.oG7bTvLR-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>     drivers/infiniband/sw/siw/siw_cm.c:360:6: warning: no previous prototype for 'siw_free_cm_id' [-Wmissing-prototypes]
>       360 | void siw_free_cm_id(struct siw_cep *cep, bool put_cep)
>           |      ^~~~~~~~~~~~~~
>>> drivers/infiniband/sw/siw/siw_cm.c:371:6: warning: no previous prototype for 'siw_destroy_cep_sock' [-Wmissing-prototypes]
>       371 | void siw_destroy_cep_sock(struct siw_cep *cep)
>           |      ^~~~~~~~~~~~~~~~~~~~
>
>
> vim +/siw_destroy_cep_sock +371 drivers/infiniband/sw/siw/siw_cm.c

Thanks for the test, I suppose add "static" at the beginning of the two 
lines will resolve the warnings.

Thanks,
Guoqing

RE: [PATCH 00/19] Cleanup for siw

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Monday, October 9, 2023 9:18 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH 00/19] Cleanup for siw
> 
> Hi,
> 
> This series aim to cleanup siw code, please review and comment!
> 
> Thanks,
> Guoqing
> 
> Guoqing Jiang (19):
>   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
> 
>  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 |  34 +++---
>  drivers/infiniband/sw/siw/siw_verbs.c |  23 +---
>  8 files changed, 142 insertions(+), 216 deletions(-)
> 
> --
> 2.35.3
Hi Guoqing,

I'll have a look later next week. Currently on vacation.
Thanks, Bernard.

Re: [PATCH 00/19] Cleanup for siw

[Guoqing Jiang]

Hi  Bernard,

On 10/13/23 23:45, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang <guoqing.jiang@linux.dev>
>> Sent: Monday, October 9, 2023 9:18 AM
>> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
>> Cc: linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH 00/19] Cleanup for siw
>>
>> Hi,
>>
>> This series aim to cleanup siw code, please review and comment!
>>
>> Thanks,
>> Guoqing
>>
>> Guoqing Jiang (19):
>>    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
>>
>>   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 |  34 +++---
>>   drivers/infiniband/sw/siw/siw_verbs.c |  23 +---
>>   8 files changed, 142 insertions(+), 216 deletions(-)
>>
>> --
>> 2.35.3
> Hi Guoqing,
>
> I'll have a look later next week. Currently on vacation.
> Thanks, Bernard.

No hurry, enjoy your vacation first :)

BTW, v2 is here.

https://lore.kernel.org/linux-rdma/20231013020053.2120-1-guoqing.jiang@linux.dev/T/#t

Thanks,
Guoqing