Re: [PATCH v1 0/3] libibverbs: On-demand paging support

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4119 bytes --]

On 09/03/2015 10:56 AM, Haggai Eran wrote:
> This series adds userspace support for on-demand paging. The first patch adds
> support for the new extended query device verb. Patch 2 adds the capability and
> interface bits related to on-demand paging, and patch 3 adds example code to
> the rc_pingpong program to use on-demand paging.

I've reviewed this series and I'm OK with it.  However, it's currently
blocked.  We need to fix the API mess that was created when the
create/destroy flow verbs were added.  I've gone back through the code,
and I'm pretty sure I know what Jason's objection to the create/destroy
flow implementation was.  In particular, we weren't supposed to have this:

 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);

There was only supposed to be ibv_create_flow and ibv_destroy_flow, not
this silly separate drv/lib versions that just amount to wasted space
and confusion.

Our choices are: 1) Leave this in place but don't do any more extensions
this way, knowing full well that this is wrong but preserving ABI or 2)
Clean this up, but break ABI.  Just to be clear, neither solution breaks
user space app ABI, just the libibverbs/libmlx4 ABI.  Option 2 does not
require any apps be recompile, only that libibverbs and libmlx4 *must*
be upgraded together or downgraded together around the ABI break.  For
future versions beyond that, we don't have to have this requirement.

What are other people's thoughts on this?  I'm of a mind to clean it up,
but it's been since May 5th of last year that this went out.  Certainly
the largest portion of users are likely running this code.  It will mean
that their next upgrade is required to be a joint upgrade between this
and libmlx4.

> Changes from v1:
> - Patch 1:
>   * move code to revert to legacy ibv_query_device when ibv_query_device_ex
>     is missing to the inline function.
>   * add an input parameter to the ibv_query_device_ex verb for future
>     extension.
>   * add the size of the ibv_device_attr_ex struct as a parameter to the
>     ibv_query_device_ex verb, to allow the verb to handle older
>     applications.
>   * check the validity of the input parameter and output struct size.
>   * remove reserved words from ibv_query_device_resp_ex, and remove unused
>     ibv_device_attr_ex_resp struct.
> - Patch 2:
>   * let print_odp_caps() receive a const pointer instead of a by-value
>     struct.
>   * check that the application has enough space for ODP capabilities in the
>     provided ibv_device_attr_ex struct.
> 
> Eli Cohen (1):
>   Add support for extended query device capabilities
> 
> Haggai Eran (1):
>   Add on-demand paging support
> 
> Majd Dibbiny (1):
>   libibverbs/examples: Support odp in rc_pingpong
> 
>  Makefile.am                   |   3 +-
>  examples/devinfo.c            | 145 +++++++++++++++++++++++++++--------------
>  examples/rc_pingpong.c        |  31 ++++++++-
>  include/infiniband/driver.h   |  10 +++
>  include/infiniband/kern-abi.h |  36 ++++++++++-
>  include/infiniband/verbs.h    |  68 ++++++++++++++++++-
>  man/ibv_query_device_ex.3     |  70 ++++++++++++++++++++
>  man/ibv_reg_mr.3              |   2 +
>  src/cmd.c                     | 147 ++++++++++++++++++++++++++++++------------
>  src/libibverbs.map            |   2 +
>  10 files changed, 420 insertions(+), 94 deletions(-)
>  create mode 100644 man/ibv_query_device_ex.3
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]