> On Mar 17, 2020, at 11:41 AM, Max Gurtovoy <maxg@xxxxxxxxxxxx> wrote:
>
>
> On 3/17/2020 5:19 PM, Chuck Lever wrote:
>> Hi Max-
>>
>>> On Mar 17, 2020, at 9:40 AM, Max Gurtovoy <maxg@xxxxxxxxxxxx> wrote:
>>>
>>> In some cases, e.g. when using ib_alloc_cq_any, one would like to know
>>> the completion vector that eventually assigned to the CQ. Cache this
>>> value during CQ creation.
>> I'm confused by the mention of the ib_alloc_cq_any() API here.
>
> Can't the user use ib_alloc_cq_any() and still want to know what is the final comp vector for his needs ?
If your caller cares about the final cv value, then it should not use
the _any API. The point of _any is that the caller really does not care,
the cv value is hidden because it is not consequential. Your design
breaks that assumption/contract.
>> Is your design somehow dependent on the way the current ib_alloc_cq_any()
>> selects comp_vectors? The contract for _any() is that it is an API for
>> callers that simply do not care about what comp_vector is chosen. There's
>> no guarantee that the _any() comp_vector allocator will continue to use
>> round-robin in the future, for instance.
>
> it's fine. I just want to make sure that I'll spread the SRQ's equally.
The _any algorithm is simplistic, it spreads cvs for the system as a whole.
All devices, all callers. You're going to get some bad degenerate cases
as soon as you have multiple users of the SRQ facility.
So, you really want to have a more specialized comp_vector selector for
the SRQ facility; one that is careful to spread cvs per device, independent
of the global allocator, which is good enough for normal cases.
I think your tests perform well simply because there was no other contender
for comp_vectors on your test system.
>> If you want to guarantee that there is an SRQ for each comp_vector and a
>> comp_vector for each SRQ, stick with a CQ allocation API that enables
>> explicit selection of the comp_vector value, and cache that value in the
>> caller, not in the core data structures.
>
> I'm Ok with that as well. This is exactly what we do in the nvmf/rdma but I wanted to stick also with the SRP target approach.
>
> Bart,
>
> Any objection to remove the call for ib_alloc_cq_any() from ib_srpt and use ib_alloc_cq() ?
>
>
>>
>>
>>> Signed-off-by: Max Gurtovoy <maxg@xxxxxxxxxxxx>
>>> ---
>>> drivers/infiniband/core/cq.c | 1 +
>>> include/rdma/ib_verbs.h | 1 +
>>> 2 files changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
>>> index 4f25b24..a7cbf52 100644
>>> --- a/drivers/infiniband/core/cq.c
>>> +++ b/drivers/infiniband/core/cq.c
>>> @@ -217,6 +217,7 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
>>> cq->device = dev;
>>> cq->cq_context = private;
>>> cq->poll_ctx = poll_ctx;
>>> + cq->comp_vector = comp_vector;
>>> atomic_set(&cq->usecnt, 0);
>>>
>>> cq->wc = kmalloc_array(IB_POLL_BATCH, sizeof(*cq->wc), GFP_KERNEL);
>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>> index fc8207d..0d61772 100644
>>> --- a/include/rdma/ib_verbs.h
>>> +++ b/include/rdma/ib_verbs.h
>>> @@ -1558,6 +1558,7 @@ struct ib_cq {
>>> struct ib_device *device;
>>> struct ib_ucq_object *uobject;
>>> ib_comp_handler comp_handler;
>>> + u32 comp_vector;
>>> void (*event_handler)(struct ib_event *, void *);
>>> void *cq_context;
>>> int cqe;
>>> --
>>> 1.8.3.1
>>>
>> --
>> Chuck Lever
>> chucklever@xxxxxxxxx
>>
>>
>>
--
Chuck Lever
chucklever@xxxxxxxxx
