Re: [PATCH for-next V2 0/9] Add completion timestamping support

[Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 6/2/2015 5:35 PM, Doug Ledford wrote:
> On Mon, 2015-06-01 at 10:43 -0600, Jason Gunthorpe wrote:
>> On Mon, Jun 01, 2015 at 07:25:04AM -0400, Doug Ledford wrote:
>>
>>> attempted abstraction of ibverbs.  Passing in the wc struct allows the
>>> driver to internally allocate a wc struct with extra private elements
>>> and pass that back to the user, when the user passes it back to
>>> ibv_get_timestamp the elements are there in the private portion of the
>>> struct.
>> wc structures are allocated by the caller, there is no option for the
>> driver to create private elements.
> Well, they *are* using an extended work completion structure.  Unlike
> what I mentioned, where they create a larger one themselves, you have to
> allocate a struct ibv_wc_ex instead of a struct ibv_wc and then you have
> to call poll_cq_ex, which expects a struct ibv_wc_ex.
>
> So, just so everyone is clear on this point: the current user space
> implementation of this feature creates an unversioned, newly named
> ibv_wc_ex struct that is ibv_wc with a 64bit timestamp tacked on at the
> end (not 64bit aligned either).  If we ever wanted to have a different
> extension to our ibv_wc struct, there is no good way to do that.  If, at
> some point, we had multiple extension and the user was able to select
> which they wanted to utilize, this structure extension is not flexible
> enough to deal with that.  At a minimum, if we are going to have a one
> shot extension to the wc struct like this, I would prefer to see it
> called struct ibv_wc_timestamp and there be a ibv_poll_cq_timestamp.  At
> least that way people would not use the generic _ex and assume this is
> the one and only _ex that we will ever need for work completions.
>
> Jason, when the XRC and flow steering extensions were added to
> libibverbs, you complained loudly that they were not added in the agreed
> upon format and cited a previous on list discussion.  Do you have a link
> to that discussion?

Doug,

Do we agree that this part of the discussion (and also the below point) 
are related to the libibverbs API to applications and not to the kernel 
-> user API to support time-stamping?

Or.

>
>> AFAIK, Christoph's use case is essentially the only meaningful use
>> case for this feature, generalizing too much may destroy the
>> performance that is valuable here.
> There is actually room in a 64byte cacheline for two 64bit timestamps
> and another 2 bytes of padding or something else.
>
>


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