Re: [PATCH 07/13] IB: add a proper completion queue abstraction

2015-12-11 Thread Christoph Hellwig
On Thu, Dec 10, 2015 at 10:42:22AM -0800, Bart Van Assche wrote: >> +struct ib_cq *ib_alloc_cq(struct ib_device *dev, void *private, >> +int nr_cqe, int comp_vector, enum ib_poll_context poll_ctx) >> +{ > > [ ... ] >> +cq->wc = kmalloc_array(IB_POLL_BATCH, sizeof(*cq->wc),

Re: [PATCH 07/13] IB: add a proper completion queue abstraction

2015-12-10 Thread Bart Van Assche
On 12/07/2015 12:51 PM, Christoph Hellwig wrote: This adds an abstraction that allows ULP to simply pass a completion ^^^ I think this should either be changed into either "an ULP" or "ULPs". +/** + * ib_process_direct_cq - process a CQ in caller

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-24 Thread Jason Gunthorpe
On Tue, Nov 24, 2015 at 06:47:14PM -0800, Caitlin Bestler wrote: > Acknowledge packet for the last packet of the request has been > committed to the wire (including the appropriate fields for RDMA > READ response). > The target side cannot generate a response before it

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-24 Thread Jason Gunthorpe
On Tue, Nov 24, 2015 at 10:08:39AM -0700, c...@asomi.com wrote: >>> My recollection of the IB verbs is that they were unlikely to have >>> overlooked something like that. If it did slip through then there >>> should be an errata. >>verbs reflects the wire protocol and the wire

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-24 Thread Tom Talpey
On 11/24/2015 2:03 AM, Jason Gunthorpe wrote: On Mon, Nov 23, 2015 at 06:35:28PM -0800, Caitlin Bestler wrote: Are there actual HCAs that make this mistake? All IB HCAs have this behavior and require apps to see a send CQ completion before making any statements about the state of the send Q

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Sat, Nov 14, 2015 at 08:08:49AM +0100, Christoph Hellwig wrote: > On Fri, Nov 13, 2015 at 11:25:13AM -0700, Jason Gunthorpe wrote: > > For instance, like this, not fulling draining the cq and then doing: > > > > > + completed = __ib_process_cq(cq, budget); > > > + if (completed < budget) { > >

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Bart Van Assche
On 11/23/2015 02:18 PM, Jason Gunthorpe wrote: On Mon, Nov 23, 2015 at 01:54:05PM -0800, Bart Van Assche wrote: What I don't see is how SRP handles things when the sendq fills up, ie the case where __srp_get_tx_iu() == NULL. It looks like the driver starts to panic and generates printks. I can't

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Mon, Nov 23, 2015 at 01:54:05PM -0800, Bart Van Assche wrote: > Not really ... Please have a look at the SRP initiator source code. What the > SRP initiator does is to poll the send queue before sending a new > SCSI I see that. What I don't see is how SRP handles things when the sendq fills

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Mon, Nov 23, 2015 at 02:33:05PM -0800, Bart Van Assche wrote: > On 11/23/2015 02:18 PM, Jason Gunthorpe wrote: > >On Mon, Nov 23, 2015 at 01:54:05PM -0800, Bart Van Assche wrote: > >What I don't see is how SRP handles things when the > >sendq fills up, ie the case where __srp_get_tx_iu() ==

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Bart Van Assche
On 11/23/2015 01:28 PM, Jason Gunthorpe wrote: On Mon, Nov 23, 2015 at 01:04:25PM -0800, Bart Van Assche wrote: Considerable time ago the send queue in the SRP initiator driver was modified from signaled to non-signaled to reduce the number of interrupts triggered by the SRP initiator driver.

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Mon, Nov 23, 2015 at 06:35:28PM -0800, Caitlin Bestler wrote: > Is it possible for an IB HCA to transmit a response on a QP and not > in that packet or a previous packet acknowledge something that it > has delivered to the user? AFAIK, the rules of ack coalescing do not interact with the send

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Mon, Nov 23, 2015 at 07:34:53PM -0500, Tom Talpey wrote: > Been there, seen that. Bluescreened on it, mysteriously. Yes, me too :( Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Mon, Nov 23, 2015 at 03:30:42PM -0800, Caitlin Bestler wrote: >The receive completion can be safely assumed to indicate transmit >completion over a reliable connection unless your peer has gone >completely bonkers and is replying to a command that it did not >receive. Perhaps

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Tom Talpey
On 11/23/2015 7:00 PM, Jason Gunthorpe wrote: On Mon, Nov 23, 2015 at 03:30:42PM -0800, Caitlin Bestler wrote: The receive completion can be safely assumed to indicate transmit completion over a reliable connection unless your peer has gone completely bonkers and is replying to a

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Caitlin Bestler
On 11/23/2015 4:00 PM, Jason Gunthorpe wrote: On Mon, Nov 23, 2015 at 03:30:42PM -0800, Caitlin Bestler wrote: The receive completion can be safely assumed to indicate transmit completion over a reliable connection unless your peer has gone completely bonkers and is replying to a

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Bart Van Assche
On 11/23/2015 12:37 PM, Jason Gunthorpe wrote: On Sat, Nov 14, 2015 at 08:13:44AM +0100, Christoph Hellwig wrote: On Fri, Nov 13, 2015 at 03:06:36PM -0700, Jason Gunthorpe wrote: Looking at that thread and then at the patch a bit more.. +void ib_process_cq_direct(struct ib_cq *cq) [..] +

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Christoph Hellwig
On Mon, Nov 23, 2015 at 01:01:36PM -0700, Jason Gunthorpe wrote: > Okay, having now read the whole thing, I think I see the flow now. I don't > see any holes in the above, other than it is doing a bit more work > than it needs in some edges cases because it doesn't know if the CQ is > actually

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-23 Thread Jason Gunthorpe
On Mon, Nov 23, 2015 at 01:04:25PM -0800, Bart Van Assche wrote: > Considerable time ago the send queue in the SRP initiator driver was > modified from signaled to non-signaled to reduce the number of interrupts > triggered by the SRP initiator driver. The SRP initiator driver polls the > send

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-22 Thread Sagi Grimberg
I think that bart wants to allow the caller to select cpu affinity per CQ. In this case ib_alloc_cq in workqueue mode would need to accept a affinity_hint from the caller (default to wild-card WORK_CPU_UNBOUND). Hmm, true. How would be set that hint from userspace? I'd really prefer to see

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-22 Thread Sagi Grimberg
Hello Christoph, The comment about locality in the above quote is interesting. How about modifying patch 2/9 as indicated below ? The modification below does not change the behavior of this patch if ib_cq.w.cpu is not modified. And it allows users who care about locality and who want to skip

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-22 Thread Christoph Hellwig
On Sun, Nov 22, 2015 at 11:51:13AM +0200, Sagi Grimberg wrote: > >> Hello Christoph, >> >> The comment about locality in the above quote is interesting. How about >> modifying patch 2/9 as indicated below ? The modification below does not >> change the behavior of this patch if ib_cq.w.cpu is not

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-22 Thread Bart Van Assche
On 11/22/15 06:57, Sagi Grimberg wrote: I think that bart wants to allow the caller to select cpu affinity per CQ. In this case ib_alloc_cq in workqueue mode would need to accept a affinity_hint from the caller (default to wild-card WORK_CPU_UNBOUND). Hmm, true. How would be set that hint

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-20 Thread Christoph Hellwig
On Wed, Nov 18, 2015 at 10:20:14AM -0800, Bart Van Assche wrote: > Are you perhaps referring to the sysfs CPU mask that allows to control > workqueue affinity ? I think he is referring to the defintion of WQ_UNBOUND: WQ_UNBOUND Work items queued to an unbound wq are served by the

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-20 Thread Bart Van Assche
On 11/20/2015 02:16 AM, Christoph Hellwig wrote: > On Wed, Nov 18, 2015 at 10:20:14AM -0800, Bart Van Assche wrote: >> Are you perhaps referring to the sysfs CPU mask that allows to control >> workqueue affinity ? > > I think he is referring to the defintion of WQ_UNBOUND: > >WQ_UNBOUND > >

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-18 Thread Christoph Hellwig
On Tue, Nov 17, 2015 at 09:52:58AM -0800, Bart Van Assche wrote: > On 11/13/2015 05:46 AM, Christoph Hellwig wrote: >> + * context and does not ask from completion interrupts from the HCA. > > Should this perhaps be changed into "for" ? Yes. > >> + */ >> +void

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-17 Thread Bart Van Assche
On 11/13/2015 05:46 AM, Christoph Hellwig wrote: + * context and does not ask from completion interrupts from the HCA. Should this perhaps be changed into "for" ? + */ +void ib_process_cq_direct(struct ib_cq *cq) +{ + WARN_ON_ONCE(cq->poll_ctx !=

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-17 Thread Sagi Grimberg
Hi Bart, + */ +void ib_process_cq_direct(struct ib_cq *cq) +{ +WARN_ON_ONCE(cq->poll_ctx != IB_POLL_DIRECT); + +__ib_process_cq(cq, INT_MAX); +} +EXPORT_SYMBOL(ib_process_cq_direct); My proposal is to drop this function and to export __ib_process_cq() instead (with or without

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-15 Thread Sagi Grimberg
On 15/11/2015 14:55, Christoph Hellwig wrote: On Sun, Nov 15, 2015 at 11:40:02AM +0200, Sagi Grimberg wrote: I doubt INT_MAX is useful as a budget in any use-case. it can easily hog the CPU. If the consumer is given access to poll a CQ, it must be able to provide some way to budget it. Why

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-15 Thread Christoph Hellwig
On Sun, Nov 15, 2015 at 11:40:02AM +0200, Sagi Grimberg wrote: > I doubt INT_MAX is useful as a budget in any use-case. it can easily > hog the CPU. If the consumer is given access to poll a CQ, it must be > able to provide some way to budget it. Why not expose a budget argument > to the consumer?

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-13 Thread Bart Van Assche
On 11/13/2015 10:25 AM, Jason Gunthorpe wrote: On Fri, Nov 13, 2015 at 02:46:43PM +0100, Christoph Hellwig wrote: This adds an abstraction that allows ULP to simply pass a completion object and completion callback with each submitted WR and let the RDMA core handle the nitty gritty details of

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-13 Thread Jason Gunthorpe
On Fri, Nov 13, 2015 at 02:46:43PM +0100, Christoph Hellwig wrote: > This adds an abstraction that allows ULP to simply pass a completion > object and completion callback with each submitted WR and let the RDMA > core handle the nitty gritty details of how to handle completion > interrupts and

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-13 Thread Christoph Hellwig
On Fri, Nov 13, 2015 at 11:25:13AM -0700, Jason Gunthorpe wrote: > For instance, like this, not fulling draining the cq and then doing: > > > + completed = __ib_process_cq(cq, budget); > > + if (completed < budget) { > > + irq_poll_complete(>iop); > > + if

Re: [PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-13 Thread Christoph Hellwig
On Fri, Nov 13, 2015 at 03:06:36PM -0700, Jason Gunthorpe wrote: > Looking at that thread and then at the patch a bit more.. > > +void ib_process_cq_direct(struct ib_cq *cq) > [..] > + __ib_process_cq(cq, INT_MAX); > > INT_MAX is not enough, it needs to loop. > This is missing a

[PATCH 2/9] IB: add a proper completion queue abstraction

2015-11-13 Thread Christoph Hellwig
This adds an abstraction that allows ULP to simply pass a completion object and completion callback with each submitted WR and let the RDMA core handle the nitty gritty details of how to handle completion interrupts and poll the CQ. In detail there is a new ib_cqe structure which just contains

add a proper completion queue abstraction

2015-11-13 Thread Christoph Hellwig
This series adds a new RDMA core abstraction that insulated the ULPs from the nitty gritty details of CQ polling. See the individual patches for more details. Note that this series should be applied on top of my "IB: merge struct ib_device_attr into struct ib_device" patch. A git tree is also