RE: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

RE: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

[Felix Marti]

> -----Original Message-----
> From: openib-general-bounces@openib.org [mailto:openib-general-
> bounces@openib.org] On Behalf Of Steve Wise
> Sent: Friday, January 05, 2007 6:22 AM
> To: Roland Dreier
> Cc: linux-kernel@vger.kernel.org; openib-general@openib.org;
> netdev@vger.kernel.org
> Subject: Re: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes
> 
> On Thu, 2007-01-04 at 13:34 -0800, Roland Dreier wrote:
> > OK, I'm back from vacation today.
> >
> > Anyway I don't have a definitive statement on this right now.  I
guess
> > I agree that I don't like having an extra parameter to a function
that
> > should be pretty fast (although req notify isn't quite as hot as
> > something like posting a send request or polling a cq), given that
it
> > adds measurable overhead.  (And I am surprised that the overhead is
> > measurable, since 3 arguments still fit in registers, but OK).
> >
> > I also agree that adding an extra entry point just to pass in the
user
> > data is ugly, and also racy.
> >
> > Giving the kernel driver a pointer it can read seems OK I guess,
> > although it's a little ugly to have a backdoor channel like that.
> >
> 
> Another alternative is for the cq-index u32 memory to be allocated by
> the kernel and mapped into the user process.  So the lib can
read/write
> it, and the kernel can read it directly.  This is the fastest way
> perfwise, but I didn't want to do it because of the page granularity
of
> mapping.  IE it would require a page of address space (and backing
> memory I guess) just for 1 u32.  The CQ element array memory is
already
> allocated this way (and its DMA coherent too), but I didn't want to
> overload that memory with this extra variable either.  Mapping just
> seemed ugly and wasteful to me.
> 
> So given 3 approaches:
> 
> 1) allow user data to be passed into ib_req_notify_cq() via the
standard
> uverbs mechanisms.
> 
> 2) hide this in the chelsio driver and have the driver copyin the info
> directly.
> 
> 3) allocate the memory for this in the kernel and map it to the user
> process.
> 
> I chose 1 because it seemed the cleanest from an architecture point of
> view and I didn't think it would impact performance much.

[Felix Marti] In addition, is arming the CQ really in the performance
path? - Don't apps poll the CQ as long as there are pending CQEs and
only arm the CQ for notification once there is nothing left to do? If
this is the case, it would mean that we waste a few cycles 'idle'
cycles. Steve, next to the micro benchmark that you did, did you run any
performance benchmark that actually moves traffic? If so, did you see a
difference in performance?

> 
> 
> Steve.
> 
> 
> 
> 
> 
> _______________________________________________
> openib-general mailing list
> openib-general@openib.org
> http://openib.org/mailman/listinfo/openib-general
> 
> To unsubscribe, please visit
http://openib.org/mailman/listinfo/openib-general

RE: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

[Steve Wise]

On Fri, 2007-01-05 at 09:32 -0800, Felix Marti wrote:
> 
> > -----Original Message-----
> > From: openib-general-bounces@openib.org [mailto:openib-general-
> > bounces@openib.org] On Behalf Of Steve Wise
> > Sent: Friday, January 05, 2007 6:22 AM
> > To: Roland Dreier
> > Cc: linux-kernel@vger.kernel.org; openib-general@openib.org;
> > netdev@vger.kernel.org
> > Subject: Re: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes
> > 
> > On Thu, 2007-01-04 at 13:34 -0800, Roland Dreier wrote:
> > > OK, I'm back from vacation today.
> > >
> > > Anyway I don't have a definitive statement on this right now.  I
> guess
> > > I agree that I don't like having an extra parameter to a function
> that
> > > should be pretty fast (although req notify isn't quite as hot as
> > > something like posting a send request or polling a cq), given that
> it
> > > adds measurable overhead.  (And I am surprised that the overhead is
> > > measurable, since 3 arguments still fit in registers, but OK).
> > >
> > > I also agree that adding an extra entry point just to pass in the
> user
> > > data is ugly, and also racy.
> > >
> > > Giving the kernel driver a pointer it can read seems OK I guess,
> > > although it's a little ugly to have a backdoor channel like that.
> > >
> > 
> > Another alternative is for the cq-index u32 memory to be allocated by
> > the kernel and mapped into the user process.  So the lib can
> read/write
> > it, and the kernel can read it directly.  This is the fastest way
> > perfwise, but I didn't want to do it because of the page granularity
> of
> > mapping.  IE it would require a page of address space (and backing
> > memory I guess) just for 1 u32.  The CQ element array memory is
> already
> > allocated this way (and its DMA coherent too), but I didn't want to
> > overload that memory with this extra variable either.  Mapping just
> > seemed ugly and wasteful to me.
> > 
> > So given 3 approaches:
> > 
> > 1) allow user data to be passed into ib_req_notify_cq() via the
> standard
> > uverbs mechanisms.
> > 
> > 2) hide this in the chelsio driver and have the driver copyin the info
> > directly.
> > 
> > 3) allocate the memory for this in the kernel and map it to the user
> > process.
> > 
> > I chose 1 because it seemed the cleanest from an architecture point of
> > view and I didn't think it would impact performance much.
> 
> [Felix Marti] In addition, is arming the CQ really in the performance
> path? - Don't apps poll the CQ as long as there are pending CQEs and
> only arm the CQ for notification once there is nothing left to do? If
> this is the case, it would mean that we waste a few cycles 'idle'
> cycles. 

I tend to agree.  This shouldn't be the hot perf path like
post_send/recv and poll.

> Steve, next to the micro benchmark that you did, did you run any
> performance benchmark that actually moves traffic? If so, did you see a
> difference in performance?
> 

No.  But I didn't explicitly measure with and without this one single
change.

Re: [PATCH v4 01/13] Linux RDMA Core Changes

[Michael S. Tsirkin]

> [Felix Marti] In addition, is arming the CQ really in the performance
> path? - Don't apps poll the CQ as long as there are pending CQEs and
> only arm the CQ for notification once there is nothing left to do? If
> this is the case, it would mean that we waste a few cycles 'idle'
> cycles.

Applications such as IPoIB might queue up packets, then ARM the CQ,
and only then they are processed by the upper layers in the stack.
So arming the CQ is on hot datapath.

-- 
MST

Re: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

[Steve Wise]

On Thu, 2007-01-04 at 13:34 -0800, Roland Dreier wrote:
> OK, I'm back from vacation today.
> 
> Anyway I don't have a definitive statement on this right now.  I guess
> I agree that I don't like having an extra parameter to a function that
> should be pretty fast (although req notify isn't quite as hot as
> something like posting a send request or polling a cq), given that it
> adds measurable overhead.  (And I am surprised that the overhead is
> measurable, since 3 arguments still fit in registers, but OK).
> 
> I also agree that adding an extra entry point just to pass in the user
> data is ugly, and also racy.
> 
> Giving the kernel driver a pointer it can read seems OK I guess,
> although it's a little ugly to have a backdoor channel like that.
> 

Another alternative is for the cq-index u32 memory to be allocated by
the kernel and mapped into the user process.  So the lib can read/write
it, and the kernel can read it directly.  This is the fastest way
perfwise, but I didn't want to do it because of the page granularity of
mapping.  IE it would require a page of address space (and backing
memory I guess) just for 1 u32.  The CQ element array memory is already
allocated this way (and its DMA coherent too), but I didn't want to
overload that memory with this extra variable either.  Mapping just
seemed ugly and wasteful to me. 

So given 3 approaches:

1) allow user data to be passed into ib_req_notify_cq() via the standard
uverbs mechanisms.

2) hide this in the chelsio driver and have the driver copyin the info
directly.

3) allocate the memory for this in the kernel and map it to the user
process.

I chose 1 because it seemed the cleanest from an architecture point of
view and I didn't think it would impact performance much.


Steve.

Re: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

[Steve Wise]

On Thu, 2007-01-04 at 13:34 -0800, Roland Dreier wrote:
> OK, I'm back from vacation today.
> 
> Anyway I don't have a definitive statement on this right now.  I guess
> I agree that I don't like having an extra parameter to a function that
> should be pretty fast (although req notify isn't quite as hot as
> something like posting a send request or polling a cq), given that it
> adds measurable overhead.  (And I am surprised that the overhead is
> measurable, since 3 arguments still fit in registers, but OK).
> 
> I also agree that adding an extra entry point just to pass in the user
> data is ugly, and also racy.
> 
> Giving the kernel driver a pointer it can read seems OK I guess,
> although it's a little ugly to have a backdoor channel like that.
> 
> I'm somewhat surprised the driver has to go into the kernel to rearm a
> CQ -- what makes the operation need kernel privileges?  (Sorry for not
> reading the code)
> -

Rearming the CQ requires reading and writing to a global adapter
register that is shared and thus needs to be protected.  They didn't
architect the rearm to be a direct user operation.

Steve.

Re: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

[Roland Dreier]

OK, I'm back from vacation today.

Anyway I don't have a definitive statement on this right now.  I guess
I agree that I don't like having an extra parameter to a function that
should be pretty fast (although req notify isn't quite as hot as
something like posting a send request or polling a cq), given that it
adds measurable overhead.  (And I am surprised that the overhead is
measurable, since 3 arguments still fit in registers, but OK).

I also agree that adding an extra entry point just to pass in the user
data is ugly, and also racy.

Giving the kernel driver a pointer it can read seems OK I guess,
although it's a little ugly to have a backdoor channel like that.

I'm somewhat surprised the driver has to go into the kernel to rearm a
CQ -- what makes the operation need kernel privileges?  (Sorry for not
reading the code)

Re: [openib-general] [PATCH v4 01/13] Linux RDMA Core Changes

[Steve Wise]

> > 
> > So what does this tell you?
> > To me it looks like there's a measurable speed difference,
> > and so we should find a way (e.g. what I proposed) to enable chelsio userspace
> > without adding overhead to other low level drivers or indeed chelsio kernel level code.
> > 
> > What do you think? Roland?
> > 
> 
> I think having a 2nd function to set the udata seems onerous.
> 
> 

Roland, 

If you think I should not add the udata parameter to the req_notify_cq()
provider verb, then I can rework the chelsio driver:

1) at cq creation time, pass the virtual address of the u32 used by the
library to track the current cq index.  That way the chelsio kernel
driver can save the address in its kernel cq context for later use.

2) change chelsio's req_notify_cq() to copy in the current cq index
value directly for rearming.

This puts all the burden on the chelsio driver, which is apparently the
only one that needs this functionality.  

Lemme know.

Steve.