* [PATCH libibverbs] Fix create/destroy flow API
@ 2015-09-04 21:17 Doug Ledford
[not found] ` <9665e46a71940f2721b30fdb4aaa0853161babc9.1441401379.git.dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Doug Ledford @ 2015-09-04 21:17 UTC (permalink / raw)
To: linux-rdma-u79uwXL29TY76Z2rM5mHXA; +Cc: Doug Ledford
When adding this API, there had been consensus that having separate
lib_* and drv_* function pointers in the extended context struct
was not needed and should not be done. However, that snuck in anyway.
This backs that out and takes us back to a single pointer for each
function, but does so in a way as to preserve both back and forward
compatibility.
Fixes: 389de6a6ef4e (Add receive flow steering support)
Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
include/infiniband/verbs.h | 25 +++++++++++--------------
src/device.c | 22 +++++++++++++---------
2 files changed, 24 insertions(+), 23 deletions(-)
---
This change will preserve binary ABI but will (intentionally) break
source API. Mellanox should release an updated libmlx4 once the
libibverbs release with this patch in it goes live.
diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h
index 28e1586b0c96..6100f5200b7a 100644
--- a/include/infiniband/verbs.h
+++ b/include/infiniband/verbs.h
@@ -977,14 +977,11 @@ enum verbs_context_mask {
struct verbs_context {
/* "grows up" - new fields go here */
- int (*drv_ibv_destroy_flow) (struct ibv_flow *flow);
- int (*lib_ibv_destroy_flow) (struct ibv_flow *flow);
- struct ibv_flow * (*drv_ibv_create_flow) (struct ibv_qp *qp,
- struct ibv_flow_attr
- *flow_attr);
- struct ibv_flow * (*lib_ibv_create_flow) (struct ibv_qp *qp,
- struct ibv_flow_attr
- *flow_attr);
+ int (*ibv_destroy_flow) (struct ibv_flow *flow);
+ void (*ABI_placeholder2) (void); /* DO NOT COPY THIS GARBAGE */
+ struct ibv_flow * (*ibv_create_flow) (struct ibv_qp *qp,
+ struct ibv_flow_attr *flow_attr);
+ void (*ABI_placeholder1) (void); /* DO NOT COPY THIS GARBAGE */
struct ibv_qp *(*open_qp)(struct ibv_context *context,
struct ibv_qp_open_attr *attr);
struct ibv_qp *(*create_qp_ex)(struct ibv_context *context,
@@ -1137,20 +1134,20 @@ static inline struct ibv_flow *ibv_create_flow(struct ibv_qp *qp,
struct ibv_flow_attr *flow)
{
struct verbs_context *vctx = verbs_get_ctx_op(qp->context,
- lib_ibv_create_flow);
- if (!vctx || !vctx->lib_ibv_create_flow)
+ ibv_create_flow);
+ if (!vctx || !vctx->ibv_create_flow)
return NULL;
- return vctx->lib_ibv_create_flow(qp, flow);
+ return vctx->ibv_create_flow(qp, flow);
}
static inline int ibv_destroy_flow(struct ibv_flow *flow_id)
{
struct verbs_context *vctx = verbs_get_ctx_op(flow_id->context,
- lib_ibv_destroy_flow);
- if (!vctx || !vctx->lib_ibv_destroy_flow)
+ ibv_destroy_flow);
+ if (!vctx || !vctx->ibv_destroy_flow)
return -ENOSYS;
- return vctx->lib_ibv_destroy_flow(flow_id);
+ return vctx->ibv_destroy_flow(flow_id);
}
/**
diff --git a/src/device.c b/src/device.c
index 9642eadba8d0..c6cfa950ca9a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -163,16 +163,20 @@ struct ibv_context *__ibv_open_device(struct ibv_device *device)
ret = verbs_device->init_context(verbs_device, context, cmd_fd);
if (ret)
goto verbs_err;
-
- /* initialize *all* library ops to either lib calls or
- * directly to provider calls.
- * context_ex->lib_new_func1 = __verbs_new_func1;
- * context_ex->lib_new_func2 = __verbs_new_func2;
+ /*
+ * In order to maintain backward/forward binary compatibility
+ * with libmlx4-1.0.6, which has the original version of the
+ * flow steering patches, we need to set the two
+ * ABI_compat_placeholder entries to match the driver
+ * set flow entries. This is because, in the specific instance
+ * of using libmlx4-1.0.6 with the fixed version of
+ * libibvberbs, the ibv_create_flow inline function already
+ * compiled into libmlx4-1.0.6 will be loooking in the
+ * ABI_placeholder spots for the function pointer to the
+ * create and destroy flow verbs.
*/
- context_ex->lib_ibv_create_flow =
- context_ex->drv_ibv_create_flow;
- context_ex->lib_ibv_destroy_flow =
- context_ex->drv_ibv_destroy_flow;
+ context_ex->ABI_placeholder1 = (void (*)(void)) context_ex->ibv_create_flow;
+ context_ex->ABI_placeholder2 = (void (*)(void)) context_ex->ibv_destroy_flow;
}
context->device = device;
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH libibverbs] Fix create/destroy flow API
[not found] ` <9665e46a71940f2721b30fdb4aaa0853161babc9.1441401379.git.dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2015-09-04 21:32 ` Jason Gunthorpe
[not found] ` <20150904213226.GB20758-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Jason Gunthorpe @ 2015-09-04 21:32 UTC (permalink / raw)
To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
On Fri, Sep 04, 2015 at 05:17:38PM -0400, Doug Ledford wrote:
> + /*
> + * In order to maintain backward/forward binary compatibility
> + * with libmlx4-1.0.6, which has the original version of the
> + * flow steering patches, we need to set the two
> + * ABI_compat_placeholder entries to match the driver
> + * set flow entries. This is because, in the specific instance
> + * of using libmlx4-1.0.6 with the fixed version of
> + * libibvberbs, the ibv_create_flow inline function already
> + * compiled into libmlx4-1.0.6 will be loooking in the
> + * ABI_placeholder spots for the function pointer to the
> + * create and destroy flow verbs.
> */
This isn't quite the right comment, it has very little to do with mlx,
ibv_create_flow is the user entry point, the above applies to
everything linked to ibverbs.
My suggestion was to not change the ibverbs->user ABI at all and just
mangle the driver side, ie move the ABI_placeholder to what was drv_
instead of lib_.
Can't see anything wrong with it this way, off hand, other than the
comment.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH libibverbs] Fix create/destroy flow API
[not found] ` <20150904213226.GB20758-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2015-09-04 21:51 ` Doug Ledford
[not found] ` <55EA124F.2070305-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Doug Ledford @ 2015-09-04 21:51 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]
On 09/04/2015 05:32 PM, Jason Gunthorpe wrote:
> On Fri, Sep 04, 2015 at 05:17:38PM -0400, Doug Ledford wrote:
>> + /*
>> + * In order to maintain backward/forward binary compatibility
>> + * with libmlx4-1.0.6, which has the original version of the
>> + * flow steering patches, we need to set the two
>> + * ABI_compat_placeholder entries to match the driver
>> + * set flow entries. This is because, in the specific instance
>> + * of using libmlx4-1.0.6 with the fixed version of
>> + * libibvberbs, the ibv_create_flow inline function already
>> + * compiled into libmlx4-1.0.6 will be loooking in the
>> + * ABI_placeholder spots for the function pointer to the
>> + * create and destroy flow verbs.
>> */
>
> This isn't quite the right comment, it has very little to do with mlx,
> ibv_create_flow is the user entry point, the above applies to
> everything linked to ibverbs.
You're right. I'll correct that.
> My suggestion was to not change the ibverbs->user ABI at all and just
> mangle the driver side, ie move the ABI_placeholder to what was drv_
> instead of lib_.
As you pointed out, this doesn't make the entire matrix of old/new
driver/libibverbs/app work. I can think of two things that would be
ugly about doing it this way if we wanted to make that matrix fully
operational. With the way I did it, things are clean in the new driver
and mostly clean in the new libibverbs, and the full matrix works.
> Can't see anything wrong with it this way, off hand, other than the
> comment.
Thanks for looking at it ;-)
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG KeyID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH libibverbs] Fix create/destroy flow API
[not found] ` <55EA124F.2070305-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2015-09-04 21:59 ` Doug Ledford
0 siblings, 0 replies; 4+ messages in thread
From: Doug Ledford @ 2015-09-04 21:59 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]
On 09/04/2015 05:51 PM, Doug Ledford wrote:
> On 09/04/2015 05:32 PM, Jason Gunthorpe wrote:
>> On Fri, Sep 04, 2015 at 05:17:38PM -0400, Doug Ledford wrote:
>>> + /*
>>> + * In order to maintain backward/forward binary compatibility
>>> + * with libmlx4-1.0.6, which has the original version of the
>>> + * flow steering patches, we need to set the two
>>> + * ABI_compat_placeholder entries to match the driver
>>> + * set flow entries. This is because, in the specific instance
>>> + * of using libmlx4-1.0.6 with the fixed version of
>>> + * libibvberbs, the ibv_create_flow inline function already
>>> + * compiled into libmlx4-1.0.6 will be loooking in the
>>> + * ABI_placeholder spots for the function pointer to the
>>> + * create and destroy flow verbs.
>>> */
>>
>> This isn't quite the right comment, it has very little to do with mlx,
>> ibv_create_flow is the user entry point, the above applies to
>> everything linked to ibverbs.
>
> You're right. I'll correct that.
The new cokment:
/*
* In order to maintain backward/forward binary
compatibility
* with apps compiled against libibverbs-1.1.8 that use the
* flow steering addition, we need to set the two
* ABI_placeholder entries to match the driver set flow
* entries. This is because apps compiled against
* libibverbs-1.1.8 use an inline ibv_create_flow and
* ibv_destroy_flow function that looks in the placeholder
* spots for the proper entry points. For apps compiled
* against libibverbs-1.1.9 and later, the inline functions
* will be looking in the right place.
*/
>> My suggestion was to not change the ibverbs->user ABI at all and just
>> mangle the driver side, ie move the ABI_placeholder to what was drv_
>> instead of lib_.
>
> As you pointed out, this doesn't make the entire matrix of old/new
> driver/libibverbs/app work. I can think of two things that would be
> ugly about doing it this way if we wanted to make that matrix fully
> operational. With the way I did it, things are clean in the new driver
> and mostly clean in the new libibverbs, and the full matrix works.
>
>> Can't see anything wrong with it this way, off hand, other than the
>> comment.
>
> Thanks for looking at it ;-)
>
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG KeyID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-09-04 21:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-04 21:17 [PATCH libibverbs] Fix create/destroy flow API Doug Ledford
[not found] ` <9665e46a71940f2721b30fdb4aaa0853161babc9.1441401379.git.dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2015-09-04 21:32 ` Jason Gunthorpe
[not found] ` <20150904213226.GB20758-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2015-09-04 21:51 ` Doug Ledford
[not found] ` <55EA124F.2070305-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2015-09-04 21:59 ` Doug Ledford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).