Re: [PATCH libibverbs V3 3/3] Add ibv_query_port_ex support

Matan Barak
2014-05-12T12:22:10Z

On 8/5/2014 10:09 PM, Jason Gunthorpe wrote:
> On Thu, May 08, 2014 at 09:51:23AM +0300, Or Gerlitz wrote:
>
>> +struct ibv_port_attr_ex {
>> + union {
>> + struct {
>> + enum ibv_port_state state;
>> + enum ibv_mtu max_mtu;
>> + enum ibv_mtu active_mtu;
>> + int gid_tbl_len;
>> + uint32_t port_cap_flags;
>> + uint32_t max_msg_sz;
>> + uint32_t bad_pkey_cntr;
>> + uint32_t qkey_viol_cntr;
>> + uint16_t pkey_tbl_len;
>> + uint16_t lid;
>> + uint16_t sm_lid;
>> + uint8_t lmc;
>> + uint8_t max_vl_num;
>> + uint8_t sm_sl;
>> + uint8_t subnet_timeout;
>> + uint8_t init_type_reply;
>> + uint8_t active_width;
>> + uint8_t active_speed;
>> + uint8_t phys_state;
>> + uint8_t link_layer;
>> + uint8_t reserved;
>> + };
>> + struct ibv_port_attr port_attr;
>> + };
>> + uint32_t comp_mask;
>> + uint32_t mask1;
>> +};
>
> I really don't like this deviation from the standard _ex
> pattern.
>
> Anonymous struct/union is a C11 feature and GNU extension. I don't
> think is appropriate for a user library to rely on. We cannot assume
> the user is has a compiler in C11 mode.
>
> The cannonical layout should be:
>
> struct ibv_port_attr_ex {
> uint64_t comp_mask;
> struct ibv_port_attr port_attr;
> // New stuff goes here
> }
>

It's not mandatory, but I think it could prevent future breakages and
inconsistencies. Anyway, anonymous unions are supported in icc 9.2+,
clang 3.1+ and of course gcc. However, I'll remove this in the next
version of this series.

> It is fine to use comp_mask to indicate all the fields, no need for
> the weird mask1, lets not over complicate things for users.

I don't think that's the right thing to do. According to my
understanding, the prefix of the extension verb is fully compatible with
the old structure. Afterwards, comp_mask has a bit for every new field.
mask1 is a new field that indicates which of the "old" values the user
is interested in. If we want to be strict with the extension verbs
definition, lets keep it all the way.

>
>> struct verbs_context {
>> /* "grows up" - new fields go here */
>> + int (*drv_query_port_ex)(struct ibv_context *context, uint8_t port_num,
>> + struct ibv_port_attr_ex *port_attr);
>> + int (*lib_query_port_ex)(struct ibv_context *context, uint8_t port_num,
>> + struct ibv_port_attr_ex *port_attr);
>> struct ibv_ah * (*drv_ibv_create_ah_ex)(struct ibv_pd *pd,
>> struct ibv_ah_attr_ex *attr);
>> int (*drv_ibv_destroy_flow) (struct ibv_flow *flow);
>
> I'm not sure what Roland decided to merge, but I am surprised to see
> drv_ elements here. That was nix'd in the round of review of the first
> patch set. eg create_qp_ex/etc don't have them.
>
> The flow is such that the verbs code has a chance to capture and
> override the function pointer after the driver sets it, there is no
> purpose to the drv_ values.
>
> I wouldn't like to see more added, and I think you should make a patch
> to ensure they are not necessary before it propogates too far.
>

I'll remove the drv_query_port_ex function and rename lib_query_port_ex
to query_port_ex and drv_ibv_create_ah_ex to create_ah_ex.

>> diff --git a/src/verbs.c b/src/verbs.c
>> index e7343a9..f8245b0 100644
>> +++ b/src/verbs.c
>> @@ -550,7 +550,7 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
>> int err;
>> struct ibv_ah *ah = NULL;
>> #ifndef NRESOLVE_NEIGH
>> - struct ibv_port_attr port_attr;
>> + struct ibv_port_attr_ex port_attr;
>> int dst_family;
>> int src_family;
>> int oif;
>> @@ -570,7 +570,10 @@ struct ibv_ah *__ibv_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
>> goto return_ah;
>> }
>>
>> - err = ibv_query_port(pd->context, attr->port_num, &port_attr);
>> + port_attr.comp_mask = IBV_QUERY_PORT_EX_ATTR_MASK1;
>> + port_attr.mask1 = IBV_QUERY_PORT_EX_LINK_LAYER |
>> + IBV_QUERY_PORT_EX_CAP_FLAGS;
>> + err = ibv_query_port_ex(pd->context, attr->port_num, &port_attr);
>>
>> if (err) {
>> fprintf(stderr, PFX "ibv_create_ah failed to query port.\n");
>
> I wonder if this belongs in a seperate patch, I had to read it twice
> to realize this change was to take advantage of the reduced query
> performance optimization.

Thanks, I'll move that to a different patch.

>
> Jason
>

Thanks for the comments,
Matan