Skip to contentSkip to conversation

Support SQ flow control disabled mode (TP 8005)

PATCH In review Sagi Grimberg 2 participants 7 messages Nov 19, 2018 – Nov 20, 2018

Subsystems (2)
NVMe NVMe target
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

v4 cover Support SQ flow control disabled mode (TP 8005)

Technical proposal 8005 adds a mode where sq head pointer updates can be omitted based on the controller capability. When a host connects to a controller it can request it to turn off SQ flow control (e.g. omit sq head pointer updates). If the controller supports it, it returns 0xffff sq_head in the connect capsule response cqe.

Supporting this mode in nvmet means we can skip an atomic update in the data path so we don't mind. Note that the host side still ignores sq head pointer updates and flow controls based on completion accounting alone. This TP at least gives us a mode that it is compliant.

When a controller implementation comes along that relies on sq head pointer updates to flow control the host, we will probably need to address that, but until then, we can start by micro-optimizing nvmet.

nvmet also exposes this capability in transport requirement field in the discovery lof entry for host implementations that may want to talk to a controller based on this capability, we use it to have our host to connect with sq flow control disabled if the controller supports it, otherwise the host connects normally as its doing today.

A nvme-cli patch is also attached sent together with the series. This patch is handling a fallback if it works with a host that does not support disable_sqflow but the target does support it.

Changes from v3:
- reworked sq flow control support in nvmet based on hch suggestion
- added disable_sqflow option to nvme-cli connect
- fixed nvme-cli connect-all fallback to stay compatible if the fabrics driver does not support disable_sqflow flag.

Changes from v2:
- fix nested if condition in nvmet_update_sq_head
- introduce meaningful define for fabrics secure channel mask
- added explicit disable_sqflow flag in nvme-cli

Changes from v1:
- changed define names (disable_sqflow) as hannes suggested
- changed the mode to not be used by default and keep the existing behavior, the host will only try to disable sq flow control when userspace asked it to.
- added nvme-cli patch that adds --disable-sqflow if the controller exposes support.

Sagi Grimberg (4):
nvmet: support fabrics sq flow control
nvmet: don't override treq upon modification.
nvmet: expose support for fabrics SQ flow control disable in treq
nvme: disable fabrics SQ flow control when asked by the user

 drivers/nvme/host/fabrics.c       | 13 ++++++++++++-
 drivers/nvme/host/fabrics.h       |  2 ++
 drivers/nvme/target/configfs.c    | 12 ++++++++----
 drivers/nvme/target/core.c        | 22 +++++++++++++---------
 drivers/nvme/target/fabrics-cmd.c |  6 ++++++
 drivers/nvme/target/nvmet.h       |  1 +
 include/linux/nvme.h              | 13 ++++++++++---
 7 files changed, 52 insertions(+), 17 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH v4 1/4 nvmet: support fabrics sq flow control

Technical proposal 8005 "fabrics SQ flow control" introduces a mode where a host and controller agree to omit sq_head pointer updates when sending nvme completions.

In case the host indicated desire to operate in this mode (connect attribute) the controller will return back a connect completion with sq_head value of 0xffff as indication that it will omit sq_head pointer updates.

This mode saves us an atomic update in the I/O path.

Reviewed-by Hannes Reinecke <hare at suse.com>
[hch: suggested better implementation]
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/core.c        | 22 +++++++++++++---------
 drivers/nvme/target/fabrics-cmd.c |  6 ++++++
 drivers/nvme/target/nvmet.h       |  1 +
 include/linux/nvme.h              |  4 ++++
 4 files changed, 24 insertions(+), 9 deletions(-)
drivers/nvme/target/core.c +13 -9
index e3e158ff207a..dd9418d050a9 100644
@@ -597,25 +597,28 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	return ns;
 }
 
-static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
+static void nvmet_update_sq_head(struct nvmet_req *req)
 {
-	u32 old_sqhd, new_sqhd;
-	u16 sqhd;
-
-	if (status)
-		nvmet_set_status(req, status);
-
 	if (req->sq->size) {
+		u32 old_sqhd, new_sqhd;
+
 		do {
 			old_sqhd = req->sq->sqhd;
 			new_sqhd = (old_sqhd + 1) % req->sq->size;
 		} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) !=
 					old_sqhd);
 	}
-	sqhd = req->sq->sqhd & 0x0000FFFF;
-	req->rsp->sq_head = cpu_to_le16(sqhd);
+	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd & 0x0000FFFF);
+}
+
+static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
+{
+	if (!req->sq->sqhd_disabled)
+		nvmet_update_sq_head(req);
 	req->rsp->sq_id = cpu_to_le16(req->sq->qid);
 	req->rsp->command_id = req->cmd->common.command_id;
+	if (status)
+		nvmet_set_status(req, status);
 
 	if (req->ns)
 		nvmet_put_namespace(req->ns);

@@ -765,6 +768,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 	req->sg_cnt = 0;
 	req->transfer_len = 0;
 	req->rsp->status = 0;
+	req->rsp->sq_head = 0;
 	req->ns = NULL;
 
 	/* no support for fused commands yet */
drivers/nvme/target/fabrics-cmd.c +6 -0
index d84ae004cb85..328ae46d8344 100644
@@ -115,6 +115,12 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 	/* note: convert queue size from 0's-based value to 1's-based value */
 	nvmet_cq_setup(ctrl, req->cq, qid, sqsize + 1);
 	nvmet_sq_setup(ctrl, req->sq, qid, sqsize + 1);
+
+	if (c->cattr & NVME_CONNECT_DISABLE_SQFLOW) {
+		req->sq->sqhd_disabled = true;
+		req->rsp->sq_head = cpu_to_le16(0xffff);
+	}
+
 	return 0;
 }
drivers/nvme/target/nvmet.h +1 -0
index 03988fe9d915..547108c41ce9 100644
@@ -106,6 +106,7 @@ struct nvmet_sq {
 	u16			qid;
 	u16			size;
 	u32			sqhd;
+	bool			sqhd_disabled;
 	struct completion	free_done;
 	struct completion	confirm_done;
 };
include/linux/nvme.h +4 -0
index 77d320d32ee5..e7d731776f62 100644
@@ -1044,6 +1044,10 @@ struct nvmf_disc_rsp_page_hdr {
 	struct nvmf_disc_rsp_page_entry entries[0];
 };
 
+enum {
+	NVME_CONNECT_DISABLE_SQFLOW	= (1 << 2),
+};
+
 struct nvmf_connect_command {
 	__u8		opcode;
 	__u8		resv1;
Signature · 1 line
2.17.1
author maintainer
#2 lore raw

PATCH v4 2/4 nvmet: don't override treq upon modification.

In reply to Sagi Grimberg

Only override the allowed parts of it.

Reviewed-by Hannes Reinecke <hare at suse.com>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/configfs.c | 11 +++++++----
 include/linux/nvme.h           |  2 ++
 2 files changed, 9 insertions(+), 4 deletions(-)
drivers/nvme/target/configfs.c +7 -4
index d37fd7713bbc..260a401db01c 100644
@@ -153,7 +153,8 @@ CONFIGFS_ATTR(nvmet_, addr_traddr);
 static ssize_t nvmet_addr_treq_show(struct config_item *item,
 		char *page)
 {
-	switch (to_nvmet_port(item)->disc_addr.treq) {
+	switch (to_nvmet_port(item)->disc_addr.treq &
+		NVME_TREQ_SECURE_CHANNEL_MASK) {
 	case NVMF_TREQ_NOT_SPECIFIED:
 		return sprintf(page, "not specified\n");
 	case NVMF_TREQ_REQUIRED:

@@ -169,6 +170,7 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
 		const char *page, size_t count)
 {
 	struct nvmet_port *port = to_nvmet_port(item);
+	u8 treq = port->disc_addr.treq & ~NVME_TREQ_SECURE_CHANNEL_MASK;
 
 	if (port->enabled) {
 		pr_err("Cannot modify address while enabled\n");

@@ -177,15 +179,16 @@ static ssize_t nvmet_addr_treq_store(struct config_item *item,
 	}
 
 	if (sysfs_streq(page, "not specified")) {
-		port->disc_addr.treq = NVMF_TREQ_NOT_SPECIFIED;
+		treq |= NVMF_TREQ_NOT_SPECIFIED;
 	} else if (sysfs_streq(page, "required")) {
-		port->disc_addr.treq = NVMF_TREQ_REQUIRED;
+		treq |= NVMF_TREQ_REQUIRED;
 	} else if (sysfs_streq(page, "not required")) {
-		port->disc_addr.treq = NVMF_TREQ_NOT_REQUIRED;
+		treq |= NVMF_TREQ_NOT_REQUIRED;
 	} else {
 		pr_err("Invalid value '%s' for treq\n", page);
 		return -EINVAL;
 	}
+	port->disc_addr.treq = treq;
 
 	return count;
 }
include/linux/nvme.h +2 -0
index e7d731776f62..72a6b00a98d6 100644
@@ -63,6 +63,8 @@ enum {
 	NVMF_TREQ_NOT_REQUIRED	= 2,	/* Not Required */
 };
 
+#define NVME_TREQ_SECURE_CHANNEL_MASK 0x3
+
 /* RDMA QP Service Type codes for Discovery Log Page entry TSAS
  * RDMA_QPTYPE field
  */
Signature · 1 line
2.17.1
author maintainer
#3 lore raw

PATCH v4 3/4 nvmet: expose support for fabrics SQ flow control disable in treq

In reply to Sagi Grimberg

Technical Proposal introduces an indication for SQ flow control disable support. Expose it since we are able to operate in this mode.

Reviewed-by Hannes Reinecke <hare at suse.com>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/configfs.c | 1 +
 include/linux/nvme.h           | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)
drivers/nvme/target/configfs.c +1 -0
index 260a401db01c..db2cb64be7ba 100644
@@ -1214,6 +1214,7 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
 	port->inline_data_size = -1;	/* < 0 == let the transport choose */
 
 	port->disc_addr.portid = cpu_to_le16(portid);
+	port->disc_addr.treq = NVMF_TREQ_DISABLE_SQFLOW;
 	config_group_init_type_name(&port->group, name, &nvmet_port_type);
 
 	config_group_init_type_name(&port->subsys_group,
include/linux/nvme.h +4 -3
index 72a6b00a98d6..2f29b480042b 100644
@@ -58,9 +58,10 @@ enum {
 
 /* Transport Requirements codes for Discovery Log Page entry TREQ field */
 enum {
-	NVMF_TREQ_NOT_SPECIFIED	= 0,	/* Not specified */
-	NVMF_TREQ_REQUIRED	= 1,	/* Required */
-	NVMF_TREQ_NOT_REQUIRED	= 2,	/* Not Required */
+	NVMF_TREQ_NOT_SPECIFIED	= 0,		/* Not specified */
+	NVMF_TREQ_REQUIRED	= 1,		/* Required */
+	NVMF_TREQ_NOT_REQUIRED	= 2,		/* Not Required */
+	NVMF_TREQ_DISABLE_SQFLOW = (1 << 2),	/* Supports SQ flow control disable */
 };
 
 #define NVME_TREQ_SECURE_CHANNEL_MASK 0x3
Signature · 1 line
2.17.1
author maintainer
#4 lore raw

PATCH v4 4/4 nvme: disable fabrics SQ flow control when asked by the user

In reply to Sagi Grimberg

As for now, we don't care about sq_head pointer updates anyway, so at least allow the controller to micro-optimize by omiting this update.

Note that we will probably need to support it when a controller that requires this comes along.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/fabrics.c | 13 ++++++++++++-
 drivers/nvme/host/fabrics.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)
drivers/nvme/host/fabrics.c +12 -1
index bd0969db6225..10074ac7731b 100644
@@ -392,6 +392,9 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 	cmd.connect.kato = ctrl->opts->discovery_nqn ? 0 :
 		cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000);
 
+	if (ctrl->opts->disable_sqflow)
+		cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
+
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;

@@ -451,6 +454,9 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 	cmd.connect.qid = cpu_to_le16(qid);
 	cmd.connect.sqsize = cpu_to_le16(ctrl->sqsize);
 
+	if (ctrl->opts->disable_sqflow)
+		cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
+
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;

@@ -607,6 +613,7 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_HOST_TRADDR,		"host_traddr=%s"	},
 	{ NVMF_OPT_HOST_ID,		"hostid=%s"		},
 	{ NVMF_OPT_DUP_CONNECT,		"duplicate_connect"	},
+	{ NVMF_OPT_DISABLE_SQFLOW,	"disable_sqflow"	},
 	{ NVMF_OPT_ERR,			NULL			}
 };
 

@@ -817,6 +824,9 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 		case NVMF_OPT_DUP_CONNECT:
 			opts->duplicate_connect = true;
 			break;
+		case NVMF_OPT_DISABLE_SQFLOW:
+			opts->disable_sqflow = true;
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);

@@ -933,7 +943,8 @@ EXPORT_SYMBOL_GPL(nvmf_free_options);
 #define NVMF_REQUIRED_OPTS	(NVMF_OPT_TRANSPORT | NVMF_OPT_NQN)
 #define NVMF_ALLOWED_OPTS	(NVMF_OPT_QUEUE_SIZE | NVMF_OPT_NR_IO_QUEUES | \
 				 NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
-				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT)
+				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT |\
+				 NVMF_OPT_DISABLE_SQFLOW)
 
 static struct nvme_ctrl *
 nvmf_create_ctrl(struct device *dev, const char *buf, size_t count)
drivers/nvme/host/fabrics.h +2 -0
index 6ea6275f332a..ecd9a006a091 100644
@@ -58,6 +58,7 @@ enum {
 	NVMF_OPT_CTRL_LOSS_TMO	= 1 << 11,
 	NVMF_OPT_HOST_ID	= 1 << 12,
 	NVMF_OPT_DUP_CONNECT	= 1 << 13,
+	NVMF_OPT_DISABLE_SQFLOW = 1 << 14,
 };
 
 /**

@@ -101,6 +102,7 @@ struct nvmf_ctrl_options {
 	unsigned int		kato;
 	struct nvmf_host	*host;
 	int			max_reconnects;
+	bool			disable_sqflow;
 };
 
 /*
Signature · 1 line
2.17.1
author maintainer
#5 lore raw

[PATCH nvme-cli 5/4 v4] fabrics: support fabrics sq flow control disable

In reply to Sagi Grimberg

If the discovery log entry indicates that the subsystem supports disabling sq flow control, we ask the host to connect and disable sq flow control (omit sq_head pointer updates). Also add a connect option to explicitly ask for disable_sqflow.

If we failed to add_ctrl in connect-all command, we retry without it to be backward compatible with a driver that cannot accept the new flag.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 fabrics.c    | 35 +++++++++++++++++++++++++++++------
 linux/nvme.h |  7 ++++---
 2 files changed, 33 insertions(+), 9 deletions(-)
fabrics.c +29 -6
index f2bbd923d95b..b17920b5b1a5 100644
@@ -60,6 +60,7 @@ static struct config {
 	char *raw;
 	char *device;
 	int  duplicate_connect;
+	int  disable_sqflow;
 } cfg = { NULL };
 
 #define BUF_SIZE		4096

@@ -129,6 +130,8 @@ static const char * const treqs[] = {
 	[NVMF_TREQ_NOT_SPECIFIED]	= "not specified",
 	[NVMF_TREQ_REQUIRED]		= "required",
 	[NVMF_TREQ_NOT_REQUIRED]	= "not required",
+	[NVMF_TREQ_DISABLE_SQFLOW]	= "not specified, "
+					  "sq flow control disable supported",
 };
 
 static inline const char *treq_str(__u8 treq)

@@ -595,7 +598,9 @@ static int build_options(char *argstr, int max_len)
 	    add_int_argument(&argstr, &max_len, "ctrl_loss_tmo",
 				cfg.ctrl_loss_tmo) ||
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
-				cfg.duplicate_connect))
+				cfg.duplicate_connect) ||
+	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
+				cfg.disable_sqflow))
 		return -EINVAL;
 
 	return 0;

@@ -603,9 +608,13 @@ static int build_options(char *argstr, int max_len)
 
 static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 {
-	char argstr[BUF_SIZE], *p = argstr;
-	bool discover = false;
-	int len;
+	char argstr[BUF_SIZE], *p;
+	bool discover, disable_sqflow = true;
+	int len, ret;
+
+retry:
+	p = argstr;
+	discover = false;
 
 	switch (e->subtype) {
 	case NVME_NQN_DISC:

@@ -732,10 +741,23 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 		return -EINVAL;
 	}
 
+	if (e->treq & NVMF_TREQ_DISABLE_SQFLOW && disable_sqflow) {
+		len = sprintf(p, ",disable_sqflow");
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (discover)
-		return do_discover(argstr, true);
+		ret = do_discover(argstr, true);
 	else
-		return add_ctrl(argstr);
+		ret = add_ctrl(argstr);
+	if (ret == -EINVAL && e->treq & NVMF_TREQ_DISABLE_SQFLOW) {
+		/* disable_sqflow param might not be supported, try without it */
+		disable_sqflow = false;
+		goto retry;
+	}
+	return ret;
 }
 
 static int connect_ctrls(struct nvmf_disc_rsp_page_hdr *log, int numrec)

@@ -935,6 +957,7 @@ int connect(const char *desc, int argc, char **argv)
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
+		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
 		{NULL},
 	};
linux/nvme.h +4 -3
index 1ad970a4c89a..a6a44b066267 100644
@@ -58,9 +58,10 @@ enum {
 
 /* Transport Requirements codes for Discovery Log Page entry TREQ field */
 enum {
-	NVMF_TREQ_NOT_SPECIFIED	= 0,	/* Not specified */
-	NVMF_TREQ_REQUIRED	= 1,	/* Required */
-	NVMF_TREQ_NOT_REQUIRED	= 2,	/* Not Required */
+	NVMF_TREQ_NOT_SPECIFIED	= 0,		/* Not specified */
+	NVMF_TREQ_REQUIRED	= 1,		/* Required */
+	NVMF_TREQ_NOT_REQUIRED	= 2,		/* Not Required */
+	NVMF_TREQ_DISABLE_SQFLOW = (1 << 2),	/* SQ flow control disable supported */
 };
 
 /* RDMA QP Service Type codes for Discovery Log Page entry TSAS
Signature · 1 line
2.17.1