Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

Sagi Grimberg
2014-10-22T11:08:54Z

On 10/20/2014 8:36 AM, Minh Duc Tran wrote:
<SNIP>
> ================
> From: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
>
> This patch allows the underlying hardware to choose
> values other than hard coded max values for cqe and send_wr
> while preventing them from exceeding max supported values.

Hi Minh,

In order to take it, I would like to address a couple of bits
below. (sorry for the late response. I'm juggling across projects...)

>
> Signed-off-by: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/infiniband/ulp/iser/iser_verbs.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
> index 67225bb..73955c1 100644
> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
> @@ -76,7 +76,7 @@ static void iser_event_handler(struct ib_event_handler *handler,
> static int iser_create_device_ib_res(struct iser_device *device)
> {
> struct ib_device_attr *dev_attr = &device->dev_attr;
> - int ret, i;
> + int ret, i, max_cqe;
>
> ret = ib_query_device(device->ib_device, dev_attr);
> if (ret) {
> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
> if (IS_ERR(device->pd))
> goto pd_err;
>
> + max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
> + dev_attr->max_cqe : ISER_MAX_CQ_LEN;
> +

Why not do:
min_t(int, dev_attr->max_cqe, ISER_MAX_CQ_LEN)?

Please move it up before the CQs print, and add this information to the
print.

> for (i = 0; i < device->comps_used; i++) {
> struct iser_comp *comp = &device->comps[i];
>
> @@ -122,7 +125,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
> iser_cq_callback,
> iser_cq_event_callback,
> (void *)comp,
> - ISER_MAX_CQ_LEN, i);
> + max_cqe, i);
> if (IS_ERR(comp->cq)) {
> comp->cq = NULL;
> goto cq_err;
> @@ -426,6 +429,7 @@ void iser_free_fastreg_pool(struct ib_conn *ib_conn)
> static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
> {
> struct iser_device *device;
> + struct ib_device_attr *dev_attr;
> struct ib_qp_init_attr init_attr;
> int ret = -ENOMEM;
> int index, min_index = 0;
> @@ -433,6 +437,7 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
> BUG_ON(ib_conn->device == NULL);
>
> device = ib_conn->device;
> + dev_attr = &device->dev_attr;
>
> memset(&init_attr, 0, sizeof init_attr);
>
> @@ -461,7 +466,9 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
> init_attr.cap.max_send_wr = ISER_QP_SIG_MAX_REQ_DTOS + 1;
> init_attr.create_flags |= IB_QP_CREATE_SIGNATURE_EN;
> } else {
> - init_attr.cap.max_send_wr = ISER_QP_MAX_REQ_DTOS + 1;
> + init_attr.cap.max_send_wr =
> + (dev_attr->max_qp_wr < ISER_QP_MAX_REQ_DTOS) ?
> + dev_attr->max_qp_wr : ISER_QP_MAX_REQ_DTOS;

Again, why not do min_t?

Sagi.