Re: [PATCH rdma-next 00/16] Flow counters support

[Guy Shattah <sguy-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

    
On Wed, Oct 25, 2017 at 06:17:15PM +0300, Jason Gunthorpe wrote:
>On Wed, Oct 25, 2017 at 05:58:15PM +0300, Yishai Hadas wrote:
>
>> Hi Jason, please go over the patches/uapi, we believe that you'll find it
>> quite easy to be used by an application, however we plan to improve as of
>> below.
>
>No, it is not, this proposal relies too much on device-specific
>information to make sense in a multi-vendor context. Counters should
>not be grouped in a hardware unique way.

counters are not group in hardware unique but in an intuitive way.
all counters related to flow (ibv_flow) are grouped togeger, and following
the same pattern all counters related to QP (ibv_qp) should be grouped together.

>
>But it is still hard to understand what you are talking about, so
>maybe if you provide a short and clearer example..
>
>> #The create API:
>> We found it better to stay with our suggested API with few changes, below
>> notes were considered.
>> 
>> 1) There might be a hardware that doesn't support attaching a counter object
>> to an existing object, your suggested API to set sampling points relay on
>> that ability.
>
>That is unfortunate, and kind of lame, but you can still use the
>proposed API by re-ordering things. Continue to have
>ibv_add_sampling_point_flow, however a NULL flow argument will make
>the counters become added when the flow is created:
>
> counters = ibv_create_counters([..]);
> ibv_add_sampling_point_flow(counters, 1, NULL, [..])
> flow_attr.counters = counters;
> ibv_create_flow(qp, &flow_attr);

As mentioned earlier - current hardware support attachment of counters
only during the creation of the object (flow/qp/etc).
Your proposal could be useful in the case where a structure of a dynamic
counter set is to be modified. However - since it is not the current case,
there is no benefit in dividing the build of such a counter-set into several steps.
Hence we suggest to supply the creation API with instructions on how to create such a set. 
Having said that - I'm not saying that the partial steps are totally useless. 
These steps could be used as 'syntactic sugar' in order to change some kind
of structured which would be passed to the creation method. 
Hence, methods such as  ibv_add_sampling_point_flow() should either be 
user-space-verbs only, or as macros or totally not there.

>
>> 2) We prefer using one single system call to create the ib_counter object
>> with its required counters instead of creating it with sampling points which
>> might involve many system calls.
>
>No. More system calls are good for something like this, there is a lot
>of possibly future variety here. Systems calls are better than huge
>structs and comp mask.
>
>It allows you to ditch the silly describe API by making the creation
>side more granular.
See response above.

Guy

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