[PATCH for-next 0/3] RDMA/hns: Fixes for calculation of sge

[PATCH for-next 0/3] RDMA/hns: Fixes for calculation of sge

[Weihang Li]

There are two issues with calculation of extended sge, one is about page
alignment and another is about the 0-length sges. Then, the path #3
refactor the code.

Lang Cheng (1):
  RDMA/hns: Fix 0-length sge calculation error

Weihang Li (1):
  RDMA/hns: Refactor process of setting extended sge

Yangyang Li (1):
  RDMA/hns: Bugfix for calculation of extended sge

 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 24 +++++-------
 drivers/infiniband/hw/hns/hns_roce_qp.c    | 61 +++++++++++++++---------------
 2 files changed, 41 insertions(+), 44 deletions(-)

-- 
2.8.1

[PATCH for-next 1/3] RDMA/hns: Fix 0-length sge calculation error

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

One RC SQ WQE can store 2 sges but UD can't, so ignore 2 valid sges of
wr.sglist for RC which have been filled in WQE before setting extended sge.
Either of RC and UD can not contain 0-length sges, so these 0-length
sges should be skipped.

Fixes: 54d6638765b0 ("RDMA/hns: Optimize WQE buffer size calculating process")
Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 1bd81fb..81f1155 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -214,25 +214,20 @@ static int fill_ext_sge_inl_data(struct hns_roce_qp *qp,
 	return 0;
 }
 
-static void set_extend_sge(struct hns_roce_qp *qp, const struct ib_send_wr *wr,
-			   unsigned int *sge_ind, unsigned int valid_num_sge)
+static void set_extend_sge(struct hns_roce_qp *qp, struct ib_sge *sge,
+			   unsigned int *sge_ind, unsigned int cnt)
 {
 	struct hns_roce_v2_wqe_data_seg *dseg;
-	unsigned int cnt = valid_num_sge;
-	struct ib_sge *sge = wr->sg_list;
 	unsigned int idx = *sge_ind;
 
-	if (qp->ibqp.qp_type == IB_QPT_RC || qp->ibqp.qp_type == IB_QPT_UC) {
-		cnt -= HNS_ROCE_SGE_IN_WQE;
-		sge += HNS_ROCE_SGE_IN_WQE;
-	}
-
 	while (cnt > 0) {
 		dseg = hns_roce_get_extend_sge(qp, idx & (qp->sge.sge_cnt - 1));
-		set_data_seg_v2(dseg, sge);
-		idx++;
+		if (likely(sge->length)) {
+			set_data_seg_v2(dseg, sge);
+			idx++;
+			cnt--;
+		}
 		sge++;
-		cnt--;
 	}
 
 	*sge_ind = idx;
@@ -340,7 +335,8 @@ static int set_rwqe_data_seg(struct ib_qp *ibqp, const struct ib_send_wr *wr,
 			}
 		}
 
-		set_extend_sge(qp, wr, sge_ind, valid_num_sge);
+		set_extend_sge(qp, wr->sg_list + i, sge_ind,
+			       valid_num_sge - HNS_ROCE_SGE_IN_WQE);
 	}
 
 	roce_set_field(rc_sq_wqe->byte_16,
@@ -503,7 +499,7 @@ static inline int set_ud_wqe(struct hns_roce_qp *qp,
 	if (ret)
 		return ret;
 
-	set_extend_sge(qp, wr, &curr_idx, valid_num_sge);
+	set_extend_sge(qp, wr->sg_list, &curr_idx, valid_num_sge);
 
 	/*
 	 * The pipeline can sequentially post all valid WQEs into WQ buffer,
-- 
2.8.1

[PATCH for-next 2/3] RDMA/hns: Bugfix for calculation of extended sge

[Weihang Li]

From: Yangyang Li <liyangyang20@huawei.com>

Page alignment is required when setting the number of extended sge
according to the hardware's achivement. If the space of needed extended sge
is greater than one page, the roundup_pow_of_two() can ensure that. But if
the needed extended sge isn't 0 and can not be filled in a whole page, the
driver should align it specifically.

Fixes: 54d6638765b0 ("RDMA/hns: Optimize WQE buffer size calculating process")
Signed-off-by: Yangyang Li <liyangyang20@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_qp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index 5e505a3..7321e74 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -432,7 +432,12 @@ static int set_extend_sge_param(struct hns_roce_dev *hr_dev, u32 sq_wqe_cnt,
 	}
 
 	hr_qp->sge.sge_shift = HNS_ROCE_SGE_SHIFT;
-	hr_qp->sge.sge_cnt = cnt;
+
+	/* If the number of extended sge is not zero, they MUST use the
+	 * space of HNS_HW_PAGE_SIZE at least.
+	 */
+	hr_qp->sge.sge_cnt = cnt ?
+			max(cnt, (u32)HNS_HW_PAGE_SIZE / HNS_ROCE_SGE_SIZE) : 0;
 
 	return 0;
 }
-- 
2.8.1

[PATCH for-next 3/3] RDMA/hns: Refactor process of setting extended sge

[Weihang Li]

The variable 'cnt' is used to represent the max number of sge an SQ WQE can
use at first, then it means how many extended sge an SQ has. In addition,
this function has no need to return a value. So refactor and encapsulate
the parts of getting number of extended sge a WQE can use to make it easier
to understand.

Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_qp.c | 60 +++++++++++++++------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index 7321e74..311fcf4 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -404,42 +404,43 @@ static int set_rq_size(struct hns_roce_dev *hr_dev, struct ib_qp_cap *cap,
 	return 0;
 }
 
-static int set_extend_sge_param(struct hns_roce_dev *hr_dev, u32 sq_wqe_cnt,
-				struct hns_roce_qp *hr_qp,
-				struct ib_qp_cap *cap)
+static u32 get_wqe_ext_sge_cnt(struct hns_roce_qp *qp)
 {
-	u32 cnt;
+	/* GSI/UD QP only has extended sge */
+	if (qp->ibqp.qp_type == IB_QPT_GSI || qp->ibqp.qp_type == IB_QPT_UD)
+		return qp->sq.max_gs;
 
-	cnt = max(1U, cap->max_send_sge);
-	if (hr_dev->hw_rev == HNS_ROCE_HW_VER1) {
-		hr_qp->sq.max_gs = roundup_pow_of_two(cnt);
-		hr_qp->sge.sge_cnt = 0;
+	if (qp->sq.max_gs > HNS_ROCE_SGE_IN_WQE)
+		return qp->sq.max_gs - HNS_ROCE_SGE_IN_WQE;
 
-		return 0;
-	}
+	return 0;
+}
 
-	hr_qp->sq.max_gs = cnt;
+static void set_ext_sge_param(struct hns_roce_dev *hr_dev, u32 sq_wqe_cnt,
+			      struct hns_roce_qp *hr_qp, struct ib_qp_cap *cap)
+{
+	u32 total_sge_cnt;
+	u32 wqe_sge_cnt;
 
-	/* UD sqwqe's sge use extend sge */
-	if (hr_qp->ibqp.qp_type == IB_QPT_GSI ||
-	    hr_qp->ibqp.qp_type == IB_QPT_UD) {
-		cnt = roundup_pow_of_two(sq_wqe_cnt * hr_qp->sq.max_gs);
-	} else if (hr_qp->sq.max_gs > HNS_ROCE_SGE_IN_WQE) {
-		cnt = roundup_pow_of_two(sq_wqe_cnt *
-				     (hr_qp->sq.max_gs - HNS_ROCE_SGE_IN_WQE));
-	} else {
-		cnt = 0;
+	hr_qp->sge.sge_shift = HNS_ROCE_SGE_SHIFT;
+
+	if (hr_dev->hw_rev == HNS_ROCE_HW_VER1) {
+		hr_qp->sq.max_gs = HNS_ROCE_SGE_IN_WQE;
+		return;
 	}
 
-	hr_qp->sge.sge_shift = HNS_ROCE_SGE_SHIFT;
+	hr_qp->sq.max_gs = max(1U, cap->max_send_sge);
+
+	wqe_sge_cnt = get_wqe_ext_sge_cnt(hr_qp);
 
 	/* If the number of extended sge is not zero, they MUST use the
 	 * space of HNS_HW_PAGE_SIZE at least.
 	 */
-	hr_qp->sge.sge_cnt = cnt ?
-			max(cnt, (u32)HNS_HW_PAGE_SIZE / HNS_ROCE_SGE_SIZE) : 0;
-
-	return 0;
+	if (wqe_sge_cnt) {
+		total_sge_cnt = roundup_pow_of_two(sq_wqe_cnt * wqe_sge_cnt);
+		hr_qp->sge.sge_cnt = max(total_sge_cnt,
+				(u32)HNS_HW_PAGE_SIZE / HNS_ROCE_SGE_SIZE);
+	}
 }
 
 static int check_sq_size_with_integrity(struct hns_roce_dev *hr_dev,
@@ -484,9 +485,7 @@ static int set_user_sq_size(struct hns_roce_dev *hr_dev,
 		return ret;
 	}
 
-	ret = set_extend_sge_param(hr_dev, cnt, hr_qp, cap);
-	if (ret)
-		return ret;
+	set_ext_sge_param(hr_dev, cnt, hr_qp, cap);
 
 	hr_qp->sq.wqe_shift = ucmd->log_sq_stride;
 	hr_qp->sq.wqe_cnt = cnt;
@@ -551,7 +550,6 @@ static int set_kernel_sq_size(struct hns_roce_dev *hr_dev,
 {
 	struct ib_device *ibdev = &hr_dev->ib_dev;
 	u32 cnt;
-	int ret;
 
 	if (!cap->max_send_wr || cap->max_send_wr > hr_dev->caps.max_wqes ||
 	    cap->max_send_sge > hr_dev->caps.max_sq_sg) {
@@ -571,9 +569,7 @@ static int set_kernel_sq_size(struct hns_roce_dev *hr_dev,
 	hr_qp->sq.wqe_shift = ilog2(hr_dev->caps.max_sq_desc_sz);
 	hr_qp->sq.wqe_cnt = cnt;
 
-	ret = set_extend_sge_param(hr_dev, cnt, hr_qp, cap);
-	if (ret)
-		return ret;
+	set_ext_sge_param(hr_dev, cnt, hr_qp, cap);
 
 	/* sync the parameters of kernel QP to user's configuration */
 	cap->max_send_wr = cnt;
-- 
2.8.1

Re: [PATCH for-next 1/3] RDMA/hns: Fix 0-length sge calculation error

[Leon Romanovsky]

On Sat, Nov 28, 2020 at 06:22:37PM +0800, Weihang Li wrote:
> From: Lang Cheng <chenglang@huawei.com>
>
> One RC SQ WQE can store 2 sges but UD can't, so ignore 2 valid sges of
> wr.sglist for RC which have been filled in WQE before setting extended sge.
> Either of RC and UD can not contain 0-length sges, so these 0-length
> sges should be skipped.
>
> Fixes: 54d6638765b0 ("RDMA/hns: Optimize WQE buffer size calculating process")
> Signed-off-by: Lang Cheng <chenglang@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH for-next 2/3] RDMA/hns: Bugfix for calculation of extended sge

[Leon Romanovsky]

On Sat, Nov 28, 2020 at 06:22:38PM +0800, Weihang Li wrote:
> From: Yangyang Li <liyangyang20@huawei.com>
>
> Page alignment is required when setting the number of extended sge
> according to the hardware's achivement. If the space of needed extended sge
> is greater than one page, the roundup_pow_of_two() can ensure that. But if
> the needed extended sge isn't 0 and can not be filled in a whole page, the
> driver should align it specifically.
>
> Fixes: 54d6638765b0 ("RDMA/hns: Optimize WQE buffer size calculating process")
> Signed-off-by: Yangyang Li <liyangyang20@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_qp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH for-next 3/3] RDMA/hns: Refactor process of setting extended sge

[Leon Romanovsky]

On Sat, Nov 28, 2020 at 06:22:39PM +0800, Weihang Li wrote:
> The variable 'cnt' is used to represent the max number of sge an SQ WQE can
> use at first, then it means how many extended sge an SQ has. In addition,
> this function has no need to return a value. So refactor and encapsulate
> the parts of getting number of extended sge a WQE can use to make it easier
> to understand.
>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_qp.c | 60 +++++++++++++++------------------
>  1 file changed, 28 insertions(+), 32 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH for-next 0/3] RDMA/hns: Fixes for calculation of sge

[Jason Gunthorpe]

On Sat, Nov 28, 2020 at 06:22:36PM +0800, Weihang Li wrote:
> There are two issues with calculation of extended sge, one is about page
> alignment and another is about the 0-length sges. Then, the path #3
> refactor the code.
> 
> Lang Cheng (1):
>   RDMA/hns: Fix 0-length sge calculation error
> 
> Weihang Li (1):
>   RDMA/hns: Refactor process of setting extended sge
> 
> Yangyang Li (1):
>   RDMA/hns: Bugfix for calculation of extended sge

Applied to for-next

Thanks,
Jason