| Message ID | 1510522903-6838-2-git-send-email-yishaih@mellanox.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote: > +struct ibv_td { > + struct ibv_context *context; > +}; As much as possible, I would like to see any new objects be 'opaque' to the application, so this should just be struct ibv_td; And ibv_td should be defined in driver.h or something This avoids leaking internal details and means we don't have to commit to an ABI for the insides of these structs. It was a mistake that so many structs were entirely exposed in the original verbs :( Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/13/2017 9:58 PM, Jason Gunthorpe wrote: > On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote: > >> +struct ibv_td { >> + struct ibv_context *context; >> +}; > > As much as possible, I would like to see any new objects be 'opaque' > to the application, so this should just be > > struct ibv_td; > > And ibv_td should be defined in driver.h or something > > This avoids leaking internal details and means we don't have to commit > to an ABI for the insides of these structs. > ibv_td should expose in verbs.h the 'context' as the inline function ibv_alloc_td() needs it to get the verbs_context and call the driver function if was set. Further extensions if will come should be 'opaque' as you pointed. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 14, 2017 at 10:26:16AM +0200, Yishai Hadas wrote: > On 11/13/2017 9:58 PM, Jason Gunthorpe wrote: > >On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote: > > > >>+struct ibv_td { > >>+ struct ibv_context *context; > >>+}; > > > >As much as possible, I would like to see any new objects be 'opaque' > >to the application, so this should just be > > > >struct ibv_td; > > > >And ibv_td should be defined in driver.h or something > > > >This avoids leaking internal details and means we don't have to commit > >to an ABI for the insides of these structs. > > > > ibv_td should expose in verbs.h the 'context' as the inline function > ibv_alloc_td() needs it to get the verbs_context and call the driver > function if was set. You mean ibv_dealloc_td, OK.. Related to my other point, ibv_alloc_td should probably accept a ibv_pd as an argument instead. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/14/2017 9:17 PM, Jason Gunthorpe wrote: > On Tue, Nov 14, 2017 at 10:26:16AM +0200, Yishai Hadas wrote: >> On 11/13/2017 9:58 PM, Jason Gunthorpe wrote: >>> On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote: >>> >>>> +struct ibv_td { >>>> + struct ibv_context *context; >>>> +}; >>> >>> As much as possible, I would like to see any new objects be 'opaque' >>> to the application, so this should just be >>> >>> struct ibv_td; >>> >>> And ibv_td should be defined in driver.h or something >>> >>> This avoids leaking internal details and means we don't have to commit >>> to an ABI for the insides of these structs. >>> >> >> ibv_td should expose in verbs.h the 'context' as the inline function >> ibv_alloc_td() needs it to get the verbs_context and call the driver >> function if was set. > > You mean ibv_dealloc_td, OK.. > > Related to my other point, ibv_alloc_td should probably accept a > ibv_pd as an argument instead. > Why ibv_alloc_td() should get an ibv_pd ? do you have a typo here ? The expected API for ibv_alloc_td() as was previously discussed expects to be as follows: struct ibv_td { struct ibv_context *context; }; struct ibv_td_init_attr { uint32_t comp_mask; }; struct ibv_td *ibv_alloc_td(struct ibv_context *context, struct ibv_td_init_attr *attr); -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 15, 2017 at 11:34:01AM +0200, Yishai Hadas wrote: > >>ibv_td should expose in verbs.h the 'context' as the inline function > >>ibv_alloc_td() needs it to get the verbs_context and call the driver > >>function if was set. > > > >You mean ibv_dealloc_td, OK.. > > > >Related to my other point, ibv_alloc_td should probably accept a > >ibv_pd as an argument instead. > > > > Why ibv_alloc_td() should get an ibv_pd ? do you have a typo here ? No.. Since ibv_alloc_td returns a 'ibv_pd' with additional information, it makes no sense to have an API that can return an 'ibv_pd' that is not actually a PD. So, all TD's have to be created from a PD, and the concept of a TD without a PD should be entirely removed from the API. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 17, 2017 at 8:42 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Wed, Nov 15, 2017 at 11:34:01AM +0200, Yishai Hadas wrote: > >> >>ibv_td should expose in verbs.h the 'context' as the inline function >> >>ibv_alloc_td() needs it to get the verbs_context and call the driver >> >>function if was set. >> > >> >You mean ibv_dealloc_td, OK.. >> > >> >Related to my other point, ibv_alloc_td should probably accept a >> >ibv_pd as an argument instead. >> > >> >> Why ibv_alloc_td() should get an ibv_pd ? do you have a typo here ? > > No.. > > Since ibv_alloc_td returns a 'ibv_pd' with additional information, it > makes no sense to have an API that can return an 'ibv_pd' that is not > actually a PD. This new ibv_alloc_td() returns a ibv_td (Thread Domain), and it only manages the Thread related information The new ibv_alloc_parent_domain() returns a ibv_pd. https://www.spinics.net/lists/linux-rdma/msg56891.html The Parent Domain gets as input the protection domain (i.e. ibv_pd), thread domain (i.e. ibv_td) and potential other domains in the future (e.g. loopback). Here you can claim you want to enforce that a parent domain 'ibv_pd' will always include a protection domain 'ibv_pd', so that the struct ibv_pd values are always valid. Just need to update the man page for that. Maybe that's the source of the confusion? > So, all TD's have to be created from a PD, and the concept of a TD > without a PD should be entirely removed from the API. I can definitely see an application that wants to do something smart with it's CQ's, like per thread or NUMA aware logic. While it has many QPs' which it can't commit to any thread logic. Here we'll force the user to provide some protection domain while creating the CQ's with specific TD's. It doesn't have to be so. Alex -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 19, 2017 at 08:43:16AM +0200, Alex Rosenbaum wrote: > This new ibv_alloc_td() returns a ibv_td (Thread Domain), and it only > manages the Thread related information Yes, sorry, my bad. I didn't read this carefully, you are right, it is this the API I was thinking about: > The new ibv_alloc_parent_domain() returns a ibv_pd. > https://www.spinics.net/lists/linux-rdma/msg56891.html So I'd like to see: struct ibv_parent_domain_init_attr { - struct ibv_pd *pd; /* referance to a protection domain, or NULL */ + struct ibv_pd *pd; /* referance to a protection domain, can not be NULL */ > > So, all TD's have to be created from a PD, and the concept of a TD > > without a PD should be entirely removed from the API. > > I can definitely see an application that wants to do something smart > with it's CQ's, like per thread or NUMA aware logic. While it has many > QPs' which it can't commit to any thread logic. > Here we'll force the user to provide some protection domain while > creating the CQ's with specific TD's. It doesn't have to be so. When we revise the CQ api it can directly accept a TD object, so there is no need for it to use the 'parent domain' version of the PD. And even if we did use the PD for consistency, it doesn't hurt at all to pass in a PD&TD to the CQ object and then have the CQ ignore the PD portion. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 19, 2017 at 5:32 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Sun, Nov 19, 2017 at 08:43:16AM +0200, Alex Rosenbaum wrote: > > So I'd like to see: > > struct ibv_parent_domain_init_attr { > - struct ibv_pd *pd; /* referance to a protection domain, or NULL */ > + struct ibv_pd *pd; /* referance to a protection domain, can not be NULL */ > > And even if we did use the PD for consistency, it doesn't hurt at all > to pass in a PD&TD to the CQ object and then have the CQ ignore the > PD portion. I see no problem with this direction. We'll take it to next step now. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/libibverbs/man/ibv_alloc_td.3 b/libibverbs/man/ibv_alloc_td.3 new file mode 100755 index 0000000..8fc2eef --- /dev/null +++ b/libibverbs/man/ibv_alloc_td.3 @@ -0,0 +1,52 @@ +.\" -*- nroff -*- +.\" Licensed under the OpenIB.org BSD license (FreeBSD Variant) - See COPYING.md +.\" +.TH IBV_ALLOC_TD 3 2017-11-06 libibverbs "Libibverbs Programmer's Manual" +.SH "NAME" +ibv_alloc_td(), ibv_dealloc_td() \- allocate and deallocate thread domain object +.SH "SYNOPSIS" +.nf +.B #include <infiniband/verbs.h> +.sp +.BI "struct ibv_td *ibv_alloc_td(struct ibv_context " "*context"); +.sp +.BI "int ibv_dealloc_td(struct ibv_td " "*td"); +.fi +.SH "DESCRIPTION" +.B ibv_alloc_td() +allocates a thread domain object for the RDMA device context +.I context\fR. +.sp +The thread domain object defines how the verbs libraries will use locks and additional HW mapping to achieve best performance for handling multi-thread or single-thread protection. +An application defines the serialization thread accesses scope by choosing which thread domain object will be used when creating verbs resources. +.sp +All verbs resources created with a unique thread domain object should be accessed in a serialized sequence by no more than single thread at a time. +.sp +Using verbs resources without a thread domain object means application can't commit to any threading access serialization model and the verbs library has to provide full thread safe access. +.sp +A +.I struct ibv_td +can be added to a parent domain via +.B ibv_alloc_parent_domain()\fR. +.sp +.B ibv_dealloc_td() +will deallocate the thread domain +.I td\fR. +All resources created with the +.I td +should be destroyed prior to deallocating the +.I td\fR. +.SH "RETURN VALUE" +.B ibv_alloc_td() +returns a pointer to the allocated struct +.I ibv_td +object, or NULL if the request fails (and sets errno to indicates the failure reason). +.sp +.B ibv_dealloc_td() +returns 0 on success, or the value of errno on failure (which indicates the failure reason). +.SH "SEE ALSO" +.BR ibv_alloc_parent_domain (3), +.SH "AUTHORS" +.TP +Alex Rosenbaum <rosenbaumalex@gmail.com> + diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h index 6465aa9..c6c536f 100644 --- a/libibverbs/verbs.h +++ b/libibverbs/verbs.h @@ -549,6 +549,10 @@ struct ibv_pd { uint32_t handle; }; +struct ibv_td { + struct ibv_context *context; +}; + enum ibv_xrcd_init_attr_mask { IBV_XRCD_INIT_ATTR_FD = 1 << 0, IBV_XRCD_INIT_ATTR_OFLAGS = 1 << 1, @@ -1637,6 +1641,8 @@ enum verbs_context_mask { struct verbs_context { /* "grows up" - new fields go here */ + int (*dealloc_td)(struct ibv_td *td); + struct ibv_td *(*alloc_td)(struct ibv_context *context); int (*post_srq_ops)(struct ibv_srq *srq, struct ibv_ops_wr *op, struct ibv_ops_wr **bad_op); @@ -2177,6 +2183,35 @@ ibv_create_qp_ex(struct ibv_context *context, struct ibv_qp_init_attr_ex *qp_ini } /** + * ibv_alloc_td - Allocate a thread domain + */ +static inline struct ibv_td *ibv_alloc_td(struct ibv_context *context) +{ + struct verbs_context *vctx; + + vctx = verbs_get_ctx_op(context, alloc_td); + if (!vctx) { + errno = ENOSYS; + return NULL; + } + return vctx->alloc_td(context); +} + +/** + * ibv_dealloc_td - Free a thread domain + */ +static inline int ibv_dealloc_td(struct ibv_td *td) +{ + struct verbs_context *vctx; + + vctx = verbs_get_ctx_op(td->context, dealloc_td); + if (!vctx) + return ENOSYS; + + return vctx->dealloc_td(td); +} + +/** * ibv_query_rt_values_ex - Get current real time @values of a device. * @values - in/out - defines the attributes we need to query/queried. * (Or's bits of enum ibv_values_mask on values->comp_mask field)
The ibv_thread_domain is a user space object that can be used by an application to mark that few verbs resources will be used from single thread. As a result the driver can share some hardware resources for those resources and can drop few locks when it accesses them. This patch introduces this object and its related verbs, more details about the expected usage are described as part of the man page of this commit. Signed-off-by: Yishai Hadas <yishaih@mellanox.com> --- libibverbs/man/ibv_alloc_td.3 | 52 +++++++++++++++++++++++++++++++++++++++++++ libibverbs/verbs.h | 35 +++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100755 libibverbs/man/ibv_alloc_td.3