Re: [PATCH 08/13] IB/srpt: chain RDMA READ/WRITE requests

[Christoph Hellwig]

On Tue, Dec 29, 2015 at 10:58:24AM +0100, Bart Van Assche wrote:
> On 12/07/2015 09:51 PM, Christoph Hellwig wrote:
> > Remove struct rdma_iu and instead allocate the struct ib_rdma_wr array
> > early and fill out directly.  This allows us to chain the WRs, and thus
> > archive both less lock contention on the HCA workqueue as well as much
> > simpler error handling.
> 
> Please consider folding the patch below into this patch.

Looks fine,

Reviewed-by: Christoph Hellwig 
--
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 08/13] IB/srpt: chain RDMA READ/WRITE requests

[Bart Van Assche]

On 12/07/2015 09:51 PM, Christoph Hellwig wrote:
> Remove struct rdma_iu and instead allocate the struct ib_rdma_wr array
> early and fill out directly.  This allows us to chain the WRs, and thus
> archive both less lock contention on the HCA workqueue as well as much
> simpler error handling.

Please consider folding the patch below into this patch.

Thanks,

Bart.

[PATCH] IB/srpt: Fix a recently introduced kernel crash

BUG: unable to handle kernel paging request at 00010198
IP: [] __lock_acquire+0xa2/0x560
Call Trace:
 [] lock_acquire+0x62/0x80
 [] _raw_spin_lock_irqsave+0x43/0x60
 [] srpt_rdma_read_done+0x57/0x120 [ib_srpt]
 [] __ib_process_cq+0x43/0xc0 [ib_core]
 [] ib_cq_poll_work+0x25/0x70 [ib_core]
 [] process_one_work+0x1bd/0x460
 [] worker_thread+0x118/0x420
 [] kthread+0xe4/0x100
 [] ret_from_fork+0x3f/0x70

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 8068aff..3daab39 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1395,7 +1395,7 @@ 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);
+   container_of(wc->wr_cqe, struct srpt_send_ioctx, rdma_cqe);
 
WARN_ON(ioctx->n_rdma <= 0);
atomic_add(ioctx->n_rdma, >sq_wr_avail);
@@ -1418,7 +1418,7 @@ static void srpt_rdma_read_done(struct ib_cq *cq, struct 
ib_wc *wc)
 static void srpt_rdma_write_done(struct ib_cq *cq, struct ib_wc *wc)
 {
struct srpt_send_ioctx *ioctx =
-   container_of(wc->wr_cqe, struct srpt_send_ioctx, ioctx.cqe);
+   container_of(wc->wr_cqe, struct srpt_send_ioctx, rdma_cqe);
 
if (unlikely(wc->status != IB_WC_SUCCESS)) {
pr_info("RDMA_WRITE for ioctx 0x%p failed with status %d\n",
-- 
2.1.4


--
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 08/13] IB/srpt: chain RDMA READ/WRITE requests

[Bart Van Assche]

On 12/07/2015 12:51 PM, Christoph Hellwig wrote:

Remove struct rdma_iu and instead allocate the struct ib_rdma_wr array
early and fill out directly.  This allows us to chain the WRs, and thus
archive both less lock contention on the HCA workqueue as well as much

  ^^^

Did you perhaps intend "achieve" ?


  struct srpt_send_ioctx {
struct srpt_ioctx   ioctx;
struct srpt_rdma_ch *ch;
-   struct rdma_iu  *rdma_ius;
+   struct ib_rdma_wr   *rdma_ius;


Please rename the "rdma_ius" member into "wr" or any other name that 
shows that this member is now a work request array and nothing else.


Otherwise this patch looks fine to me.

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 08/13] IB/srpt: chain RDMA READ/WRITE requests

[Christoph Hellwig]

Remove struct rdma_iu and instead allocate the struct ib_rdma_wr array
early and fill out directly.  This allows us to chain the WRs, and thus
archive both less lock contention on the HCA workqueue as well as much
simpler error handling.

Signed-off-by: Christoph Hellwig 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 100 +-
 drivers/infiniband/ulp/srpt/ib_srpt.h |  14 +
 2 files changed, 39 insertions(+), 75 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index b8b654c..b5544b2 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1055,7 +1055,7 @@ static void srpt_unmap_sg_to_ib_sge(struct srpt_rdma_ch 
*ch,
BUG_ON(ioctx->n_rdma && !ioctx->rdma_ius);
 
while (ioctx->n_rdma)
-   kfree(ioctx->rdma_ius[--ioctx->n_rdma].sge);
+   kfree(ioctx->rdma_ius[--ioctx->n_rdma].wr.sg_list);
 
kfree(ioctx->rdma_ius);
ioctx->rdma_ius = NULL;
@@ -1082,7 +1082,7 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
struct scatterlist *sg, *sg_orig;
int sg_cnt;
enum dma_data_direction dir;
-   struct rdma_iu *riu;
+   struct ib_rdma_wr *riu;
struct srp_direct_buf *db;
dma_addr_t dma_addr;
struct ib_sge *sge;
@@ -1115,7 +1115,8 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
nrdma = (count + SRPT_DEF_SG_PER_WQE - 1) / SRPT_DEF_SG_PER_WQE
+ ioctx->n_rbuf;
 
-   ioctx->rdma_ius = kzalloc(nrdma * sizeof *riu, GFP_KERNEL);
+   ioctx->rdma_ius = kcalloc(nrdma, sizeof(*ioctx->rdma_ius),
+   GFP_KERNEL);
if (!ioctx->rdma_ius)
goto free_mem;
 
@@ -1139,9 +1140,9 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
 j < count && i < ioctx->n_rbuf && tsize > 0; ++i, ++riu, ++db) {
rsize = be32_to_cpu(db->len);
raddr = be64_to_cpu(db->va);
-   riu->raddr = raddr;
+   riu->remote_addr = raddr;
riu->rkey = be32_to_cpu(db->key);
-   riu->sge_cnt = 0;
+   riu->wr.num_sge = 0;
 
/* calculate how many sge required for this remote_buf */
while (rsize > 0 && tsize > 0) {
@@ -1165,27 +1166,29 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch 
*ch,
rsize = 0;
}
 
-   ++riu->sge_cnt;
+   ++riu->wr.num_sge;
 
-   if (rsize > 0 && riu->sge_cnt == SRPT_DEF_SG_PER_WQE) {
+   if (rsize > 0 &&
+   riu->wr.num_sge == SRPT_DEF_SG_PER_WQE) {
++ioctx->n_rdma;
-   riu->sge =
-   kmalloc(riu->sge_cnt * sizeof *riu->sge,
-   GFP_KERNEL);
-   if (!riu->sge)
+   riu->wr.sg_list = kmalloc_array(riu->wr.num_sge,
+   sizeof(*riu->wr.sg_list),
+   GFP_KERNEL);
+   if (!riu->wr.sg_list)
goto free_mem;
 
++riu;
-   riu->sge_cnt = 0;
-   riu->raddr = raddr;
+   riu->wr.num_sge = 0;
+   riu->remote_addr = raddr;
riu->rkey = be32_to_cpu(db->key);
}
}
 
++ioctx->n_rdma;
-   riu->sge = kmalloc(riu->sge_cnt * sizeof *riu->sge,
-  GFP_KERNEL);
-   if (!riu->sge)
+   riu->wr.sg_list = kmalloc_array(riu->wr.num_sge,
+   sizeof(*riu->wr.sg_list),
+   GFP_KERNEL);
+   if (!riu->wr.sg_list)
goto free_mem;
}
 
@@ -1200,7 +1203,7 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
for (i = 0, j = 0;
 j < count && i < ioctx->n_rbuf && tsize > 0; ++i, ++riu, ++db) {
rsize = be32_to_cpu(db->len);
-   sge = riu->sge;
+   sge = riu->wr.sg_list;
k = 0;
 
while (rsize > 0 && tsize > 0) {
@@ -1232,9 +1235,9 @@ static int srpt_map_sg_to_ib_sge(struct srpt_rdma_ch *ch,
}
 
++k;
-   if (k == riu->sge_cnt && rsize > 0 && tsize > 0) {
+   if (k == riu->wr.num_sge && rsize > 0 && tsize > 0) {
++riu;
-