On Wed, May 02, 2018 at 09:15:09AM -0500, Rohit Zambre wrote:
> 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.
We have a ABI between libibverbs.so and the linking application that
must be preserved as well, we don't break it.
> > 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.
Yes, allowing a null protection domain in a parent domain is something
we could do.
> > 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.
ibv_create_cq_ex()
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
