Re: [PATCH for-next 01/10] IB/core: Change provider's API of create_cq to be extendible

Jason Gunthorpe
2015-05-19T18:45:35Z

On Sun, May 17, 2015 at 04:36:17PM +0300, Or Gerlitz wrote:
> From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> Add a new ib_cq_init_attr structure which contains the
> previous cqe (minimum number of CQ entries) and comp_vector
> (completion vector) in addition to a new flags field.
> All vendors' create_cq callbacks are changed in order
> to work with the new API.
>
> This commit does not change any functionality.

This seems reasonable to me.

> @@ -1341,6 +1341,7 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
> struct ib_uverbs_event_file *ev_file = NULL;
> struct ib_cq *cq;
> int ret;
> + struct ib_cq_init_attr attr = {.cqe = 0};

This doesn't seem necessary, it is unconditionally set below:

> if (out_len < sizeof resp)
> return -ENOSPC;
> @@ -1376,8 +1377,9 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
> INIT_LIST_HEAD(&obj->comp_list);
> INIT_LIST_HEAD(&obj->async_list);
>
> - cq = file->device->ib_dev->create_cq(file->device->ib_dev, cmd.cqe,
> - cmd.comp_vector,
> + attr.cqe = cmd.cqe;
> + attr.comp_vector = cmd.comp_vector;
> + cq = file->device->ib_dev->create_cq(file->device->ib_dev, &attr,
> file->ucontext, &udata);

> +++ b/drivers/infiniband/core/verbs.c
> @@ -1013,8 +1013,9 @@ struct ib_cq *ib_create_cq(struct ib_device *device,
> void *cq_context, int cqe, int comp_vector)
> {
> struct ib_cq *cq;
> + struct ib_cq_init_attr attr = {.cqe = cqe, .comp_vector = comp_vector};
>
> - cq = device->create_cq(device, cqe, comp_vector, NULL, NULL);
> + cq = device->create_cq(device, &attr, NULL, NULL);

Hum, I guess it makes sense to stop flowing ib_cq_init_attr at this
point, for this patch, but it does seem a bit weird from an API design
perspective.

> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 8d59479..ad0e2ea 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -173,6 +173,12 @@ struct ib_odp_caps {
> } per_transport_caps;
> };
>
> +struct ib_cq_init_attr {
> + int cqe;

unsigned int cqe

Can't be negative..

> + struct ib_cq * (*create_cq)(struct ib_device *device,
> + struct ib_cq_init_attr *attr,

const struct ib_cq_init_attr *attr,

And related changes that will cause.

Jason