Re: [PATCH for-next 4/7] RDMA/hns: Optimize qp buffer allocation flow

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jan 20, 2020 at 04:19:34PM +0800, Weihang Li wrote:
> From: Xi Wang <wangxi11@huawei.com>
>
> Encapsulate qp buffer allocation related code into 3 functions:
> alloc_qp_buf(), map_qp_buf() and free_qp_buf().
>
> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_device.h |   1 -
>  drivers/infiniband/hw/hns/hns_roce_qp.c     | 268 +++++++++++++++-------------
>  2 files changed, 147 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> index 1f361e6..9ddeb2b 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -660,7 +660,6 @@ struct hns_roce_qp {
>  	/* this define must less than HNS_ROCE_MAX_BT_REGION */
>  #define HNS_ROCE_WQE_REGION_MAX	 3
>  	struct hns_roce_buf_region regions[HNS_ROCE_WQE_REGION_MAX];
> -	int			region_cnt;
>  	int                     wqe_bt_pg_shift;
>
>  	u32			buff_size;
> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
> index 3bd5809..5184cb4 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_qp.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
> @@ -716,23 +716,150 @@ static void free_rq_inline_buf(struct hns_roce_qp *hr_qp)
>  	kfree(hr_qp->rq_inl_buf.wqe_list);
>  }
>
> +static int map_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
> +		      u32 page_shift, bool is_user)
> +{
> +	dma_addr_t *buf_list[ARRAY_SIZE(hr_qp->regions)] = { NULL };
> +	struct ib_device *ibdev = &hr_dev->ib_dev;
> +	struct hns_roce_buf_region *r;
> +	int region_count;
> +	int buf_count;
> +	int ret;
> +	int i;
> +
> +	region_count = split_wqe_buf_region(hr_dev, hr_qp, hr_qp->regions,
> +					ARRAY_SIZE(hr_qp->regions), page_shift);
> +
> +	/* alloc a tmp list for storing wqe buf address */
> +	ret = hns_roce_alloc_buf_list(hr_qp->regions, buf_list, region_count);
> +	if (ret) {
> +		ibdev_err(ibdev, "alloc buf_list error for create qp\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < region_count; i++) {
> +		r = &hr_qp->regions[i];
> +		if (is_user)
> +			buf_count = hns_roce_get_umem_bufs(hr_dev, buf_list[i],
> +					r->count, r->offset, hr_qp->umem,
> +					page_shift);
> +		else
> +			buf_count = hns_roce_get_kmem_bufs(hr_dev, buf_list[i],
> +					r->count, r->offset, &hr_qp->hr_buf);
> +
> +		if (buf_count != r->count) {
> +			ibdev_err(ibdev, "get %s qp buf err,expect %d,ret %d.\n",
> +				  is_user ? "user" : "kernel",
> +				  r->count, buf_count);
> +			ret = -ENOBUFS;
> +			goto done;
> +		}
> +	}
> +
> +	hr_qp->wqe_bt_pg_shift = calc_wqe_bt_page_shift(hr_dev, hr_qp->regions,
> +							region_count);
> +	hns_roce_mtr_init(&hr_qp->mtr, PAGE_SHIFT + hr_qp->wqe_bt_pg_shift,
> +			  page_shift);
> +	ret = hns_roce_mtr_attach(hr_dev, &hr_qp->mtr, buf_list, hr_qp->regions,
> +				  region_count);
> +	if (ret)
> +		ibdev_err(ibdev, "mtr attatch error for create qp\n");
> +
> +	goto done;
> +
> +	hns_roce_mtr_cleanup(hr_dev, &hr_qp->mtr);
> +done:
> +	hns_roce_free_buf_list(buf_list, region_count);
> +
> +	return ret;
> +}
> +
> +static int alloc_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
> +			struct ib_qp_init_attr *init_attr,
> +			struct ib_udata *udata, unsigned long addr)
> +{
> +	u32 page_shift = PAGE_SHIFT + hr_dev->caps.mtt_buf_pg_sz;
> +	struct ib_device *ibdev = &hr_dev->ib_dev;
> +	bool is_rq_buf_inline;
> +	int ret;
> +
> +	is_rq_buf_inline = (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RQ_INLINE) &&
> +			   hns_roce_qp_has_rq(init_attr);
> +	if (is_rq_buf_inline) {
> +		ret = alloc_rq_inline_buf(hr_qp, init_attr);
> +		if (ret) {
> +			ibdev_err(ibdev, "alloc recv inline buffer error\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (udata) {
> +		hr_qp->umem = ib_umem_get(udata, addr, hr_qp->buff_size, 0);
> +		if (IS_ERR(hr_qp->umem)) {
> +			ibdev_err(ibdev, "get umem error for qp buf\n");
> +			ret = PTR_ERR(hr_qp->umem);
> +			goto err_inline;
> +		}
> +	} else {
> +		ret = hns_roce_buf_alloc(hr_dev, hr_qp->buff_size,
> +					 (1 << page_shift) * 2,
> +					 &hr_qp->hr_buf, page_shift);
> +		if (ret) {
> +			ibdev_err(ibdev, "alloc roce buf error\n");
> +			goto err_inline;
> +		}
> +	}
> +
> +	ret = map_qp_buf(hr_dev, hr_qp, page_shift, udata);


I don't remember what was the resolution if it is ok to rely on "udata"
as an indicator of user/kernel flow.

> +	if (ret) {
> +		ibdev_err(ibdev, "map roce buf error\n");

You put ibdev_err() on almost every line in map_qp_buf(), please leave
only one place.

Thanks