8K Inline Support

RFC PATCH New Steve Wise 4 participants 11 messages May 9, 2018 – May 14, 2018

Subsystems (2)

NVMe NVMe target

lore

Share & tools

lore

Steve Wise

author

2018-05-09 15:38 UTC #0 lore raw

cover 8K Inline Support

For small nvmf write IO over the rdma transport, it is advantagous to make use of inline mode to avoid the latency of the target issuing an rdma read to fetch the data. Currently inline is used for <= 4K writes. 8K, though, requires the rdma read. For iWARP transports additional latency is incurred because the target mr of the read must be registered with remote write access. By allowing 2 pages worth of inline payload, I see a reduction in 8K nvmf write latency of anywhere from 2-7 usecs depending on the RDMA transport..

Is this a worthwhile change? I think it is. Please comment!

Thanks,

Steve

Steve Wise (2):
nvme-rdma: Support 8K inline
nvmet-rdma: Support 8K inline

 drivers/nvme/host/rdma.c   | 21 +++++++++++++++------
 drivers/nvme/target/rdma.c |  4 ++--
 2 files changed, 17 insertions(+), 8 deletions(-)

Signature · 1 line

1.8.3.1

Steve Wise

author

2018-05-09 14:31 UTC #1 lore raw

RFC PATCH 1/2 nvme-rdma: Support 8K inline

Allow up to 2 pages of inline for NVMF WRITE operations. This reduces latency for 8K WRITEs by removing the need to issue a READ WR for IB, or a REG_MR+READ WR chain for iWarp.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

---
 drivers/nvme/host/rdma.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

drivers/nvme/host/rdma.c +15 -6

index 1eb4438..9b8af98 100644
@@ -40,7 +40,7 @@
 
 #define NVME_RDMA_MAX_SEGMENTS		256
 
-#define NVME_RDMA_MAX_INLINE_SEGMENTS	1
+#define NVME_RDMA_MAX_INLINE_SEGMENTS	2
 
 struct nvme_rdma_device {
 	struct ib_device	*dev;

@@ -1086,19 +1086,28 @@ static int nvme_rdma_set_sg_null(struct nvme_command *c)
 }
 
 static int nvme_rdma_map_sg_inline(struct nvme_rdma_queue *queue,
-		struct nvme_rdma_request *req, struct nvme_command *c)
+		struct nvme_rdma_request *req, int count,
+		struct nvme_command *c)
 {
 	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	u32 len;
 
 	req->sge[1].addr = sg_dma_address(req->sg_table.sgl);
 	req->sge[1].length = sg_dma_len(req->sg_table.sgl);
 	req->sge[1].lkey = queue->device->pd->local_dma_lkey;
+	len = req->sge[1].length;
+	if (count == 2) {
+		req->sge[2].addr = sg_dma_address(req->sg_table.sgl+1);
+		req->sge[2].length = sg_dma_len(req->sg_table.sgl+1);
+		req->sge[2].lkey = queue->device->pd->local_dma_lkey;
+		len += req->sge[2].length;
+	}
 
 	sg->addr = cpu_to_le64(queue->ctrl->ctrl.icdoff);
-	sg->length = cpu_to_le32(sg_dma_len(req->sg_table.sgl));
+	sg->length = cpu_to_le32(len);
 	sg->type = (NVME_SGL_FMT_DATA_DESC << 4) | NVME_SGL_FMT_OFFSET;
 
-	req->num_sge++;
+	req->num_sge += count;
 	return 0;
 }

@@ -1191,13 +1200,13 @@ static int nvme_rdma_map_data(struct nvme_rdma_queue *queue,
 		return -EIO;
 	}
 
-	if (count == 1) {
+	if (count <= 2) {
 		if (rq_data_dir(rq) == WRITE && nvme_rdma_queue_idx(queue) &&
 		    blk_rq_payload_bytes(rq) <=
 				nvme_rdma_inline_data_size(queue))
 			return nvme_rdma_map_sg_inline(queue, req, c);
 
-		if (dev->pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)
+		if (count == 1 && dev->pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)
 			return nvme_rdma_map_sg_single(queue, req, c);
 	}

Signature · 1 line

1.8.3.1

Parav Pandit

2018-05-09 16:55 UTC #2 lore raw

RFC PATCH 1/2 nvme-rdma: Support 8K inline

Hi Steve,

Quoted 23 lines

-----Original Message-----
From: linux-rdma-owner at vger.kernel.org [mailto:linux-rdma-
owner at vger.kernel.org] On Behalf Of Steve Wise
Sent: Wednesday, May 09, 2018 9:31 AM
To: axboe at fb.com; hch at lst.de; keith.busch at intel.com; sagi at grimberg.me; linux-nvme at lists.infradead.org
Cc: linux-rdma at vger.kernel.org
Subject: [PATCH RFC 1/2] nvme-rdma: Support 8K inline
Allow up to 2 pages of inline for NVMF WRITE operations. This reduces latency for 8K WRITEs by removing the need to issue a READ WR for IB, or a REG_MR+READ WR chain for iWarp.
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
drivers/nvme/host/rdma.c index renamed from drivers/nvme/host/rdma.c +0 -0
1eb4438..9b8af98 100644

drivers/nvme/host/rdma.c line 44

Quoted 6 lines

drivers/nvme/host/rdma.c +1 -1

@@ -40,7 +40,7 @@
 
 #define NVME_RDMA_MAX_SEGMENTS		256
 
-#define NVME_RDMA_MAX_INLINE_SEGMENTS	1
+#define NVME_RDMA_MAX_INLINE_SEGMENTS	2

I wrote this patch back in Feb 2017 but didn't spend time on V2 to address comments from Sagi, Christoph.
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008057.html
Thanks for taking this forward.
This is helpful for certain db workload who have 8K typical data size too.

Quoted 61 lines

drivers/nvme/host/rdma.c +13 -4

 struct nvme_rdma_device {
 	struct ib_device	*dev;

@@ -1086,19 +1086,28 @@ static int nvme_rdma_set_sg_null(struct
 nvme_command *c)  }
 
 static int nvme_rdma_map_sg_inline(struct nvme_rdma_queue *queue,
-		struct nvme_rdma_request *req, struct nvme_command *c)
+		struct nvme_rdma_request *req, int count,
+		struct nvme_command *c)
 {
 	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	u32 len;
 
 	req->sge[1].addr = sg_dma_address(req->sg_table.sgl);
 	req->sge[1].length = sg_dma_len(req->sg_table.sgl);
 	req->sge[1].lkey = queue->device->pd->local_dma_lkey;
+	len = req->sge[1].length;
+	if (count == 2) {
+		req->sge[2].addr = sg_dma_address(req->sg_table.sgl+1);
+		req->sge[2].length = sg_dma_len(req->sg_table.sgl+1);
+		req->sge[2].lkey = queue->device->pd->local_dma_lkey;
+		len += req->sge[2].length;
+	}
 
 	sg->addr = cpu_to_le64(queue->ctrl->ctrl.icdoff);
-	sg->length = cpu_to_le32(sg_dma_len(req->sg_table.sgl));
+	sg->length = cpu_to_le32(len);
 	sg->type = (NVME_SGL_FMT_DATA_DESC << 4) |
 NVME_SGL_FMT_OFFSET;
 
-	req->num_sge++;
+	req->num_sge += count;
 	return 0;
 }

@@ -1191,13 +1200,13 @@ static int nvme_rdma_map_data(struct
 nvme_rdma_queue *queue,
 		return -EIO;
 	}
 
-	if (count == 1) {
+	if (count <= 2) {
 		if (rq_data_dir(rq) == WRITE && nvme_rdma_queue_idx(queue)

drivers/nvme/host/rdma.c +1 -1

 		    blk_rq_payload_bytes(rq) <=
 				nvme_rdma_inline_data_size(queue))
 			return nvme_rdma_map_sg_inline(queue, req, c);
 
-		if (dev->pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)
+		if (count == 1 && dev->pd->flags &

IB_PD_UNSAFE_GLOBAL_RKEY)

drivers/nvme/host/rdma.c +0 -1

 			return nvme_rdma_map_sg_single(queue, req, c);
 	}
 
--

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo at vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html

Steve Wise

author

2018-05-09 19:28 UTC #3 lore raw

RFC PATCH 1/2 nvme-rdma: Support 8K inline

Quoted 28 lines · Parav Pandit

Hi Steve,
Quoted 26 lines
-----Original Message-----
From: linux-rdma-owner at vger.kernel.org [mailto:linux-rdma-
owner at vger.kernel.org] On Behalf Of Steve Wise
Sent: Wednesday, May 09, 2018 9:31 AM
To: axboe at fb.com; hch at lst.de; keith.busch at intel.com; sagi at grimberg.me; linux-nvme at lists.infradead.org
Cc: linux-rdma at vger.kernel.org
Subject: [PATCH RFC 1/2] nvme-rdma: Support 8K inline
Allow up to 2 pages of inline for NVMF WRITE operations. This reduces latency for 8K WRITEs by removing the need to issue a READ WR for IB, or a REG_MR+READ WR chain for iWarp.
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
drivers/nvme/host/rdma.c index renamed from drivers/nvme/host/rdma.c +0 -0
1eb4438..9b8af98 100644
drivers/nvme/host/rdma.c +0 -0
@@ -40,7 +40,7 @@
 
 #define NVME_RDMA_MAX_SEGMENTS		256
 

Quoted 7 lines

drivers/nvme/host/rdma.c line 44
Quoted 3 lines
drivers/nvme/host/rdma.c +1 -1
-#define NVME_RDMA_MAX_INLINE_SEGMENTS	1
+#define NVME_RDMA_MAX_INLINE_SEGMENTS	2
 
I wrote this patch back in Feb 2017 but didn't spend time on V2 to address comments from Sagi, Christoph.
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008057.html
Thanks for taking this forward.
This is helpful for certain db workload who have 8K typical data size too.

Hey Parav,

I thought I'd remembered something similar previously. :)? Let me see if I can address the previous comments, going forward.? They are:

- why just 1 or 2 pages vs more?
- dynamic allocation of nvme-rdma resources based on the target's advertised ioccsz.

And I see you posted the nvmet-rdma patch here, which allows up to 16KB inline:

http://lists.infradead.org/pipermail/linux-nvme/2017-February/008064.html

And I think the comments needing resolution are:

- make it a configfs option
- adjust the qp depth some if the inline depth is bigger to try and keep the overall memory footprint reasonable
- avoid high-order allocations - maybe per-core SRQ could be helpful

So the question is, do we have agreement on the way forward?? Sagi and Christoph, I appreciate any feedback on this.? I'd like to get this featured merged.??

Thanks,

Steve.

Christoph Hellwig

maintainer

2018-05-11 06:48 UTC #4 lore raw

RFC PATCH 1/2 nvme-rdma: Support 8K inline

In reply to Steve Wise

drivers/nvme/host/rdma.c line 43

Quoted 2 lines

drivers/nvme/host/rdma.c +1 -1

-#define NVME_RDMA_MAX_INLINE_SEGMENTS	1
+#define NVME_RDMA_MAX_INLINE_SEGMENTS	2

Given how little space we use for just the sge array maybe we want to bump this to 4 once we need to deal with multiple entries?

drivers/nvme/host/rdma.c static int nvme_rdma_set_sg_null(struct nvme_command *c) · line 1090

Quoted 4 lines

static int nvme_rdma_map_sg_inline(struct nvme_rdma_queue *queue,
drivers/nvme/host/rdma.c +2 -1
-		struct nvme_rdma_request *req, struct nvme_command *c)
+		struct nvme_rdma_request *req, int count,
+		struct nvme_command *c)

Just gut feeling, but I'd pass the count argument last.

drivers/nvme/host/rdma.c static int nvme_rdma_set_sg_null(struct nvme_command *c) · line 1104

Quoted 13 lines

drivers/nvme/host/rdma.c +8 -0

 	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	u32 len;
 
 	req->sge[1].addr = sg_dma_address(req->sg_table.sgl);
 	req->sge[1].length = sg_dma_len(req->sg_table.sgl);
 	req->sge[1].lkey = queue->device->pd->local_dma_lkey;
+	len = req->sge[1].length;
+	if (count == 2) {
+		req->sge[2].addr = sg_dma_address(req->sg_table.sgl+1);
+		req->sge[2].length = sg_dma_len(req->sg_table.sgl+1);
+		req->sge[2].lkey = queue->device->pd->local_dma_lkey;
+		len += req->sge[2].length;
+	}

I think this should be turned into a for loop, e.g.

	u32 len, i;

	for (i = 0; i < count; i++) {
		req->sge[1 + i].addr = sg_dma_address(&req->sg_table.sgl[i]);
		req->sge[1 + i].length = sg_dma_len(&req->sg_table.sgl[i]);
		req->sge[1 + i].lkey = queue->device->pd->local_dma_lkey;
		req->num_sge++;
		len += req->sge[i + 1].length;
	}

drivers/nvme/host/rdma.c static int nvme_rdma_map_data(struct nvme_rdma_queue *queue, · line 1203

Quoted 2 lines

drivers/nvme/host/rdma.c +1 -1
-	if (count == 1) {
+	if (count <= 2) {

This should be NVME_RDMA_MAX_INLINE_SEGMENTS.

Steve Wise

author

2018-05-14 18:33 UTC #5 lore raw

RFC PATCH 1/2 nvme-rdma: Support 8K inline

Quoted 4 lines · Christoph Hellwig

drivers/nvme/host/rdma.c line 43
Quoted 2 lines
drivers/nvme/host/rdma.c +1 -1
-#define NVME_RDMA_MAX_INLINE_SEGMENTS	1
+#define NVME_RDMA_MAX_INLINE_SEGMENTS	2
Given how little space we use for just the sge array maybe we want to bump this to 4 once we need to deal with multiple entries?

Agreed.

Quoted 5 lines

drivers/nvme/host/rdma.c static int nvme_rdma_set_sg_null(struct nvme_command *c) · line 1090
Quoted 4 lines
static int nvme_rdma_map_sg_inline(struct nvme_rdma_queue *queue,
drivers/nvme/host/rdma.c +2 -1
-		struct nvme_rdma_request *req, struct nvme_command *c)
+		struct nvme_rdma_request *req, int count,
+		struct nvme_command *c)
Just gut feeling, but I'd pass the count argument last.

Yea ok.

Quoted 14 lines

drivers/nvme/host/rdma.c static int nvme_rdma_set_sg_null(struct nvme_command *c) · line 1104

Quoted 13 lines

drivers/nvme/host/rdma.c +8 -0

 	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	u32 len;
 
 	req->sge[1].addr = sg_dma_address(req->sg_table.sgl);
 	req->sge[1].length = sg_dma_len(req->sg_table.sgl);
 	req->sge[1].lkey = queue->device->pd->local_dma_lkey;
+	len = req->sge[1].length;
+	if (count == 2) {
+		req->sge[2].addr = sg_dma_address(req->sg_table.sgl+1);
+		req->sge[2].length = sg_dma_len(req->sg_table.sgl+1);
+		req->sge[2].lkey = queue->device->pd->local_dma_lkey;
+		len += req->sge[2].length;
+	}

I think this should be turned into a for loop, e.g.

Yes, And I think in the previous incarnation of this patch series, Sagi suggested an iterator pointer vs sge[blah].

Quoted 10 lines

	u32 len, i;

	for (i = 0; i < count; i++) {
		req->sge[1 + i].addr = sg_dma_address(&req->sg_table.sgl[i]);
		req->sge[1 + i].length = sg_dma_len(&req->sg_table.sgl[i]);
		req->sge[1 + i].lkey = queue->device->pd->local_dma_lkey;
		req->num_sge++;
		len += req->sge[i + 1].length;
	}

Quoted 3 lines

drivers/nvme/host/rdma.c static int nvme_rdma_map_data(struct nvme_rdma_queue *queue, · line 1203
Quoted 2 lines
drivers/nvme/host/rdma.c +1 -1
-	if (count == 1) {
+	if (count <= 2) {
This should be NVME_RDMA_MAX_INLINE_SEGMENTS.

Yup.

Thanks for reviewing!

Steve.

Steve Wise

author

2018-05-09 14:34 UTC #6 lore raw

RFC PATCH 2/2 nvmet-rdma: Support 8K inline

In reply to Steve Wise

Allow up to 2 pages of inline for NVMF WRITE operations. This reduces latency for 8K WRITEs by removing the need to issue a READ WR for IB, or a REG_MR+READ WR chain for iWarp.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

---
 drivers/nvme/target/rdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/nvme/target/rdma.c +2 -2

index 52e0c5d..9e3f08a 100644
@@ -33,9 +33,9 @@
 #include "nvmet.h"
 
 /*
- * We allow up to a page of inline data to go with the SQE
+ * We allow up to 2 pages of inline data to go with the SQE
  */
-#define NVMET_RDMA_INLINE_DATA_SIZE	PAGE_SIZE
+#define NVMET_RDMA_INLINE_DATA_SIZE    (PAGE_SIZE << 1)
 
 struct nvmet_rdma_cmd {
 	struct ib_sge		sge[2];

Signature · 1 line

1.8.3.1

Max Gurtovoy

2018-05-14 10:16 UTC #7 lore raw

RFC PATCH 2/2 nvmet-rdma: Support 8K inline

Thanks Steve for running this.
Me and Parav kinda put this task aside...

Quoted 14 lines · Steve Wise

Allow up to 2 pages of inline for NVMF WRITE operations. This reduces latency for 8K WRITEs by removing the need to issue a READ WR for IB, or a REG_MR+READ WR chain for iWarp.
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
  drivers/nvme/target/rdma.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/nvme/target/rdma.c +0 -0
index 52e0c5d..9e3f08a 100644

drivers/nvme/target/rdma.c line 38

Quoted 8 lines

drivers/nvme/target/rdma.c +2 -2

@@ -33,9 +33,9 @@
  #include "nvmet.h"
 
  /*
- * We allow up to a page of inline data to go with the SQE
+ * We allow up to 2 pages of inline data to go with the SQE
   */
-#define NVMET_RDMA_INLINE_DATA_SIZE	PAGE_SIZE
+#define NVMET_RDMA_INLINE_DATA_SIZE    (PAGE_SIZE << 1)

Sometimes 8K != (PAGE_SIZE << 1).
do we realy want to have this in PPC systems, for example, that PAGE_SIZE == 64K ?
We might want to re-think on changing this to SZ_4K.

Quoted 4 lines

drivers/nvme/target/rdma.c +0 -0

  struct nvmet_rdma_cmd {
  	struct ib_sge		sge[2];

Steve Wise

author

2018-05-14 14:58 UTC #8 lore raw

RFC PATCH 2/2 nvmet-rdma: Support 8K inline

Quoted 3 lines · Max Gurtovoy

Thanks Steve for running this.
Me and Parav kinda put this task aside...

Hey Max,

Quoted 19 lines · Steve Wise

Quoted 18 lines
Allow up to 2 pages of inline for NVMF WRITE operations.? This reduces latency for 8K WRITEs by removing the need to issue a READ WR for IB, or a REG_MR+READ WR chain for iWarp.
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
? drivers/nvme/target/rdma.c | 4 ++--
? 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/nvme/target/rdma.c +0 -0
index 52e0c5d..9e3f08a 100644
@@ -33,9 +33,9 @@
? #include "nvmet.h"
? ? /*
- * We allow up to a page of inline data to go with the SQE
+ * We allow up to 2 pages of inline data to go with the SQE

Quoted 9 lines

drivers/nvme/target/rdma.c
Quoted 3 lines
?? */
drivers/nvme/target/rdma.c +1 -1
-#define NVMET_RDMA_INLINE_DATA_SIZE??? PAGE_SIZE
+#define NVMET_RDMA_INLINE_DATA_SIZE??? (PAGE_SIZE << 1)
Sometimes 8K != (PAGE_SIZE << 1).
do we realy want to have this in PPC systems, for example, that PAGE_SIZE == 64K ?
We might want to re-think on changing this to SZ_4K.

Yes, I agree.

Thanks,

Steve.

Steve Wise

author

2018-05-09 18:46 UTC #9 lore raw

cover 8K Inline Support

In reply to Steve Wise

Quoted 9 lines · Steve Wise

For small nvmf write IO over the rdma transport, it is advantagous to make use of inline mode to avoid the latency of the target issuing an rdma read to fetch the data. Currently inline is used for <= 4K writes. 8K, though, requires the rdma read. For iWARP transports additional latency is incurred because the target mr of the read must be registered with remote write access. By allowing 2 pages worth of inline payload, I see a reduction in 8K nvmf write latency of anywhere from 2-7 usecs depending on the RDMA transport..

Quoted 5 lines

Is this a worthwhile change? I think it is. Please comment!
Thanks,
Steve

By the way, this patch series is untested on the current top-of-tree; I ported it from an older 4.9 kernel where I was doing the performance testing.? As the series is RFC, I'm just looking for whether folks think this is a "good idea" or not.?? So I'll post something that is ready to merge if I get positive feedback on moving forward.

Steve.

Christoph Hellwig

maintainer

2018-05-11 06:19 UTC #10 lore raw

cover 8K Inline Support

In reply to Steve Wise

Quoted 10 lines · Steve Wise

For small nvmf write IO over the rdma transport, it is advantagous to make use of inline mode to avoid the latency of the target issuing an rdma read to fetch the data. Currently inline is used for <= 4K writes. 8K, though, requires the rdma read. For iWARP transports additional latency is incurred because the target mr of the read must be registered with remote write access. By allowing 2 pages worth of inline payload, I see a reduction in 8K nvmf write latency of anywhere from 2-7 usecs depending on the RDMA transport..
Is this a worthwhile change? I think it is. Please comment!

I think this came up before. I'm fine with supporting larger sizes, but I really want the size configurable. We can argue about the default separately.