On Tue, May 19, 2020 at 03:43:34PM +0300, Yamin Friedman wrote:
> +void ib_cq_pool_init(struct ib_device *dev)
> +{
> + int i;
I generally rather see unsigned types used for unsigned values
> +
> + spin_lock_init(&dev->cq_pools_lock);
> + for (i = 0; i < ARRAY_SIZE(dev->cq_pools); i++)
> + INIT_LIST_HEAD(&dev->cq_pools[i]);
> +}
> +
> +void ib_cq_pool_destroy(struct ib_device *dev)
> +{
> + struct ib_cq *cq, *n;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(dev->cq_pools); i++) {
> + list_for_each_entry_safe(cq, n, &dev->cq_pools[i],
> + pool_entry) {
> + cq->shared = false;
> + ib_free_cq_user(cq, NULL);
WARN_ON cqe_used == 0?
> + }
> + }
> +
> +}
> +
> +static int ib_alloc_cqs(struct ib_device *dev, int nr_cqes,
unsigned types especially in function signatures please
> +struct ib_cq *ib_cq_pool_get(struct ib_device *dev, unsigned int nr_cqe,
> + int comp_vector_hint,
> + enum ib_poll_context poll_ctx)
> +{
> + static unsigned int default_comp_vector;
> + int vector, ret, num_comp_vectors;
> + struct ib_cq *cq, *found = NULL;
> + unsigned long flags;
> +
> + if (poll_ctx > ARRAY_SIZE(dev->cq_pools) || poll_ctx == IB_POLL_DIRECT)
> + return ERR_PTR(-EINVAL);
> +
> + num_comp_vectors = min_t(int, dev->num_comp_vectors,
> + num_online_cpus());
> + /* Project the affinty to the device completion vector range */
> + if (comp_vector_hint < 0)
> + vector = default_comp_vector++ % num_comp_vectors;
> + else
> + vector = comp_vector_hint % num_comp_vectors;
Modulo with signed types..
> + /*
> + * Find the least used CQ with correct affinity and
> + * enough free CQ entries
> + */
> + while (!found) {
> + spin_lock_irqsave(&dev->cq_pools_lock, flags);
> + list_for_each_entry(cq, &dev->cq_pools[poll_ctx - 1],
> + pool_entry) {
> + /*
> + * Check to see if we have found a CQ with the
> + * correct completion vector
> + */
> + if (vector != cq->comp_vector)
> + continue;
> + if (cq->cqe_used + nr_cqe > cq->cqe)
> + continue;
> + found = cq;
> + break;
> + }
> +
> + if (found) {
> + found->cqe_used += nr_cqe;
> + spin_unlock_irqrestore(&dev->cq_pools_lock, flags);
> +
> + return found;
> + }
> + spin_unlock_irqrestore(&dev->cq_pools_lock, flags);
> +
> + /*
> + * Didn't find a match or ran out of CQs in the device
> + * pool, allocate a new array of CQs.
> + */
> + ret = ib_alloc_cqs(dev, nr_cqe, poll_ctx);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> +
> + return found;
> +}
> +EXPORT_SYMBOL(ib_cq_pool_get);
> +
> +void ib_cq_pool_put(struct ib_cq *cq, unsigned int nr_cqe)
> +{
> + unsigned long flags;
> +
> + if (WARN_ON_ONCE(nr_cqe > cq->cqe_used))
> + return;
> +
> + spin_lock_irqsave(&cq->device->cq_pools_lock, flags);
> + cq->cqe_used -= nr_cqe;
> + spin_unlock_irqrestore(&cq->device->cq_pools_lock, flags);
It doesn't look to me like this spinlock can be used from anywhere but
a user context, why is it an irqsave?
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index d9f565a..0966f86 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -1418,6 +1418,7 @@ int ib_register_device(struct ib_device *device, const char *name)
> device->ops.dealloc_driver = dealloc_fn;
> return ret;
> }
> + ib_cq_pool_init(device);
> ib_device_put(device);
This look like wrong placement, it should be done before enable_device
as enable_device triggers ULPs t start using the device and they might
start allocating using this API.
> return 0;
> @@ -1446,6 +1447,7 @@ static void __ib_unregister_device(struct ib_device *ib_dev)
> if (!refcount_read(&ib_dev->refcount))
> goto out;
>
> + ib_cq_pool_destroy(ib_dev);
> disable_device(ib_dev);
similar issue, should be after disable_device as ULPs are still
running here
> /* Expedite removing unregistered pointers from the hash table */
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 1659131..d40604a 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -1555,6 +1555,7 @@ enum ib_poll_context {
> IB_POLL_SOFTIRQ, /* poll from softirq context */
> IB_POLL_WORKQUEUE, /* poll from workqueue */
> IB_POLL_UNBOUND_WORKQUEUE, /* poll from unbound workqueue */
> + IB_POLL_LAST,
> };
>
> struct ib_cq {
> @@ -1564,9 +1565,12 @@ struct ib_cq {
> void (*event_handler)(struct ib_event *, void *);
> void *cq_context;
> int cqe;
> + int cqe_used;
unsigned
> atomic_t usecnt; /* count number of work queues */
> enum ib_poll_context poll_ctx;
> + int comp_vector;
and put new members in sane places, don't make holes, etc
> const struct uapi_definition *driver_def;
> @@ -3952,6 +3960,33 @@ static inline int ib_req_notify_cq(struct ib_cq *cq,
> return cq->device->ops.req_notify_cq(cq, flags);
> }
>
> +/*
> + * ib_cq_pool_get() - Find the least used completion queue that matches
> + * a given cpu hint (or least used for wild card affinity)
> + * and fits nr_cqe
> + * @dev: rdma device
> + * @nr_cqe: number of needed cqe entries
> + * @comp_vector_hint: completion vector hint (-1) for the driver to assign
> + * a comp vector based on internal counter
> + * @poll_ctx: cq polling context
> + *
> + * Finds a cq that satisfies @comp_vector_hint and @nr_cqe requirements and
> + * claim entries in it for us. In case there is no available cq, allocate
> + * a new cq with the requirements and add it to the device pool.
> + * IB_POLL_DIRECT cannot be used for shared cqs so it is not a valid value
> + * for @poll_ctx.
> + */
> +struct ib_cq *ib_cq_pool_get(struct ib_device *dev, unsigned int nr_cqe,
> + int comp_vector_hint,
> + enum ib_poll_context poll_ctx);
kdoc comments belong in the C files please, and this isn't even in
proper kdoc format.
Jason
