RE: [PATCH V4 02/18] RDMA/siw: Introduce siw_update_skb_rcvd

RE: [PATCH V4 02/18] RDMA/siw: Introduce siw_update_skb_rcvd

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 27, 2023 3:26 PM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V4 02/18] RDMA/siw: Introduce
> siw_update_skb_rcvd
> 
> 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..10805a7d0487 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_update_skb_rcvd(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_update_skb_rcvd(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_update_skb_rcvd(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_update_skb_rcvd(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_update_skb_rcvd(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_update_skb_rcvd(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

Looks good.

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

RE: [PATCH V4 12/18] RDMA/siw: Introduce siw_free_cm_id

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 27, 2023 3:27 PM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V4 12/18] RDMA/siw: Introduce siw_free_cm_id
> 
> Factor out a helper to simplify code.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: INVALID URI REMOVED
> 3A__lore.kernel.org_oe-2Dkbuild-2Dall_202310091656.JlrmcNXB-2Dlkp-
> 40intel.com_&d=DwIDAg&c=jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-
> r8ZO1PP1alNwU_QJcRRLfmYTAgd3QCvqSc&m=aD4gL7u0nROWIiFk0ch8zNX6vYWKMUOorYEWPY
> b9amSqnQTH0qNMcLijHRNn7r_s&s=kmGGMztuOUMejyxISpfW94VtbHKDivS9DMunEaOx5uc&e=
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
> Changes since last version:
> 1. add missed check of cep->cm_id
> 
>  drivers/infiniband/sw/siw/siw_cm.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> b/drivers/infiniband/sw/siw/siw_cm.c
> index 2f338bb3a24c..9c79d3ea5b66 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -364,6 +364,15 @@ 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)
> +{
> +	if (!cep->cm_id)
> +		return;
> +
> +	cep->cm_id->rem_ref(cep->cm_id);
> +	cep->cm_id = NULL;
> +}
> +
>  /*
>   * siw_qp_cm_drop()
>   *
> @@ -415,8 +424,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_free_cm_id(cep);
>  			siw_cep_put(cep);
>  		}
>  		cep->state = SIW_EPSTATE_CLOSED;
> @@ -1176,8 +1184,7 @@ static void siw_cm_work_handler(struct work_struct
> *w)
>  			cep->sock = NULL;
>  		}
>  		if (cep->cm_id) {
> -			cep->cm_id->rem_ref(cep->cm_id);
> -			cep->cm_id = NULL;
> +			siw_free_cm_id(cep);
>  			siw_cep_put(cep);
>  		}
>  	}
> @@ -1702,10 +1709,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);
>  	if (qp->cep) {
>  		siw_cep_put(cep);
>  		qp->cep = NULL;
> @@ -1880,10 +1884,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);
>  		cep->sock = NULL;
>  		siw_socket_disassoc(s);
>  		cep->state = SIW_EPSTATE_CLOSED;
> @@ -1912,10 +1913,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);
>  		if (cep->sock) {
>  			siw_socket_disassoc(cep->sock);
>  			sock_release(cep->sock);
> --
> 2.35.3

Looks good.

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

RE: [PATCH V4 08/18] RDMA/siw: Factor out siw_rx_data helper

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 27, 2023 3:27 PM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V4 08/18] RDMA/siw: Factor out siw_rx_data
> helper
> 
> Remove the redundant code given they share the same logic.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
> Change since last version:
> 1. rename the helper to siw_rx_data per Bernard's suggestion
> 
>  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 10805a7d0487..ed4fc39718b4 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_rx_data(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_rx_data(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_rx_data(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_rx_data(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
Looks good.

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

RE: [PATCH V4 01/18] RDMA/siw: Introduce siw_get_page

[Bernard Metzler]

> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 27, 2023 3:26 PM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V4 01/18] RDMA/siw: Introduce siw_get_page
> 
> 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
Looks good.

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

Re: [PATCH V4 00/18] Cleanup for siw

[Leon Romanovsky]

On Fri, Oct 27, 2023 at 09:26:26PM +0800, Guoqing Jiang wrote:
> V4 changes:
> 1. add Acked-by tags.
> 2. update patch 3 and patch 12 per Bernard's review.
> 3. update patch header in patch 18.
> 
> V3 changes:
> 1. add Acked-by tags.
> 2. drop 2 patches and address other comments.
> 
> Appreciate for Bernard's review!
> 
> 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!

This series wasn't sent properly and it doesn't exist in patchworks and lore.
Please resend it properly.

Thanks

Re: [PATCH V4 00/18] Cleanup for siw

[Guoqing Jiang]

On 11/13/23 16:38, Leon Romanovsky wrote:
> On Fri, Oct 27, 2023 at 09:26:26PM +0800, Guoqing Jiang wrote:
>> V4 changes:
>> 1. add Acked-by tags.
>> 2. update patch 3 and patch 12 per Bernard's review.
>> 3. update patch header in patch 18.
>>
>> V3 changes:
>> 1. add Acked-by tags.
>> 2. drop 2 patches and address other comments.
>>
>> Appreciate for Bernard's review!
>>
>> 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!
> This series wasn't sent properly and it doesn't exist in patchworks and lore.
> Please resend it properly.

Done.

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

Thanks,
Guoqing

Re: [PATCH V4 00/18] Cleanup for siw

[Leon Romanovsky]

On Mon, Nov 13, 2023 at 07:59:27PM +0800, Guoqing Jiang wrote:
> 
> 
> On 11/13/23 16:38, Leon Romanovsky wrote:
> > On Fri, Oct 27, 2023 at 09:26:26PM +0800, Guoqing Jiang wrote:
> > > V4 changes:
> > > 1. add Acked-by tags.
> > > 2. update patch 3 and patch 12 per Bernard's review.
> > > 3. update patch header in patch 18.
> > > 
> > > V3 changes:
> > > 1. add Acked-by tags.
> > > 2. drop 2 patches and address other comments.
> > > 
> > > Appreciate for Bernard's review!
> > > 
> > > 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!
> > This series wasn't sent properly and it doesn't exist in patchworks and lore.
> > Please resend it properly.
> 
> Done.
> 
> https://lore.kernel.org/linux-rdma/20231113115726.12762-1-guoqing.jiang@linux.dev/T/#t

Thanks

> 
> Thanks,
> Guoqing