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

From: Bart Van Assche
Date: Tue Nov 17 2015 - 12:53:12 EST

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 != IB_POLL_DIRECT);
+ __ib_process_cq(cq, INT_MAX);

My proposal is to drop this function and to export __ib_process_cq() instead (with or without renaming). That will allow callers of this function to compare the poll budget with the number of completions that have been processed and use that information to decide whether or not to call this function again.

+static void ib_cq_poll_work(struct work_struct *work)
+ struct ib_cq *cq = container_of(work, struct ib_cq, work);
+ int completed;
+ completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE);
+ if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
+ ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
+ queue_work(ib_comp_wq, &cq->work);
+static void ib_cq_completion_workqueue(struct ib_cq *cq, void *private)
+ queue_work(ib_comp_wq, &cq->work);

The above code will cause all polling to occur on the context of the CPU that received the completion interrupt. This approach is not powerful enough. For certain workloads throughput is higher if work completions are processed by another CPU core on the same CPU socket. Has it been considered to make the CPU core on which work completions are processed configurable ?

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 62b6cba..3027824 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -457,10 +457,11 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
static void srp_destroy_qp(struct srp_rdma_ch *ch)
static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
- static struct ib_recv_wr wr = { .wr_id = SRP_LAST_WR_ID };
+ static struct ib_recv_wr wr = { 0 };
struct ib_recv_wr *bad_wr;
int ret;

Since the 'wr' structure is static I don't think it needs to be zero-initialized explicitly.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at