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-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
