Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1269433

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

Show all headers | View raw

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

No.  Direct cases _never_ calls ib_req_notify.  Its for the case where
the SRP case polls the send CQ only from the same context it sends for
without any interrupt notification at al.

> +static int __ib_process_cq(struct ib_cq *cq, int budget)
> +	while ((n = ib_poll_cq(cq, IB_POLL_BATCH, cq->wc)) > 0) {
> 
> Does an unnecessary ib_poll_cq call in common cases. I'd suggest
> change the result to bool and do:
> 
> // true return means the caller should attempt ib_req_notify_cq
> while ((n = ib_poll_cq(cq, IB_POLL_BATCH, cq->wc)) > 0) {
>  for (...)
>  if (n != IB_POLL_BATCH)
>    return true;
>  completed += n;
>  if (completed > budget)
>     return false;
> }
> return true;
> 
> And then change call site like:
> 
> static void ib_cq_poll_work(struct work_struct *work)
> {
>     if (__ib_process_cq(...))
>         if (ib_req_notify_cq(cq, IB_POLL_FLAGS) == 0)
> 	    return;
>     // Else we need to loop again.
>     queue_work(ib_comp_wq, &cq->work);
> }
> 
> Which avoids the rearm.
> 
> void ib_process_cq_direct(struct ib_cq *cq)
> {
>    while (1) {
>        if (__ib_process_cq(..) &&
>            ib_req_notify_cq(cq, IB_POLL_FLAGS) == 0)
>            return;
>    }
> }
> 
> Which adds the inf loop and rearm.
> 
> etc for softirq

For the workqueue and softirq cases this looks reasonable.  For the
direct case there is no rearming, though.

> Perhaps ib_req_notify_cq should be folded into __ib_process_cq, then
> it can trivially honour the budget on additional loops from
> IB_CQ_REPORT_MISSED_EVENTS.

Which also defeats this proposal.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 2/9] IB: add a proper completion queue abstraction Christoph Hellwig <hch@lst.de> - 2015-11-13 15:00 +0100
  Re: [PATCH 2/9] IB: add a proper completion queue abstraction Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-11-13 19:30 +0100
    Re: [PATCH 2/9] IB: add a proper completion queue abstraction Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-11-13 23:10 +0100
      Re: [PATCH 2/9] IB: add a proper completion queue abstraction Christoph Hellwig <hch@lst.de> - 2015-11-14 08:20 +0100
    Re: [PATCH 2/9] IB: add a proper completion queue abstraction Christoph Hellwig <hch@lst.de> - 2015-11-14 08:10 +0100
  Re: [PATCH 2/9] IB: add a proper completion queue abstraction Sagi Grimberg <sagig@dev.mellanox.co.il> - 2015-11-15 10:50 +0100
    Re: [PATCH 2/9] IB: add a proper completion queue abstraction Christoph Hellwig <hch@lst.de> - 2015-11-15 14:00 +0100
      Re: [PATCH 2/9] IB: add a proper completion queue abstraction Sagi Grimberg <sagig@dev.mellanox.co.il> - 2015-11-15 14:30 +0100

csiph-web