On Fri, Nov 13, 2015 at 11:57:56AM -0800, Bart Van Assche wrote: > I think this is the conversation you are referring to: "About a shortcoming > of the verbs API" (http://thread.gmane.org/gmane.linux.drivers.rdma/5028). > That conversation occurred five years ago, which means that you have an > excellent memory :-)
Heh, the whole thread is interesting, but this message is what I was thinking of http://thread.gmane.org/gmane.linux.drivers.rdma/5028 And it looks like this patch is OK relative to that discussion. > I doesn't seem to me like Christoph wanted to support dynamic switching > between the IB_POLL_DIRECT, IB_POLL_SOFTIRQ and IB_POLL_WORKQUEUE polling > modes. I think this should have been mentioned in the patch description. Indeed. Which is probably OK. > The implementation of this patch makes it clear that it is essential that > all polling is serialized. The WC array that is used for polling is embedded > in the CQ and is not protected against concurrent access. This means that it > is essential that _ib_process_cq() calls are serialized. I need some more > time to verify whether such serialization is always guaranteed by this > patch. Yes, the two big design/review checks - ib_process_cq is fully serialized/etc - All re-arm cases are done properly - rearm is only called when the CQ is empty and all cases where it is not empty guarentee that the polling loop happens again. 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. And this structure: +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 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. 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 http://vger.kernel.org/majordomo-info.html
