[PATCH RFC 0/2] 8K Inline Support

[PATCH RFC 0/2] 8K Inline Support

[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!

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(-)

-- 
1.8.3.1

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

[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/host/rdma.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 1eb4438..9b8af98 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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);
 	}
 
-- 
1.8.3.1

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

[Parav Pandit]

Hi Steve,

> -----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(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index
> 1eb4438..9b8af98 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -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.

>  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);
>  	}
> 
> --
> 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

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

[Steve Wise]

On 5/9/2018 11:55 AM, Parav Pandit wrote:
> Hi Steve,
>
>> -----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(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index
>> 1eb4438..9b8af98 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -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.

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.

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

[Christoph Hellwig]

> -#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?

>  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)

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

>  	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;
	}
		
> -	if (count == 1) {
> +	if (count <= 2) {

This should be NVME_RDMA_MAX_INLINE_SEGMENTS.

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

[Steve Wise]

On 5/11/2018 1:48 AM, Christoph Hellwig wrote:
>> -#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.

>>  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)
> Just gut feeling, but I'd pass the count argument last.

Yea ok.

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

> 	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;
> 	}
> 		
>> -	if (count == 1) {
>> +	if (count <= 2) {
> This should be NVME_RDMA_MAX_INLINE_SEGMENTS.

Yup.

Thanks for reviewing!

Steve.

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

[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(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 52e0c5d..9e3f08a 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -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];
-- 
1.8.3.1

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

[Max Gurtovoy]

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

On 5/9/2018 5:34 PM, Steve Wise wrote:
> 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(-)
> 
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 52e0c5d..9e3f08a 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -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.

>   
>   struct nvmet_rdma_cmd {
>   	struct ib_sge		sge[2];
> 

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

[Steve Wise]

On 5/14/2018 5:16 AM, Max Gurtovoy wrote:
> Thanks Steve for running this.
> Me and Parav kinda put this task aside...
>

Hey Max,

> On 5/9/2018 5:34 PM, Steve Wise wrote:
>> 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(-)
>>
>> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
>> index 52e0c5d..9e3f08a 100644
>> --- a/drivers/nvme/target/rdma.c
>> +++ b/drivers/nvme/target/rdma.c
>> @@ -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.
>

Yes, I agree.

Thanks,

Steve.

[PATCH RFC 0/2] 8K Inline Support

[Steve Wise]

On 5/9/2018 10:38 AM, Steve Wise wrote:
> 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


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.

[PATCH RFC 0/2] 8K Inline Support

[Christoph Hellwig]

On Wed, May 09, 2018@08:38:05AM -0700, Steve Wise wrote:
> 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.