On 3/17/2020 10:36 PM, Chuck Lever wrote:
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.
How come it breaks ?
If the ULP want to let the rdma-core layer to allocate the optimal
vector and rely on it to do so, why is it wrong to know the final vector
assigned ?
I can remove it and change the SRP target to use ib_alloc_cq but it
doesn't break the 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.
how come ? This facility is per PD that is created by the ULP.
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.
For the testing result I did I used NVMf target that uses ib_alloc_cq so
it would be good anyway.
According to your theory ib_alloc_cq_any will not perform well and have
degenerate cases anyway regardless the SRQ feature that was intended to
save resources and stay with great performance.
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
