On Tue, May 1, 2018 at 3:46 PM, Jason Gunthorpe <jgg@xxxxxxxxxxxx> wrote:
> On Tue, May 01, 2018 at 08:13:07PM +0000, Rohit Zambre wrote:
>> Today, the provider ensures safe multi-thread access to any CQ. However,
>> if the user can guarantee that a CQ will be accessed from only one thread,
>> the lock on the CQ is still unnecessarily taken in the critical data path.
>> The current standard Verbs API for CQ-creation doesn't take any attributes
>> or objects to allow the user to convey single-threaded information to the
>> provider. Thus, the current standard API needs to be modified.
>>
>> This RFC patch modifies the current ibv_create_cq to take the recently
>> introduced Thread Domain standard verbs object. An object assigned to a
>> Thread Domain is guaranteed by the user to be accessed from only one
>> thread. So, if the user assigns a CQ to a Thread Domain, the provider
>> can use this information to disable locking that protects the CQ against
>> concurrent accesses.
>>
>> Other Verbs objects, such as the QP, are assigned to a Thread Domain via
>> the Parent Domain object, which is nothing but an extended Protection
>> Domain. This patch does not use the same methodology since it would limit
>> what is currently allowed in the design space of Verbs applications.
>> Today, QPs belonging to different Protection/Parent Domains can be bound
>> to the same CQ. The CQ is not associated to any PD. Extending
>> ibv_create_cq to take in a Parent Domain will enforce the isolating
>> purpose of the PD on the CQ and will not allow a QP from another PD to be
>> bound to it.
>>
>> I have included the diffs only for libibverbs and the mlx5 provider as an
>> example. The control of locking in mlx5 is based on top of the changes
>> introduced in [1].
>>
>> The summary of diffs is just to give an idea of what a complete
>> incorporation would look like. It includes extending the create_cq
>> functions of each provider to take in the Thread Domain, and adding a NULL
>> argument to the current ibv_create_cq calls.
>
> I agree this is something we should be doing, however..
>
>> diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
>> index eb57824..72d68ac 100644
>> +++ b/libibverbs/verbs.h
>> @@ -1632,7 +1632,7 @@ struct ibv_context_ops {
>> int (*dealloc_mw)(struct ibv_mw *mw);
>> struct ibv_cq * (*create_cq)(struct ibv_context *context, int cqe,
>> struct ibv_comp_channel *channel,
>> - int comp_vector);
>> + int comp_vector, struct ibv_td *td);
>
> This change is a nope, it breaks the ABI which we cannot do.
My understanding is that the ABI is relevant only for the write()s to
the uverbs character device. The Thread Domain here is only a
userspace entity and won't change the information passed on to the
kernel. Is there an ABI for some other interaction apart from the
user-kernel interaction? I'm missing something here.
> Also I think this should be a pd not a td as the PD concept is
> intended to carry more general aspects, this would not be a PD as a
> protection domain but a PD as a parent domain, so it would not
> restrict the generality of the CQ. But I should check that again to be sure..
A Parent Domain must contain a Protection Domain
(ibv_check_alloc_parent_domain). While I don't see why the user would,
the user *could* still pass a Parent Domain that does not have a
thread domain in ibv_create_qp, for example, and the Parent Domain
would be used as the QP's Protection Domain. So the Parent Domain will
need to satisfy the isolating semantics of the Protection Domain.
Unless, we remove the restriction of the the Parent Domain requiring a
valid Protection Domain to account for general aspects.. it would need
the userspace driver to check for a valid Protection Domain for verbs
that need a Protection Domain before writing to the character device
driver.
> So this will have to create some kind of new _ex function(s) for create
> cq I guess..
>
> Also, performance sensitive users should be using the new
> dis-aggregated CQ poller as it is faster..
Could you point me to this new CQ poller you are referring to here?
Not sure what you mean by dis-aggregated.
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
