Support SQ flow control disabled mode (TP 8005)

PATCH Superseded Sagi Grimberg 3 participants 15 messages Oct 3, 2018 – Nov 14, 2018

Subsystems (2)

NVMe NVMe target

lore

Share & tools

lore

Sagi Grimberg

author maintainer

2018-10-03 08:13 UTC #0 lore raw

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

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    | 11 ++++++----
 drivers/nvme/target/core.c        | 34 ++++++++++++++++++++-----------
 drivers/nvme/target/fabrics-cmd.c |  8 +++++++-
 drivers/nvme/target/nvmet.h       |  3 ++-
 include/linux/nvme.h              | 11 +++++++---
 7 files changed, 61 insertions(+), 21 deletions(-)

Signature · 1 line

2.17.1

Sagi Grimberg

author maintainer

2018-10-03 08:13 UTC #1 lore raw

PATCH v2 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>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/target/core.c        | 34 ++++++++++++++++++++-----------
 drivers/nvme/target/fabrics-cmd.c |  8 +++++++-
 drivers/nvme/target/nvmet.h       |  3 ++-
 include/linux/nvme.h              |  4 ++++
 4 files changed, 35 insertions(+), 14 deletions(-)

drivers/nvme/target/core.c +22 ~~-12~~

index 7084704b468d..df2af5e3e4d6 100644
@@ -503,26 +503,35 @@ 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) {
-		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);
+	if (!req->sq->sqhd_disabled) {
+		if (req->sq->size) {
+			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);
+}
+
+static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
+{
+	if (status)
+		nvmet_set_status(req, status);
+
 	req->rsp->sq_id = cpu_to_le16(req->sq->qid);
 	req->rsp->command_id = req->cmd->common.command_id;
 
+	nvmet_update_sq_head(req);
+
 	if (req->ns)
 		nvmet_put_namespace(req->ns);
 	req->ops->queue_response(req);

@@ -545,9 +554,10 @@ void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq,
 }
 
 void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq,
-		u16 qid, u16 size)
+		u16 qid, u16 size, bool sqhd_disabled)
 {
-	sq->sqhd = 0;
+	sq->sqhd_disabled = sqhd_disabled;
+	sq->sqhd = sq->sqhd_disabled ? 0xffff : 0;
 	sq->qid = qid;
 	sq->size = size;

drivers/nvme/target/fabrics-cmd.c +7 -1

index d84ae004cb85..1f05d8507e35 100644
@@ -114,7 +114,9 @@ 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);
+	nvmet_sq_setup(ctrl, req->sq, qid, sqsize + 1,
+			!!(c->cattr & NVME_CONNECT_DISABLE_SQFLOW));
+
 	return 0;
 }

@@ -173,6 +175,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	kfree(d);
 complete:
 	nvmet_req_complete(req, status);
+	if (req->sq->sqhd_disabled)
+		req->sq->sqhd = 0;
 }
 
 static void nvmet_execute_io_connect(struct nvmet_req *req)

@@ -229,6 +233,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 	kfree(d);
 complete:
 	nvmet_req_complete(req, status);
+	if (req->sq->sqhd_disabled)
+		req->sq->sqhd = 0;
 	return;
 
 out_ctrl_put:

drivers/nvme/target/nvmet.h +2 -1

index c5255e743f63..9681be52f6e6 100644
@@ -95,6 +95,7 @@ struct nvmet_sq {
 	u16			qid;
 	u16			size;
 	u32			sqhd;
+	bool			sqhd_disabled;
 	struct completion	free_done;
 	struct completion	confirm_done;
 };

@@ -343,7 +344,7 @@ void nvmet_req_complete(struct nvmet_req *req, u16 status);
 void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq, u16 qid,
 		u16 size);
 void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq, u16 qid,
-		u16 size);
+		u16 size, bool sqhd_disabled);
 void nvmet_sq_destroy(struct nvmet_sq *sq);
 int nvmet_sq_init(struct nvmet_sq *sq);

include/linux/nvme.h +4 -0

index 429c4cf90899..60c9dfc67627 100644
@@ -1035,6 +1035,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

Christoph Hellwig

2018-11-14 14:19 UTC #2 lore raw

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

Quoted 25 lines · Sagi Grimberg

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>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/core.c        | 34 ++++++++++++++++++++-----------
 drivers/nvme/target/fabrics-cmd.c |  8 +++++++-
 drivers/nvme/target/nvmet.h       |  3 ++-
 include/linux/nvme.h              |  4 ++++
 4 files changed, 35 insertions(+), 14 deletions(-)
drivers/nvme/target/core.c +0 -0
index 7084704b468d..df2af5e3e4d6 100644
@@ -503,26 +503,35 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	return ns;

drivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 512

Quoted 10 lines

drivers/nvme/target/core.c +3 -1

 }
 
-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 (!req->sq->sqhd_disabled) {
+		if (req->sq->size) {

This should be (!req->sq->sqhd_disabled && req->sq->size))

drivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 522

Quoted 10 lines

drivers/nvme/target/core.c +7 -0

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

sq_head is a const 0xffff if flow control is disabled isn't it?

Based on that I'd just move the sqhd_disabled check in the caller and assign the constant value there.

drivers/nvme/target/core.c static void nvmet_execute_admin_connect(struct nvmet_req *req) · line 179

Quoted 6 lines

drivers/nvme/target/core.c +2 -0

@@ -173,6 +175,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	kfree(d);
 complete:
 	nvmet_req_complete(req, status);
+	if (req->sq->sqhd_disabled)
+		req->sq->sqhd = 0;

Huh, why?

Sagi Grimberg

author maintainer

2018-11-14 16:29 UTC #3 lore raw

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

Quoted 15 lines

drivers/nvme/target/core.c +1 -1

index 7084704b468d..df2af5e3e4d6 100644
@@ -503,26 +503,35 @@ 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;

Quoted 4 lines

drivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 512
Quoted 2 lines
drivers/nvme/target/core.c +2 -0
+	if (!req->sq->sqhd_disabled) {
+		if (req->sq->size) {
This should be (!req->sq->sqhd_disabled && req->sq->size))

No problem.

Quoted 4 lines

drivers/nvme/target/core.c +4 -0

+			do {
+				old_sqhd = req->sq->sqhd;
+				new_sqhd = (old_sqhd + 1) % req->sq->size;
+			} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) !=

Quoted 8 lines

drivers/nvme/target/core.c struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid) · line 522
Quoted 6 lines
drivers/nvme/target/core.c +3 -0
+						old_sqhd);
+		}
  	}
+
  	sqhd = req->sq->sqhd & 0x0000FFFF;
  	req->rsp->sq_head = cpu_to_le16(sqhd);
sq_head is a const 0xffff if flow control is disabled isn't it?

Unfortunately no, sqhd is supposed to be 0xffff only for the connect response, otherwise its reserved.

Quoted 11 lines

Based on that I'd just move the sqhd_disabled check in the caller and assign the constant value there.
drivers/nvme/target/core.c static void nvmet_execute_admin_connect(struct nvmet_req *req) · line 179
Quoted 6 lines
drivers/nvme/target/core.c +2 -0
@@ -173,6 +175,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
  	kfree(d);
  complete:
  	nvmet_req_complete(req, status);
+	if (req->sq->sqhd_disabled)
+		req->sq->sqhd = 0;
Huh, why?

Because its reserved after the connect response.

Sagi Grimberg

author maintainer

2018-10-03 08:13 UTC #4 lore raw

PATCH v2 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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/nvme/target/configfs.c +6 -4

index b37a8e3e3f80..53b88858ee97 100644
@@ -148,7 +148,7 @@ 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 & 0x3) {
 	case NVMF_TREQ_NOT_SPECIFIED:
 		return sprintf(page, "not specified\n");
 	case NVMF_TREQ_REQUIRED:

@@ -164,6 +164,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 & 0xfc;
 
 	if (port->enabled) {
 		pr_err("Cannot modify address while enabled\n");

@@ -172,15 +173,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;
 }

Signature · 1 line

2.17.1

Christoph Hellwig

2018-11-14 14:20 UTC #5 lore raw

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

Quoted 22 lines · 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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/nvme/target/configfs.c +1 -1

index b37a8e3e3f80..53b88858ee97 100644
@@ -148,7 +148,7 @@ 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 & 0x3) {
 	case NVMF_TREQ_NOT_SPECIFIED:
 		return sprintf(page, "not specified\n");
 	case NVMF_TREQ_REQUIRED:

drivers/nvme/target/configfs.c static ssize_t nvmet_addr_treq_store(struct config_item *item, · line 167

Quoted 4 lines

drivers/nvme/target/configfs.c +1 -0

@@ -164,6 +164,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 & 0xfc;

I think this wants a named constant instead of a magic value.

Sagi Grimberg

author maintainer

2018-11-14 16:30 UTC #6 lore raw

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

Quoted 17 lines

---
  drivers/nvme/target/configfs.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)

drivers/nvme/target/configfs.c +1 -1

index b37a8e3e3f80..53b88858ee97 100644
@@ -148,7 +148,7 @@ 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 & 0x3) {
  	case NVMF_TREQ_NOT_SPECIFIED:
  		return sprintf(page, "not specified\n");

Quoted 8 lines

drivers/nvme/target/configfs.c static ssize_t nvmet_addr_treq_store(struct config_item *item, · line 167
Quoted 6 lines
drivers/nvme/target/configfs.c +1 -0
  	case NVMF_TREQ_REQUIRED:
@@ -164,6 +164,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 & 0xfc;
I think this wants a named constant instead of a magic value.

its a mask that only used here and pretty clear in my mind that takes only the upper bits, but I can change that.

Sagi Grimberg

author maintainer

2018-10-03 08:13 UTC #7 lore raw

PATCH v2 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 53b88858ee97..0ed88e4c28f4 100644
@@ -1150,6 +1150,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 60c9dfc67627..eeb0ed53768f 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 */
 };
 
 /* RDMA QP Service Type codes for Discovery Log Page entry TSAS

Signature · 1 line

2.17.1

Sagi Grimberg

author maintainer

2018-10-03 08:13 UTC #8 lore raw

PATCH v2 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, 15 insertions(+)

drivers/nvme/host/fabrics.c +13 -0

index 38cf9d371953..d1fffaae94e1 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);

@@ -890,6 +900,9 @@ EXPORT_SYMBOL_GPL(nvmf_ip_options_match);
 static int nvmf_check_allowed_opts(struct nvmf_ctrl_options *opts,
 		unsigned int allowed_opts)
 {
+	/* fabrics parameter, allowed regardless of the transport */
+	allowed_opts |= NVMF_OPT_DISABLE_SQFLOW;
+
 	if (opts->mask & ~allowed_opts) {
 		int i;

drivers/nvme/host/fabrics.h +2 -0

index 4f5757e5801f..11cb9747fbf9 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

Hannes Reinecke

2018-10-03 08:41 UTC #9 lore raw

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

Quoted 12 lines · 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, 15 insertions(+)

Reviewed-by Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

Christoph Hellwig

2018-11-14 14:21 UTC #10 lore raw

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

In reply to Sagi Grimberg

Quoted 5 lines · 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.

Shouldn't we default to disabling control for any controller that supports it instead of adding an option?

Sagi Grimberg

author maintainer

2018-11-14 16:32 UTC #11 lore raw

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

Quoted 9 lines

Quoted 6 lines
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.
Shouldn't we default to disabling control for any controller that supports it instead of adding an option?

We only know if the controller supports it from the discovery log entry which is processed from userspace, and it does default to disable if the controller supports it.

Would you prefer that the flag would be opposite? that would change the existing behavior as Hannes indicated in the last version.

Sagi Grimberg

author maintainer

2018-10-03 08:13 UTC #12 lore raw

[PATCH 5/4 nvme-cli] 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).

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 fabrics.c    | 9 +++++++++
 linux/nvme.h | 7 ++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

fabrics.c +9 -0

index d937df47b68e..2db651645f52 100644
@@ -132,6 +132,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)

@@ -748,6 +750,13 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 		return -EINVAL;
 	}
 
+	if (e->treq & NVMF_TREQ_DISABLE_SQFLOW) {
+		len = sprintf(p, ",disable_sqflow");
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (discover)
 		return do_discover(argstr, true);
 	else

linux/nvme.h +4 -3

index 37d3bc1ace6a..4f12c94db4d7 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

Sagi Grimberg

author maintainer

2018-10-16 01:06 UTC #13 lore raw

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

In reply to Sagi Grimberg

Adding James...

Sagi Grimberg

author maintainer

2018-10-31 05:03 UTC #14 lore raw

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

In reply to Sagi Grimberg

Christoph, Keith,

Any objections before moving forward with this?