Re: [PATCH 5/9] srpt: use the new CQ API

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 11:38:48AM -0800, Bart Van Assche wrote:
> On 11/13/2015 05:46 AM, Christoph Hellwig wrote:
>> [ ... ]
>
> This patch contains two logical changes:
> - Conversion to the new CQ API.
> - Removal of the ib_srpt_compl thread.
>
> Had it been considered to implement these changes as two separate patches ?

It's intentional.  I want the new style completions to happen from
a controlled environment, so I don't want to allow calling them from
random context.
--
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

Re: [PATCH 5/9] srpt: use the new CQ API

[Bart Van Assche]

On 11/13/2015 05:46 AM, Christoph Hellwig wrote:
> [ ... ]

The previous patch and this patch look like great work to me. However, 
this patch not only reworks the SRP target driver but also prevents 
users to move the SRP completion thread to another CPU core than the CPU 
core that processes the completion interrupts (with the help of e.g. the 
taskset command). Hence my request to make that CPU core configurable in 
the second patch of this patch series.


Bart.
--
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

Re: [PATCH 5/9] srpt: use the new CQ API

[Bart Van Assche]

On 11/13/2015 05:46 AM, Christoph Hellwig wrote:

[ ... ]


This patch contains two logical changes:
- Conversion to the new CQ API.
- Removal of the ib_srpt_compl thread.

Had it been considered to implement these changes as two separate patches ?

Thanks,

Bart.
--
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

[PATCH 5/9] srpt: use the new CQ API

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 327 +-
 drivers/infiniband/ulp/srpt/ib_srpt.h |  28 +--
 2 files changed, 88 insertions(+), 267 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2b6dd71..d4bbad3 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -95,6 +95,8 @@ MODULE_PARM_DESC(srpt_service_guid,
 static struct ib_client srpt_client;
 static void srpt_release_channel(struct srpt_rdma_ch *ch);
 static int srpt_queue_status(struct se_cmd *cmd);
+static void srpt_recv_done(struct ib_cq *cq, struct ib_wc *wc);
+static void srpt_send_done(struct ib_cq *cq, struct ib_wc *wc);
 
 /**
  * opposite_dma_dir() - Swap DMA_TO_DEVICE and DMA_FROM_DEVICE.
@@ -780,12 +782,12 @@ static int srpt_post_recv(struct srpt_device *sdev,
struct ib_recv_wr wr, *bad_wr;
 
BUG_ON(!sdev);
-   wr.wr_id = encode_wr_id(SRPT_RECV, ioctx->ioctx.index);
-
list.addr = ioctx->ioctx.dma;
list.length = srp_max_req_size;
list.lkey = sdev->pd->local_dma_lkey;
 
+   ioctx->ioctx.cqe.done = srpt_recv_done;
+   wr.wr_cqe = >ioctx.cqe;
wr.next = NULL;
wr.sg_list = 
wr.num_sge = 1;
@@ -821,8 +823,9 @@ static int srpt_post_send(struct srpt_rdma_ch *ch,
list.length = len;
list.lkey = sdev->pd->local_dma_lkey;
 
+   ioctx->ioctx.cqe.done = srpt_send_done;
wr.next = NULL;
-   wr.wr_id = encode_wr_id(SRPT_SEND, ioctx->ioctx.index);
+   wr.wr_cqe = >ioctx.cqe;
wr.sg_list = 
wr.num_sge = 1;
wr.opcode = IB_WR_SEND;
@@ -1385,116 +1388,44 @@ out:
 }
 
 /**
- * srpt_handle_send_err_comp() - Process an IB_WC_SEND error completion.
- */
-static void srpt_handle_send_err_comp(struct srpt_rdma_ch *ch, u64 wr_id)
-{
-   struct srpt_send_ioctx *ioctx;
-   enum srpt_command_state state;
-   u32 index;
-
-   atomic_inc(>sq_wr_avail);
-
-   index = idx_from_wr_id(wr_id);
-   ioctx = ch->ioctx_ring[index];
-   state = srpt_get_cmd_state(ioctx);
-
-   WARN_ON(state != SRPT_STATE_CMD_RSP_SENT
-   && state != SRPT_STATE_MGMT_RSP_SENT
-   && state != SRPT_STATE_NEED_DATA
-   && state != SRPT_STATE_DONE);
-
-   /* If SRP_RSP sending failed, undo the ch->req_lim change. */
-   if (state == SRPT_STATE_CMD_RSP_SENT
-   || state == SRPT_STATE_MGMT_RSP_SENT)
-   atomic_dec(>req_lim);
-
-   srpt_abort_cmd(ioctx);
-}
-
-/**
- * srpt_handle_send_comp() - Process an IB send completion notification.
- */
-static void srpt_handle_send_comp(struct srpt_rdma_ch *ch,
- struct srpt_send_ioctx *ioctx)
-{
-   enum srpt_command_state state;
-
-   atomic_inc(>sq_wr_avail);
-
-   state = srpt_set_cmd_state(ioctx, SRPT_STATE_DONE);
-
-   if (WARN_ON(state != SRPT_STATE_CMD_RSP_SENT
-   && state != SRPT_STATE_MGMT_RSP_SENT
-   && state != SRPT_STATE_DONE))
-   pr_debug("state = %d\n", state);
-
-   if (state != SRPT_STATE_DONE) {
-   srpt_unmap_sg_to_ib_sge(ch, ioctx);
-   transport_generic_free_cmd(>cmd, 0);
-   } else {
-   pr_err("IB completion has been received too late for"
-  " wr_id = %u.\n", ioctx->ioctx.index);
-   }
-}
-
-/**
- * srpt_handle_rdma_comp() - Process an IB RDMA completion notification.
- *
  * XXX: what is now target_execute_cmd used to be asynchronous, and unmapping
  * the data that has been transferred via IB RDMA had to be postponed until the
  * check_stop_free() callback.  None of this is necessary anymore and needs to
  * be cleaned up.
  */
-static void srpt_handle_rdma_comp(struct srpt_rdma_ch *ch,
- struct srpt_send_ioctx *ioctx,
- enum srpt_opcode opcode)
+static void srpt_rdma_read_done(struct ib_cq *cq, struct ib_wc *wc)
 {
+   struct srpt_rdma_ch *ch = cq->cq_context;
+   struct srpt_send_ioctx *ioctx =
+   container_of(wc->wr_cqe, struct srpt_send_ioctx, ioctx.cqe);
+
WARN_ON(ioctx->n_rdma <= 0);
atomic_add(ioctx->n_rdma, >sq_wr_avail);
 
-   if (opcode == SRPT_RDMA_READ_LAST) {
-   if (srpt_test_and_set_cmd_state(ioctx, SRPT_STATE_NEED_DATA,
-   SRPT_STATE_DATA_IN))
-   target_execute_cmd(>cmd);
-   else
-   pr_err("%s[%d]: wrong state = %d\n", __func__,
-  __LINE__, srpt_get_cmd_state(ioctx));
-   } else {
-   WARN(true, "unexpected opcode %d\n", opcode);
+   if (unlikely(wc->status != IB_WC_SUCCESS)) {
+   pr_info("RDMA_READ for ioctx 0x%p failed with status %d\n",
+   ioctx,