Skip to contentSkip to conversation

NVMF/RDMA 16K Inline Support

PATCH Superseded Steve Wise 5 participants 20 messages Jun 5, 2018 – Jun 20, 2018

Subsystems (2)

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Steve Wise

author
#0 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Hey,

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

This series is a respin of a series floated last year by Parav and Max [1]. I'm continuing it now and have addressed some of the comments from their submission [2].

Changes since v3:

- nvme-rdma: remove pr_debug.

- nvme-rdma: add Sagi's reviewed-by tag.

- nvmet-rdma: avoid > order 0 page allocations for inline data bufffers by using multiple sges. If the device cannot support the required sge depth then reduce the inline data size to fit.

- nvmet-rdma: set max_recv_sge correctly

- nvmet-rdma: if the configured inline data size exceeds the max supported by the nvmet-rdma, a warning is logged and the size is reduced.

Changes since RFC v2:

- Removed RFC tag

- prefix the inline_data_size configfs attribute with param_

- implementation/formatting tweaks suggested by Christoph

- support inline_data_size of 0, which disables inline data use

- added a new patch to fix the check for keyed sgls (bit 2 instead of 20).

- check the inline_data bit (bit 20 in the ctrl.sgls field) when connecting and only use inline if it was set for that device.

- added Christoph's review-by tag for patch 1

[1] Original submissions:
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008057.html
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008059.html

[2] These comments from [1] have been addressed:

- nvme-rdma: Support up to 4 segments of inline data.

- nvme-rdma: Cap the number of inline segments to not exceed device limitations.

- nvmet-rdma: Make the inline data size configurable in nvmet-rdma via configfs.

- nvmet-rdma: avoid > 0 order page allocations

Other issues from [1] that I don't plan to incorporate into the series:

- nvme-rdma: make the sge array for inline segments dynamic based on the target's advertised inline_data_size. Since we're limiting the max count to 4, I'm not sure this is worth the complexity of allocating the sge array vs just embedding the max.

- nvmet-rdma: reduce the qp depth if the inline size greatly increases the memory footprint. I'm not sure how to do this in a reasonable mannor. Since the inline data size is now configurable, do we still need this?

- nvmet-rdma: make the qp depth configurable so the admin can reduce it manually to lower the memory footprint.

Steve Wise (3):
nvme-rdma: correctly check for target keyed sgl support
nvme-rdma: support up to 4 segments of inline data
nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

 drivers/nvme/host/rdma.c        |  43 +++++++---
 drivers/nvme/target/admin-cmd.c |   4 +-
 drivers/nvme/target/configfs.c  |  31 +++++++
 drivers/nvme/target/core.c      |   4 +
 drivers/nvme/target/discovery.c |   2 +-
 drivers/nvme/target/nvmet.h     |   2 +-
 drivers/nvme/target/rdma.c      | 174 ++++++++++++++++++++++++++++++----------
 7 files changed, 202 insertions(+), 58 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH v4 1/3 nvme-rdma: correctly check for target keyed sgl support

The code was checking bit 20 instead of bit 2. Also fixed the log entry.

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/nvme/host/rdma.c +3 -2
index 1eb4438..d9f5fbe 100644
@@ -1949,8 +1949,9 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	}
 
 	/* sanity check keyed sgls */
-	if (!(ctrl->ctrl.sgls & (1 << 20))) {
-		dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not support\n");
+	if (!(ctrl->ctrl.sgls & (1 << 2))) {
+		dev_err(ctrl->ctrl.device,
+			"Mandatory keyed sgls are not supported!\n");
 		ret = -EINVAL;
 		goto out_remove_admin_queue;
 	}
Signature · 1 line
1.8.3.1
#2 lore raw

PATCH v4 1/3 nvme-rdma: correctly check for target keyed sgl support

Quoted 4 lines · Steve Wise

The code was checking bit 20 instead of bit 2. Also fixed the log entry.

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
Signed-off-by Steve Wise <swise at opengridcomputing.com>

Thanks, added to nvme-4.18.

author
#3 lore raw

PATCH v4 2/3 nvme-rdma: support up to 4 segments of inline data

In reply to Steve Wise

Allow up to 4 segments of inline data for NVMF WRITE operations. This reduces latency for small WRITEs by removing the need for the target to issue a READ WR for IB, or a REG_MR + READ WR chain for iWarp.

Also cap the inline segments used based on the limitations of the device.

Reviewed-by Christoph Hellwig <hch at lst.de>
Reviewed-by Sagi Grimberg <sagi at grimberg.me>
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)
drivers/nvme/host/rdma.c +27 -11
index d9f5fbe..622b13b 100644
@@ -40,13 +40,14 @@
 
 #define NVME_RDMA_MAX_SEGMENTS		256
 
-#define NVME_RDMA_MAX_INLINE_SEGMENTS	1
+#define NVME_RDMA_MAX_INLINE_SEGMENTS	4
 
 struct nvme_rdma_device {
 	struct ib_device	*dev;
 	struct ib_pd		*pd;
 	struct kref		ref;
 	struct list_head	entry;
+	unsigned int		num_inline_segments;
 };
 
 struct nvme_rdma_qe {
@@ -117,6 +118,7 @@ struct nvme_rdma_ctrl {
 	struct sockaddr_storage src_addr;
 
 	struct nvme_ctrl	ctrl;
+	bool			use_inline_data;
 };
 
 static inline struct nvme_rdma_ctrl *to_rdma_ctrl(struct nvme_ctrl *ctrl)
@@ -249,7 +251,7 @@ static int nvme_rdma_create_qp(struct nvme_rdma_queue *queue, const int factor)
 	/* +1 for drain */
 	init_attr.cap.max_recv_wr = queue->queue_size + 1;
 	init_attr.cap.max_recv_sge = 1;
-	init_attr.cap.max_send_sge = 1 + NVME_RDMA_MAX_INLINE_SEGMENTS;
+	init_attr.cap.max_send_sge = 1 + dev->num_inline_segments;
 	init_attr.sq_sig_type = IB_SIGNAL_REQ_WR;
 	init_attr.qp_type = IB_QPT_RC;
 	init_attr.send_cq = queue->ib_cq;
@@ -374,6 +376,8 @@ static int nvme_rdma_dev_get(struct nvme_rdma_device *dev)
 		goto out_free_pd;
 	}
 
+	ndev->num_inline_segments = min(NVME_RDMA_MAX_INLINE_SEGMENTS,
+					ndev->dev->attrs.max_sge - 1);
 	list_add(&ndev->entry, &device_list);
 out_unlock:
 	mutex_unlock(&device_list_mutex);
@@ -1086,19 +1090,27 @@ 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, struct nvme_command *c,
+		int count)
 {
 	struct nvme_sgl_desc *sg = &c->common.dptr.sgl;
+	struct scatterlist *sgl = req->sg_table.sgl;
+	struct ib_sge *sge = &req->sge[1];
+	u32 len = 0;
+	int i;
 
-	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;
+	for (i = 0; i < count; i++, sgl++, sge++) {
+		sge->addr = sg_dma_address(sgl);
+		sge->length = sg_dma_len(sgl);
+		sge->lkey = queue->device->pd->local_dma_lkey;
+		len += sge->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 +1203,14 @@ static int nvme_rdma_map_data(struct nvme_rdma_queue *queue,
 		return -EIO;
 	}
 
-	if (count == 1) {
+	if (count <= dev->num_inline_segments) {
 		if (rq_data_dir(rq) == WRITE && nvme_rdma_queue_idx(queue) &&
+		    queue->ctrl->use_inline_data &&
 		    blk_rq_payload_bytes(rq) <=
 				nvme_rdma_inline_data_size(queue))
-			return nvme_rdma_map_sg_inline(queue, req, c);
+			return nvme_rdma_map_sg_inline(queue, req, c, count);
 
-		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);
 	}
 
@@ -1956,6 +1969,9 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		goto out_remove_admin_queue;
 	}
 
+	if (ctrl->ctrl.sgls & (1 << 20))
+		ctrl->use_inline_data = true;
+
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
 		dev_warn(ctrl->ctrl.device,
Signature · 1 line
1.8.3.1
author
#4 lore raw

PATCH v4 3/3 nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

In reply to Steve Wise

The patch enables inline data sizes using up to 4 recv sges, and capping the size at 16KB or at least 1 page size. So on a 4K page system, up to 16KB is supported, and for a 64K page system 1 page of 64KB is supported.

We avoid > 0 order page allocations for the inline buffers by using multiple recv sges, one for each page. If the device cannot support the configured inline data size due to lack of enough recv sges, then log a warning and reduce the inline size.

Add a new configfs port attribute, called param_inline_data_size, to allow configuring the size of inline data for a given nvmf port. The maximum size allowed is still enforced by nvmet-rdma with NVMET_RDMA_MAX_INLINE_DATA_SIZE, which is now max(16KB, PAGE_SIZE). And the default size, if not specified via configfs, is still PAGE_SIZE. This preserves the existing behavior, but allows larger inline sizes for small page systems. If the configured inline data size exceeds NVMET_RDMA_MAX_INLINE_DATA_SIZE, a warning is logged and the size is reduced. If param_inline_data_size is set to 0, then inline data is disabled for that nvmf port.

Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/target/admin-cmd.c |   4 +-
 drivers/nvme/target/configfs.c  |  31 +++++++
 drivers/nvme/target/core.c      |   4 +
 drivers/nvme/target/discovery.c |   2 +-
 drivers/nvme/target/nvmet.h     |   2 +-
 drivers/nvme/target/rdma.c      | 174 ++++++++++++++++++++++++++++++----------
 6 files changed, 172 insertions(+), 45 deletions(-)
drivers/nvme/target/admin-cmd.c +2 -2
index 5e0e9fc..a9e3223 100644
@@ -247,14 +247,14 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
 	if (ctrl->ops->has_keyed_sgls)
 		id->sgls |= cpu_to_le32(1 << 2);
-	if (ctrl->ops->sqe_inline_size)
+	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
 
 	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
 
 	/* Max command capsule size is sqe + single page of in-capsule data */
 	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
-				  ctrl->ops->sqe_inline_size) / 16);
+				  req->port->inline_data_size) / 16);
 	/* Max response capsule size is cqe */
 	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
 
drivers/nvme/target/configfs.c +31 -0
index ad9ff27..9867783 100644
@@ -214,6 +214,35 @@ static ssize_t nvmet_addr_trsvcid_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_, addr_trsvcid);
 
+static ssize_t nvmet_param_inline_data_size_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+
+	return snprintf(page, PAGE_SIZE, "%d\n", port->inline_data_size);
+}
+
+static ssize_t nvmet_param_inline_data_size_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+	int ret;
+
+	if (port->enabled) {
+		pr_err("Cannot modify inline_data_size enabled\n");
+		pr_err("Disable the port before modifying\n");
+		return -EACCES;
+	}
+	ret = kstrtoint(page, 0, &port->inline_data_size);
+	if (ret) {
+		pr_err("Invalid value '%s' for inline_data_size\n", page);
+		return -EINVAL;
+	}
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_, param_inline_data_size);
+
 static ssize_t nvmet_addr_trtype_show(struct config_item *item,
 		char *page)
 {
@@ -870,6 +899,7 @@ static void nvmet_port_release(struct config_item *item)
 	&nvmet_attr_addr_traddr,
 	&nvmet_attr_addr_trsvcid,
 	&nvmet_attr_addr_trtype,
+	&nvmet_attr_param_inline_data_size,
 	NULL,
 };
 
@@ -899,6 +929,7 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
 	INIT_LIST_HEAD(&port->entry);
 	INIT_LIST_HEAD(&port->subsystems);
 	INIT_LIST_HEAD(&port->referrals);
+	port->inline_data_size = -1;	/* < 0 == let the transport choose */
 
 	port->disc_addr.portid = cpu_to_le16(portid);
 	config_group_init_type_name(&port->group, name, &nvmet_port_type);
drivers/nvme/target/core.c +4 -0
index e95424f..695ec17 100644
@@ -189,6 +189,10 @@ int nvmet_enable_port(struct nvmet_port *port)
 		return ret;
 	}
 
+	/* If the transport didn't set inline_data_size, then disable it. */
+	if (port->inline_data_size < 0)
+		port->inline_data_size = 0;
+
 	port->enabled = true;
 	return 0;
 }
drivers/nvme/target/discovery.c +1 -1
index 231e04e..fc2e675 100644
@@ -171,7 +171,7 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
 	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
 	if (ctrl->ops->has_keyed_sgls)
 		id->sgls |= cpu_to_le32(1 << 2);
-	if (ctrl->ops->sqe_inline_size)
+	if (req->port->inline_data_size)
 		id->sgls |= cpu_to_le32(1 << 20);
 
 	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
drivers/nvme/target/nvmet.h +1 -1
index 15fd84a..db29e45 100644
@@ -98,6 +98,7 @@ struct nvmet_port {
 	struct list_head		referrals;
 	void				*priv;
 	bool				enabled;
+	int				inline_data_size;
 };
 
 static inline struct nvmet_port *to_nvmet_port(struct config_item *item)
@@ -202,7 +203,6 @@ struct nvmet_subsys_link {
 struct nvmet_fabrics_ops {
 	struct module *owner;
 	unsigned int type;
-	unsigned int sqe_inline_size;
 	unsigned int msdbd;
 	bool has_keyed_sgls : 1;
 	void (*queue_response)(struct nvmet_req *req);
drivers/nvme/target/rdma.c +133 -41

284 lines not on this page · raw message

Signature · 1 line
1.8.3.1
maintainer
#5 lore raw

PATCH v4 3/3 nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

Quoted 100 lines · Steve Wise

The patch enables inline data sizes using up to 4 recv sges, and capping the size at 16KB or at least 1 page size. So on a 4K page system, up to 16KB is supported, and for a 64K page system 1 page of 64KB is supported.

We avoid > 0 order page allocations for the inline buffers by using multiple recv sges, one for each page. If the device cannot support the configured inline data size due to lack of enough recv sges, then log a warning and reduce the inline size.

Add a new configfs port attribute, called param_inline_data_size, to allow configuring the size of inline data for a given nvmf port. The maximum size allowed is still enforced by nvmet-rdma with NVMET_RDMA_MAX_INLINE_DATA_SIZE, which is now max(16KB, PAGE_SIZE). And the default size, if not specified via configfs, is still PAGE_SIZE. This preserves the existing behavior, but allows larger inline sizes for small page systems. If the configured inline data size exceeds NVMET_RDMA_MAX_INLINE_DATA_SIZE, a warning is logged and the size is reduced. If param_inline_data_size is set to 0, then inline data is disabled for that nvmf port.

Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
  drivers/nvme/target/admin-cmd.c |   4 +-
  drivers/nvme/target/configfs.c  |  31 +++++++
  drivers/nvme/target/core.c      |   4 +
  drivers/nvme/target/discovery.c |   2 +-
  drivers/nvme/target/nvmet.h     |   2 +-
  drivers/nvme/target/rdma.c      | 174 ++++++++++++++++++++++++++++++----------
  6 files changed, 172 insertions(+), 45 deletions(-)
drivers/nvme/target/admin-cmd.c +2 -2
index 5e0e9fc..a9e3223 100644
@@ -247,14 +247,14 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
  	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
  	if (ctrl->ops->has_keyed_sgls)
  		id->sgls |= cpu_to_le32(1 << 2);
-	if (ctrl->ops->sqe_inline_size)
+	if (req->port->inline_data_size)
  		id->sgls |= cpu_to_le32(1 << 20);
 
  	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
 
  	/* Max command capsule size is sqe + single page of in-capsule data */
  	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
-				  ctrl->ops->sqe_inline_size) / 16);
+				  req->port->inline_data_size) / 16);
  	/* Max response capsule size is cqe */
  	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
 
drivers/nvme/target/configfs.c +30 -0
index ad9ff27..9867783 100644
@@ -214,6 +214,35 @@ static ssize_t nvmet_addr_trsvcid_store(struct config_item *item,
 
  CONFIGFS_ATTR(nvmet_, addr_trsvcid);
 
+static ssize_t nvmet_param_inline_data_size_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+
+	return snprintf(page, PAGE_SIZE, "%d\n", port->inline_data_size);
+}
+
+static ssize_t nvmet_param_inline_data_size_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+	int ret;
+
+	if (port->enabled) {
+		pr_err("Cannot modify inline_data_size enabled\n");
+		pr_err("Disable the port before modifying\n");
+		return -EACCES;
+	}
+	ret = kstrtoint(page, 0, &port->inline_data_size);
+	if (ret) {
+		pr_err("Invalid value '%s' for inline_data_size\n", page);
+		return -EINVAL;
+	}
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_, param_inline_data_size);
+
  static ssize_t nvmet_addr_trtype_show(struct config_item *item,
  		char *page)
  {
@@ -870,6 +899,7 @@ static void nvmet_port_release(struct config_item *item)
  	&nvmet_attr_addr_traddr,
  	&nvmet_attr_addr_trsvcid,
  	&nvmet_attr_addr_trtype,
+	&nvmet_attr_param_inline_data_size,
  	NULL,
  };
 

drivers/nvme/target/configfs.c static struct config_group *nvmet_ports_make(struct config_group *group, · line 932

Quoted 4 lines
drivers/nvme/target/configfs.c +1 -0
@@ -899,6 +929,7 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
  	INIT_LIST_HEAD(&port->entry);
  	INIT_LIST_HEAD(&port->subsystems);
  	INIT_LIST_HEAD(&port->referrals);
+	port->inline_data_size = -1;	/* < 0 == let the transport choose */

Why not init to 0?

Quoted 88 lines
drivers/nvme/target/configfs.c +0 -0
  	port->disc_addr.portid = cpu_to_le16(portid);
  	config_group_init_type_name(&port->group, name, &nvmet_port_type);
drivers/nvme/target/core.c +4 -0
index e95424f..695ec17 100644
@@ -189,6 +189,10 @@ int nvmet_enable_port(struct nvmet_port *port)
  		return ret;
  	}
 
+	/* If the transport didn't set inline_data_size, then disable it. */
+	if (port->inline_data_size < 0)
+		port->inline_data_size = 0;
+
  	port->enabled = true;
  	return 0;
  }
drivers/nvme/target/discovery.c +1 -1
index 231e04e..fc2e675 100644
@@ -171,7 +171,7 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
  	id->sgls = cpu_to_le32(1 << 0);	/* we always support SGLs */
  	if (ctrl->ops->has_keyed_sgls)
  		id->sgls |= cpu_to_le32(1 << 2);
-	if (ctrl->ops->sqe_inline_size)
+	if (req->port->inline_data_size)
  		id->sgls |= cpu_to_le32(1 << 20);
 
  	strcpy(id->subnqn, ctrl->subsys->subsysnqn);
drivers/nvme/target/nvmet.h +1 -1
index 15fd84a..db29e45 100644
@@ -98,6 +98,7 @@ struct nvmet_port {
  	struct list_head		referrals;
  	void				*priv;
  	bool				enabled;
+	int				inline_data_size;
  };
 
  static inline struct nvmet_port *to_nvmet_port(struct config_item *item)
@@ -202,7 +203,6 @@ struct nvmet_subsys_link {
  struct nvmet_fabrics_ops {
  	struct module *owner;
  	unsigned int type;
-	unsigned int sqe_inline_size;
  	unsigned int msdbd;
  	bool has_keyed_sgls : 1;
  	void (*queue_response)(struct nvmet_req *req);
drivers/nvme/target/rdma.c +8 -5
index 52e0c5d..eb5f1b0 100644
@@ -33,16 +33,17 @@
  #include "nvmet.h"
 
  /*
- * We allow up to a page of inline data to go with the SQE
+ * We allow at least 1 page, up to 4 SGEs, and up to 16KB of inline data
   */
-#define NVMET_RDMA_INLINE_DATA_SIZE	PAGE_SIZE
+#define NVMET_RDMA_DEFAULT_INLINE_DATA_SIZE	PAGE_SIZE
+#define NVMET_RDMA_MAX_INLINE_SGE		4
+#define NVMET_RDMA_MAX_INLINE_DATA_SIZE		max_t(int, SZ_16K, PAGE_SIZE)
 
  struct nvmet_rdma_cmd {
-	struct ib_sge		sge[2];
+	struct ib_sge		sge[NVMET_RDMA_MAX_INLINE_SGE + 1];
  	struct ib_cqe		cqe;
  	struct ib_recv_wr	wr;
-	struct scatterlist	inline_sg;
-	struct page		*inline_page;
+	struct scatterlist	inline_sg[NVMET_RDMA_MAX_INLINE_SGE];
  	struct nvme_command     *nvme_cmd;
  	struct nvmet_rdma_queue	*queue;
  };
@@ -116,6 +117,8 @@ struct nvmet_rdma_device {
  	size_t			srq_size;
  	struct kref		ref;
  	struct list_head	entry;
+	int			inline_data_size;
+	int			inline_page_count;
  };
 
  static bool nvmet_rdma_use_srq;

drivers/nvme/target/rdma.c struct nvmet_rdma_device { · line 147

Quoted 7 lines
drivers/nvme/target/rdma.c +4 -0
@@ -138,6 +141,11 @@ struct nvmet_rdma_device {
 
  static const struct nvmet_fabrics_ops nvmet_rdma_ops;
 
+static int num_pages(int len)
+{
+	return 1 + (((len - 1) & PAGE_MASK) >> PAGE_SHIFT);
+}

get_order()?

author
#6 lore raw

PATCH v4 3/3 nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

Quoted 107 lines · Sagi Grimberg
Quoted 104 lines · Steve Wise

The patch enables inline data sizes using up to 4 recv sges, and capping the size at 16KB or at least 1 page size.? So on a 4K page system, up to 16KB is supported, and for a 64K page system 1 page of 64KB is supported.

We avoid > 0 order page allocations for the inline buffers by using multiple recv sges, one for each page.? If the device cannot support the configured inline data size due to lack of enough recv sges, then log a warning and reduce the inline size.

Add a new configfs port attribute, called param_inline_data_size, to allow configuring the size of inline data for a given nvmf port. The maximum size allowed is still enforced by nvmet-rdma with NVMET_RDMA_MAX_INLINE_DATA_SIZE, which is now max(16KB, PAGE_SIZE). And the default size, if not specified via configfs, is still PAGE_SIZE. This preserves the existing behavior, but allows larger inline sizes for small page systems.? If the configured inline data size exceeds NVMET_RDMA_MAX_INLINE_DATA_SIZE, a warning is logged and the size is reduced.? If param_inline_data_size is set to 0, then inline data is disabled for that nvmf port.

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

---

? drivers/nvme/target/admin-cmd.c |?? 4 +-
? drivers/nvme/target/configfs.c? |? 31 +++++++
? drivers/nvme/target/core.c????? |?? 4 +
? drivers/nvme/target/discovery.c |?? 2 +-
? drivers/nvme/target/nvmet.h???? |?? 2 +-
? drivers/nvme/target/rdma.c????? | 174

++++++++++++++++++++++++++++++----------
? 6 files changed, 172 insertions(+), 45 deletions(-)

drivers/nvme/target/admin-cmd.c +0 -0
index 5e0e9fc..a9e3223 100644
@@ -247,14 +247,14 @@ static void nvmet_execute_identify_ctrl(struct
 nvmet_req *req)

????? id->sgls = cpu_to_le32(1 << 0);??? /* we always support SGLs */ ????? if (ctrl->ops->has_keyed_sgls)
????????? id->sgls |= cpu_to_le32(1 << 2);
-??? if (ctrl->ops->sqe_inline_size)
+??? if (req->port->inline_data_size)
????????? id->sgls |= cpu_to_le32(1 << 20);
? ????? strcpy(id->subnqn, ctrl->subsys->subsysnqn);
? ????? /* Max command capsule size is sqe + single page of in-capsule data */
????? id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) + -????????????????? ctrl->ops->sqe_inline_size) / 16); +????????????????? req->port->inline_data_size) / 16); ????? /* Max response capsule size is cqe */
????? id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16); ? diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index ad9ff27..9867783 100644

drivers/nvme/target/configfs.c +0 -0
@@ -214,6 +214,35 @@ static ssize_t nvmet_addr_trsvcid_store(struct
 config_item *item,
 ? ? CONFIGFS_ATTR(nvmet_, addr_trsvcid);

? +static ssize_t nvmet_param_inline_data_size_show(struct config_item *item,
+??????? char *page)

drivers/nvme/target/configfs.c +7 -0
+{
+??? struct nvmet_port *port = to_nvmet_port(item);
+
+??? return snprintf(page, PAGE_SIZE, "%d\n", port->inline_data_size);
+}
+
+static ssize_t nvmet_param_inline_data_size_store(struct config_item

*item,
+??????? const char *page, size_t count)

drivers/nvme/target/configfs.c +19 -0
+{
+??? struct nvmet_port *port = to_nvmet_port(item);
+??? int ret;
+
+??? if (port->enabled) {
+??????? pr_err("Cannot modify inline_data_size enabled\n");
+??????? pr_err("Disable the port before modifying\n");
+??????? return -EACCES;
+??? }
+??? ret = kstrtoint(page, 0, &port->inline_data_size);
+??? if (ret) {
+??????? pr_err("Invalid value '%s' for inline_data_size\n", page);
+??????? return -EINVAL;
+??? }
+??? return count;
+}
+
+CONFIGFS_ATTR(nvmet_, param_inline_data_size);
+

? static ssize_t nvmet_addr_trtype_show(struct config_item *item, ????????? char *page)
? {

drivers/nvme/target/configfs.c +1 -0
@@ -870,6 +899,7 @@ static void nvmet_port_release(struct config_item
 *item)
 ????? &nvmet_attr_addr_traddr,
 ????? &nvmet_attr_addr_trsvcid,
 ????? &nvmet_attr_addr_trtype,
+??? &nvmet_attr_param_inline_data_size,
 ????? NULL,
 ? };

? @@ -899,6 +929,7 @@ static struct config_group

Quoted 9 lines
Quoted 6 lines

*nvmet_ports_make(struct config_group *group,
????? INIT_LIST_HEAD(&port->entry);
????? INIT_LIST_HEAD(&port->subsystems);
????? INIT_LIST_HEAD(&port->referrals);
+??? port->inline_data_size = -1;??? /* < 0 == let the transport choose */

Why not init to 0?

-1 means "transport chooses the size"
0 means "no inline data"

Quoted 86 lines
Quoted 86 lines

? ????? port->disc_addr.portid = cpu_to_le16(portid);
????? config_group_init_type_name(&port->group, name, &nvmet_port_type);

drivers/nvme/target/core.c +0 -0
index e95424f..695ec17 100644
@@ -189,6 +189,10 @@ int nvmet_enable_port(struct nvmet_port *port)
 ????????? return ret;
 ????? }

? +??? /* If the transport didn't set inline_data_size, then disable it. */
+??? if (port->inline_data_size < 0)
+??????? port->inline_data_size = 0;
+
????? port->enabled = true;
????? return 0;
? }

drivers/nvme/target/discovery.c +0 -0
index 231e04e..fc2e675 100644
@@ -171,7 +171,7 @@ static void
 nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)

????? id->sgls = cpu_to_le32(1 << 0);??? /* we always support SGLs */ ????? if (ctrl->ops->has_keyed_sgls)
????????? id->sgls |= cpu_to_le32(1 << 2);
-??? if (ctrl->ops->sqe_inline_size)
+??? if (req->port->inline_data_size)
????????? id->sgls |= cpu_to_le32(1 << 20);
? ????? strcpy(id->subnqn, ctrl->subsys->subsysnqn);

drivers/nvme/target/nvmet.h +1 -0
index 15fd84a..db29e45 100644
@@ -98,6 +98,7 @@ struct nvmet_port {
 ????? struct list_head??????? referrals;
 ????? void??????????????? *priv;
 ????? bool??????????????? enabled;
+??? int??????????????? inline_data_size;
 ? };

? ? static inline struct nvmet_port *to_nvmet_port(struct config_item *item)

drivers/nvme/target/nvmet.h +0 -1
@@ -202,7 +203,6 @@ struct nvmet_subsys_link {
 ? struct nvmet_fabrics_ops {
 ????? struct module *owner;
 ????? unsigned int type;
-??? unsigned int sqe_inline_size;
 ????? unsigned int msdbd;
 ????? bool has_keyed_sgls : 1;
 ????? void (*queue_response)(struct nvmet_req *req);
drivers/nvme/target/rdma.c +0 -0
index 52e0c5d..eb5f1b0 100644
@@ -33,16 +33,17 @@

? #include "nvmet.h"
? ? /*
- * We allow up to a page of inline data to go with the SQE
+ * We allow at least 1 page, up to 4 SGEs, and up to 16KB of inline data
?? */

drivers/nvme/target/rdma.c +3 -1
-#define NVMET_RDMA_INLINE_DATA_SIZE??? PAGE_SIZE
+#define NVMET_RDMA_DEFAULT_INLINE_DATA_SIZE??? PAGE_SIZE
+#define NVMET_RDMA_MAX_INLINE_SGE??????? 4
+#define NVMET_RDMA_MAX_INLINE_DATA_SIZE??????? max_t(int, SZ_16K,

PAGE_SIZE)
? ? struct nvmet_rdma_cmd {
-??? struct ib_sge??????? sge[2];
+??? struct ib_sge??????? sge[NVMET_RDMA_MAX_INLINE_SGE + 1]; ????? struct ib_cqe??????? cqe;
????? struct ib_recv_wr??? wr;
-??? struct scatterlist??? inline_sg;
-??? struct page??????? *inline_page;
+??? struct scatterlist??? inline_sg[NVMET_RDMA_MAX_INLINE_SGE]; ????? struct nvme_command???? *nvme_cmd;
????? struct nvmet_rdma_queue??? *queue;
? };

drivers/nvme/target/rdma.c +2 -0
@@ -116,6 +117,8 @@ struct nvmet_rdma_device {
 ????? size_t??????????? srq_size;
 ????? struct kref??????? ref;
 ????? struct list_head??? entry;
+??? int??????????? inline_data_size;
+??? int??????????? inline_page_count;
 ? };
 ? ? static bool nvmet_rdma_use_srq;
Quoted 9 lines

drivers/nvme/target/rdma.c struct nvmet_rdma_device { · line 145

Quoted 6 lines
drivers/nvme/target/rdma.c +3 -0
@@ -138,6 +141,11 @@ struct nvmet_rdma_device {
 ? ? static const struct nvmet_fabrics_ops nvmet_rdma_ops;
 ? +static int num_pages(int len)
+{
+??? return 1 + (((len - 1) & PAGE_MASK) >> PAGE_SHIFT);
+}

get_order()?

We don't want the order, like 1, 2, 4, 8.? We want, for example, 12KB to

be 3 pages, not 4.

Steve.

maintainer
#7 lore raw

PATCH v4 3/3 nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

In reply to Steve Wise
Quoted 2 lines · Steve Wise

The patch enables inline data sizes using up to 4 recv sges, and capping the size at 16KB or at least 1 page size.

Question: any reason for that cap? Just seems like an arbitrary limit...

So on a 4K page system, up to

Quoted 1 line

16KB is supported, and for a 64K page system 1 page of 64KB is supported.

Well if someone asked for 16K and got 64K its a bit of a surprise isn't it? without exposing knob for this, using 64K OK I guess, but when we expose controls for this its a bit surprising.

Would page_frags work better here? (page_frag_alloc/page_frag_free) Given that most likely the backend device will work with 4K pages, the fragments won't cause gaps...

Thoughts?

...

drivers/nvme/target/rdma.c struct nvmet_rdma_device { · line 147

Quoted 4 lines
drivers/nvme/target/rdma.c +4 -0
+static int num_pages(int len)
+{
+	return 1 + (((len - 1) & PAGE_MASK) >> PAGE_SHIFT);
+}

Steve, can you explain why is this needed? why isn't get_order() sufficient?

author
#8 lore raw

PATCH v4 3/3 nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

Quoted 8 lines · Sagi Grimberg
Quoted 2 lines · Steve Wise

The patch enables inline data sizes using up to 4 recv sges, and capping the size at 16KB or at least 1 page size.

Question: any reason for that cap? Just seems like an arbitrary limit...

It was there in the original patch series, and I continued it.? I guess the idea is we don't want to be a memory hog.

Quoted 8 lines

? So on a 4K page system, up to

Quoted 2 lines

16KB is supported, and for a 64K page system 1 page of 64KB is supported.

Well if someone asked for 16K and got 64K its a bit of a surprise isn't it? without exposing knob for this, using 64K OK I guess, but when we expose controls for this its a bit surprising.

I'm open to proposals for a better way to do all this.? Like perhaps just a knob for how many pages to allow?

Quoted 4 lines

Would page_frags work better here? (page_frag_alloc/page_frag_free) Given that most likely the backend device will work with 4K pages, the fragments won't cause gaps...

There's no comments on this API.? How does it work?? It allocates some number of contiguous fragments < a page?

Quoted 5 lines

Thoughts?

...

Quoted 8 lines

drivers/nvme/target/rdma.c struct nvmet_rdma_device { · line 147

Quoted 4 lines
drivers/nvme/target/rdma.c +4 -0
+static int num_pages(int len)
+{
+??? return 1 + (((len - 1) & PAGE_MASK) >> PAGE_SHIFT);
+}

Steve, can you explain why is this needed? why isn't get_order() sufficient?

I thought get_order() gives you a power of two >= the length. ie 1, 2, 4, 8, 16.? For inline_data length of 12KB, for example, we want 3 pages, not 4.? Or am I mistaken?

Steve.

author
#9 lore raw

PATCH v4 3/3 nvmet-rdma: support max(16KB, PAGE_SIZE) inline data

Quoted 40 lines · Steve Wise
Quoted 6 lines · Sagi Grimberg
Quoted 2 lines · Steve Wise

The patch enables inline data sizes using up to 4 recv sges, and capping the size at 16KB or at least 1 page size.

Question: any reason for that cap? Just seems like an arbitrary limit...

It was there in the original patch series, and I continued it.? I guess the idea is we don't want to be a memory hog.

Quoted 7 lines

? So on a 4K page system, up to

Quoted 2 lines

16KB is supported, and for a 64K page system 1 page of 64KB is supported.

Well if someone asked for 16K and got 64K its a bit of a surprise isn't it? without exposing knob for this, using 64K OK I guess, but when we expose controls for this its a bit surprising.

I'm open to proposals for a better way to do all this.? Like perhaps just a knob for how many pages to allow?

Quoted 4 lines

Would page_frags work better here? (page_frag_alloc/page_frag_free) Given that most likely the backend device will work with 4K pages, the fragments won't cause gaps...

There's no comments on this API.? How does it work?? It allocates some number of contiguous fragments < a page?

Quoted 5 lines

Thoughts?

...

Quoted 7 lines

drivers/nvme/target/rdma.c struct nvmet_rdma_device { · line 147

Quoted 4 lines
drivers/nvme/target/rdma.c +4 -0
+static int num_pages(int len)
+{
+??? return 1 + (((len - 1) & PAGE_MASK) >> PAGE_SHIFT);
+}

Steve, can you explain why is this needed? why isn't get_order() sufficient?

Quoted 5 lines

I thought get_order() gives you a power of two >= the length. ie 1, 2, 4, 8, 16.? For inline_data length of 12KB, for example, we want 3 pages, not 4.? Or am I mistaken?

Just to clarify here:? The target never allocates more that a single page per recv SGE for inline data.? That was a change from v3-v4 of this series.? I eliminated any page allocations of order > 0.? So num_pages() is there to compute the number of pages used to represent len bytes of inline data.? IE <= 4KB, 1 page. ? > 4KB and <= 8KB, 2 pages, etc...

Steve

author
#10 lore raw

v4 cover NVMF/RDMA 16K Inline Support

In reply to Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Thanks,

Steve.

author
#11 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Quoted 5 lines · Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Or I can rebase on 4.18-rc1 and resend...

Steve.

#12 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Quoted 10 lines · Steve Wise
Quoted 5 lines · Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Or I can rebase on 4.18-rc1 and resend...

Yes, please do. I'd also like to see at least an ACK from one or our RDMA low level experts.

#13 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Steve Wise
Quoted 5 lines · Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Quoted 5 lines
Quoted 1 line

Or I can rebase on 4.18-rc1 and resend...

Yes, please do. I'd also like to see at least an ACK from one or our RDMA low level experts.

Actually I was debugging it offline with Steve and sent him some fix that is included in this version.
Please re-send a rebased version and I'll review it.

I realy would like to see numbers before/after this patch like Parav did:
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008059.html

we need to show the trade-off we are doing here between using more memory and the performance we get from it.

-Max.

author
#14 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Hey Max,

Quoted 15 lines · Max Gurtovoy
Quoted 12 lines · Christoph Hellwig
Quoted 10 lines · Steve Wise
Quoted 5 lines · Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Or I can rebase on 4.18-rc1 and resend...

Quoted 8 lines
Quoted 3 lines

Yes, please do.? I'd also like to see at least an ACK from one or our RDMA low level experts.

Actually I was debugging it offline with Steve and sent him some fix that is included in this version.
Please re-send a rebased version and I'll review it.

Should I add a Tested-by line for you?

Quoted 3 lines

I realy would like to see numbers before/after this patch like Parav did:
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008059.html

Given the logic is pretty much the same, I would think those results will stand.? But I can run some of these numbers and add it to the patch comment (or cover letter, whichever is appropriate).

Steve.

author
#15 lore raw

v4 cover NVMF/RDMA 16K Inline Support

In reply to Christoph Hellwig
Quoted 12 lines · Christoph Hellwig
Quoted 8 lines · Steve Wise
Quoted 5 lines · Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Or I can rebase on 4.18-rc1 and resend...

Yes, please do. I'd also like to see at least an ACK from one or our RDMA low level experts.

Sure.

+doug and jason.

There is one merge issue: This series will require the max_send/recv_sge commit, which is merged in rdma-next [1] just recently.? Perhaps you can pull that from the linux-rdma repo if and when these 2 nvme patches are pulled into your repo?? I think that will allow easy merging when the block and rdma repos get merged into Linus' repo.

Steve.

[1] https://www.spinics.net/lists/linux-rdma/msg66118.html

#16 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Quoted 18 lines · Steve Wise
Quoted 12 lines · Christoph Hellwig
Quoted 8 lines · Steve Wise
Quoted 5 lines · Steve Wise

Hey Christoph,? will you merge patches 2 and 3 for -next?? I think they're ready.?? I also have a nvmetcli patch I can send out for using param_inline_data_size.

Or I can rebase on 4.18-rc1 and resend...

Yes, please do. I'd also like to see at least an ACK from one or our RDMA low level experts.

Sure.

Quoted 7 lines

+doug and jason.

There is one merge issue: This series will require the max_send/recv_sge commit, which is merged in rdma-next [1] just recently. Perhaps you can pull that from the linux-rdma repo if and when these 2 nvme patches are pulled into your repo? I think that will allow easy merging when the block and rdma repos get merged into Linus' repo.

Pulling the rdma tree into nvme is probably not something anyone would want to do..

Can we take these patches through rdma instead?

Jason

#17 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Quoted 8 lines · Jason Gunthorpe
Quoted 7 lines

+doug and jason.

There is one merge issue: This series will require the max_send/recv_sge commit, which is merged in rdma-next [1] just recently. Perhaps you can pull that from the linux-rdma repo if and when these 2 nvme patches are pulled into your repo? I think that will allow easy merging when the block and rdma repos get merged into Linus' repo.

Quoted 4 lines

Pulling the rdma tree into nvme is probably not something anyone would want to do..

Can we take these patches through rdma instead?

There are chances for various conflicts as well. I think the best is to just apply the patches without using the new split fields to the nvme tree and then carry the fixup to max_send_sge/max_recv_sge in linux-next.

author
#18 lore raw

v4 cover NVMF/RDMA 16K Inline Support

Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Jason Gunthorpe
Quoted 7 lines

+doug and jason.

There is one merge issue: This series will require the max_send/recv_sge commit, which is merged in rdma-next [1] just recently. Perhaps you can pull that from the linux-rdma repo if and when these 2 nvme patches are pulled into your repo? I think that will allow easy merging when the block and rdma repos get merged into Linus' repo.

Quoted 8 lines
Quoted 4 lines

Pulling the rdma tree into nvme is probably not something anyone would want to do..

Can we take these patches through rdma instead?

There are chances for various conflicts as well. I think the best is to just apply the patches without using the new split fields to the nvme tree and then carry the fixup to max_send_sge/max_recv_sge in linux-next.

Ok, I'll remove the dependency in the next version.

Thanks,

Steve.

#19 lore raw

v4 cover NVMF/RDMA 16K Inline Support

In reply to Christoph Hellwig
Quoted 12 lines · Christoph Hellwig
Quoted 11 lines · Jason Gunthorpe
Quoted 7 lines

+doug and jason.

There is one merge issue: This series will require the max_send/recv_sge commit, which is merged in rdma-next [1] just recently. Perhaps you can pull that from the linux-rdma repo if and when these 2 nvme patches are pulled into your repo? I think that will allow easy merging when the block and rdma repos get merged into Linus' repo.

Pulling the rdma tree into nvme is probably not something anyone would want to do..

Quoted 6 lines
Quoted 1 line

Can we take these patches through rdma instead?

There are chances for various conflicts as well. I think the best is to just apply the patches without using the new split fields to the nvme tree and then carry the fixup to max_send_sge/max_recv_sge in linux-next.

Okay, Steve, however you do this, please make sure your NVMe patch creates a merge conflict with linux-rdma so it does get carried up to Linus as a conflict.

Thanks,
Jason