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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2327 bytes --]

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?

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


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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]