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: 3027 bytes --]

On Tue, 2015-06-02 at 17:44 +0300, Or Gerlitz wrote:
> 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?

Yes.

> 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


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