***SPAM*** Re: [ewg] [PATCH 1/2 v2]libibvers: add create_qp_expanded

[Ron Livne]

Roland,
do you have any comments on what Jack wrote?

>> Also, I wonder if it's worth a new verb in the kernel ABI for this.
>> Maybe we should add a new command in the ABI where libibverbs can pass
>> in a bitmask of supported extensions, and the kernel can respond with
>> which extensions it supports.  And then we can just continue to use the
>> reserved field in the existing create_qp command if both kernel and
>> userspace agree that they support create flags there.
>
> I think there is a problem here, because the reserved field only has 8 bits --
> we may run out of create flags quickly.  Then, we will have the problem of
> still using the reserved field for the old create flags, but needing a new
> structure for new create flags.
>

Do you still think I should avoid a new verb in the kernel?

>> I don't like the name "_expanded" when all we are doing is adding a
>> flags parameter.

I'll change the function's name to create_qp_masked and take the
ext_mask approach.

BTW, when do you think the XRC patches will be merged into ib/uverbs,
libibverbs, libmlx4?
As you know, I need to create my patches on top of the XRC patches,
and I can't create them until they're merged.

Ron



On Wed, Aug 13, 2008 at 5:26 PM, Jack Morgenstein
<jackm at dev.mellanox.co.il> wrote:
> On Tuesday 12 August 2008 22:37, Roland Dreier wrote:
>> Sorry for jumping in so late in the process, but a few big concerns:
>>
>>  >         struct ibv_qp *ibv_create_qp_expanded(struct ibv_pd *pd,
>>  >                                struct ibv_qp_init_attr *qp_init_attr,
>>  >                                uint32_t create_flags);
>>
>> I don't like the name "_expanded" when all we are doing is adding a
>> flags parameter.
> We'll need to implement any changes here very quickly if this is to get
> into the beta release of ofed 1.4 (we do not want to put an API into the
> beta, and then change it immediately).
>
> I suggested "expanded" because I was afraid of "extended" (possible
> confusion with "extended" RC QPs)
>> The next time we need to tweak this API, then we end
>> up with _extra_super_expanded or something like that.
> The names can still be changed -- we've not yet put out the API for this feature
> in userspace.
>
>> or keep
>> the name ibv_create_qp_expanded but instead of create_flags, have the
>> new parameter be ext_mask, have one bit in ext_mask indicate create
>> flags, and add create_flags to struct ibv_qp_init_attr -- then we can
>> add more extra stuff by using more bits in ext_mask.
>
> This may be doable.  We can define the ABI for the new verb using a new
> struct ibv_qp_init_attr_expanded, with lots of reserved space up front
> (for future extensions -- note that the ABI unpacks the fields in
> struct ibv_create_qp_attr, passing values individually. Since this is not
> data path, who cares if we have, say, 12 reserved DWORDS in the structure)?
> If userspace sets an extension flag that the kernel does not support, the create_qp
> verb in kernel space should return error.
>
> We need the new new structure so that we can maintain backwards compatibility for the old ABI.
> (new libraries running over an old kernel).
>
>>
>> Also, I wonder if it's worth a new verb in the kernel ABI for this.
>> Maybe we should add a new command in the ABI where libibverbs can pass
>> in a bitmask of supported extensions, and the kernel can respond with
>> which extensions it supports.  And then we can just continue to use the
>> reserved field in the existing create_qp command if both kernel and
>> userspace agree that they support create flags there.
>
> I think there is a problem here, because the reserved field only has 8 bits --
> we may run out of create flags quickly.  Then, we will have the problem of
> still using the reserved field for the old create flags, but needing a new
> structure for new create flags.
>
> I think it is better to take the "ext_mask" approach.
>
> - Jack
> _______________________________________________
> ewg mailing list
> ewg at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg
>