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: 2615 bytes --]

On 09/04/2015 04:42 PM, Jason Gunthorpe wrote:
> On Fri, Sep 04, 2015 at 04:23:05PM -0400, Doug Ledford wrote:
>> 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
> 
> We should drop the code so people don't copy it.
> 
> Replace the one that isn't used with a 'void * __compat_foo_Bar' and
> if necessary have the common setup in ibverbs copy a non-null
> __compat_ into lib_.
> 
> This will compile-break drivers using the wrong scheme, the driver
> should be changed.
> 
> Using a new driver with a old verbs means only the flow API doesn't
> work, old driver with new verbs is stil OK. Drop a dependency in the
> mlx4 package. Safe breakage in an obscure scenario..

I like the idea, but in implementing it I found it easy enough to
preserve both forward and backward compatibility.  Proposed patch on
linux-rdma, comments welcome.


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



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