Re: [PATCH V4 rdma-core 3/5] bnxt_re/lib: add a function to initialize software queue

From: Haakon Bugge <haakon.bugge@oracle.com>
To: Devesh Sharma <devesh.sharma@broadcom.com>
Cc: OFED mailing list <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH V4 rdma-core 3/5] bnxt_re/lib: add a function to initialize software queue
Date: Thu, 10 Jun 2021 12:50:06 +0000	[thread overview]
Message-ID: <05C8B5D1-B086-47CB-BE2F-891FA7BB9EBC@oracle.com> (raw)
In-Reply-To: <20210610104910.1147756-4-devesh.sharma@broadcom.com>

> On 10 Jun 2021, at 12:49, Devesh Sharma <devesh.sharma@broadcom.com> wrote:
> 
> Splitting the shadow software queue initialization into
> a separate function. Same is being called for both RQ and
> SQ during create QP.
> 
> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com>
> ---
> providers/bnxt_re/main.h  |  3 ++
> providers/bnxt_re/verbs.c | 65 ++++++++++++++++++++++++---------------
> 2 files changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/providers/bnxt_re/main.h b/providers/bnxt_re/main.h
> index dc8166f2..94d42958 100644
> --- a/providers/bnxt_re/main.h
> +++ b/providers/bnxt_re/main.h
> @@ -96,7 +96,10 @@ struct bnxt_re_wrid {
> 	uint64_t wrid;
> 	uint32_t bytes;
> 	int next_idx;
> +	uint32_t st_slot_idx;
> +	uint8_t slots;
> 	uint8_t sig;
> +

Unintentional blank line?

> };
> 
> struct bnxt_re_qpcap {
> diff --git a/providers/bnxt_re/verbs.c b/providers/bnxt_re/verbs.c
> index 11c01574..e0e6e045 100644
> --- a/providers/bnxt_re/verbs.c
> +++ b/providers/bnxt_re/verbs.c
> @@ -847,9 +847,27 @@ static void bnxt_re_free_queues(struct bnxt_re_qp *qp)
> 	bnxt_re_free_aligned(qp->jsqq->hwque);
> }
> 
> +static int bnxt_re_alloc_init_swque(struct bnxt_re_joint_queue *jqq, int nwr)
> +{
> +	int indx;
> +
> +	jqq->swque = calloc(nwr, sizeof(struct bnxt_re_wrid));
> +	if (!jqq->swque)
> +		return -ENOMEM;
> +	jqq->start_idx = 0;
> +	jqq->last_idx = nwr - 1;
> +	for (indx = 0; indx < nwr; indx++)
> +		jqq->swque[indx].next_idx = indx + 1;
> +	jqq->swque[jqq->last_idx].next_idx = 0;
> +	jqq->last_idx = 0;
> +
> +	return 0;
> +}
> +
> static int bnxt_re_alloc_queues(struct bnxt_re_qp *qp,
> 				struct ibv_qp_init_attr *attr,
> -				uint32_t pg_size) {
> +				uint32_t pg_size)
> +{

Not related to commit

> 	struct bnxt_re_psns_ext *psns_ext;
> 	struct bnxt_re_wrid *swque;
> 	struct bnxt_re_queue *que;
> @@ -857,22 +875,23 @@ static int bnxt_re_alloc_queues(struct bnxt_re_qp *qp,
> 	uint32_t psn_depth;
> 	uint32_t psn_size;
> 	int ret, indx;
> +	uint32_t nswr;
> 
> 	que = qp->jsqq->hwque;
> 	que->stride = bnxt_re_get_sqe_sz();
> 	/* 8916 adjustment */
> -	que->depth = roundup_pow_of_two(attr->cap.max_send_wr + 1 +
> -					BNXT_RE_FULL_FLAG_DELTA);
> -	que->diff = que->depth - attr->cap.max_send_wr;
> +	nswr  = roundup_pow_of_two(attr->cap.max_send_wr + 1 +
> +				   BNXT_RE_FULL_FLAG_DELTA);
> +	que->diff = nswr - attr->cap.max_send_wr;
> 
> 	/* psn_depth extra entries of size que->stride */
> 	psn_size = bnxt_re_is_chip_gen_p5(qp->cctx) ?
> 					sizeof(struct bnxt_re_psns_ext) :
> 					sizeof(struct bnxt_re_psns);
> -	psn_depth = (que->depth * psn_size) / que->stride;
> -	if ((que->depth * psn_size) % que->stride)
> +	psn_depth = (nswr * psn_size) / que->stride;
> +	if ((nswr * psn_size) % que->stride)
> 		psn_depth++;
> -	que->depth += psn_depth;
> +	que->depth = nswr + psn_depth;
> 	/* PSN-search memory is allocated without checking for
> 	 * QP-Type. Kenrel driver do not map this memory if it
> 	 * is UD-qp. UD-qp use this memory to maintain WC-opcode.
> @@ -884,44 +903,42 @@ static int bnxt_re_alloc_queues(struct bnxt_re_qp *qp,
> 	/* exclude psns depth*/
> 	que->depth -= psn_depth;
> 	/* start of spsn space sizeof(struct bnxt_re_psns) each. */
> -	psns = (que->va + que->stride * que->depth);
> +	psns = (que->va + que->stride * nswr);
> 	psns_ext = (struct bnxt_re_psns_ext *)psns;
> -	swque = calloc(que->depth, sizeof(struct bnxt_re_wrid));
> -	if (!swque) {
> +
> +	ret = bnxt_re_alloc_init_swque(qp->jsqq, nswr);
> +	if (ret) {
> 		ret = -ENOMEM;
> 		goto fail;
> 	}
> 
> -	for (indx = 0 ; indx < que->depth; indx++, psns++)
> +	swque = qp->jsqq->swque;
> +	for (indx = 0 ; indx < nswr; indx++, psns++)

no space in "0 ;"

> 		swque[indx].psns = psns;
> 	if (bnxt_re_is_chip_gen_p5(qp->cctx)) {
> -		for (indx = 0 ; indx < que->depth; indx++, psns_ext++) {
> +		for (indx = 0 ; indx < nswr; indx++, psns_ext++) {

ditto

> 			swque[indx].psns_ext = psns_ext;
> 			swque[indx].psns = (struct bnxt_re_psns *)psns_ext;
> 		}
> 	}
> -	qp->jsqq->swque = swque;
> -
> -	qp->cap.max_swr = que->depth;
> +	qp->cap.max_swr = nswr;
> 	pthread_spin_init(&que->qlock, PTHREAD_PROCESS_PRIVATE);
> 
> 	if (qp->jrqq) {
> 		que = qp->jrqq->hwque;
> 		que->stride = bnxt_re_get_rqe_sz();
> -		que->depth = roundup_pow_of_two(attr->cap.max_recv_wr + 1);
> -		que->diff = que->depth - attr->cap.max_recv_wr;
> +		nswr = roundup_pow_of_two(attr->cap.max_recv_wr + 1);
> +		que->depth = nswr;
> +		que->diff = nswr - attr->cap.max_recv_wr;
> 		ret = bnxt_re_alloc_aligned(que, pg_size);
> 		if (ret)
> 			goto fail;
> -		pthread_spin_init(&que->qlock, PTHREAD_PROCESS_PRIVATE);
> 		/* For RQ only bnxt_re_wri.wrid is used. */
> -		qp->jrqq->swque = calloc(que->depth,
> -					 sizeof(struct bnxt_re_wrid));
> -		if (!qp->jrqq->swque) {
> -			ret = -ENOMEM;
> +		ret = bnxt_re_alloc_init_swque(qp->jrqq, nswr);
> +		if (ret)
> 			goto fail;

Here you have not "ret = -ENOMEM;". You have that above, unnecessary.

Thxs, Håkon

> -		}
> -		qp->cap.max_rwr = que->depth;
> +		pthread_spin_init(&que->qlock, PTHREAD_PROCESS_PRIVATE);
> +		qp->cap.max_rwr = nswr;
> 	}
> 
> 	return 0;
> -- 
> 2.25.1
> 