[PATCH rfc 0/4] Support SQ flow control disabled mode (TP 8005)

[PATCH rfc 0/4] Support SQ flow control disabled mode (TP 8005)

[Sagi Grimberg]

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, but the Linux host will always attempt
to set it in the connect.

A follow up nvme-cli parser for this will follow this series.

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: Ask for fabrics SQ flow control disable by default

 drivers/nvme/host/fabrics.c       |  6 ++++++
 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 +++++++---
 6 files changed, 52 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH rfc 1/4] nvmet: support fabrics sq flow control

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

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

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 7084704b468d..df2af5e3e4d6 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -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;
 
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index d84ae004cb85..16511517dcac 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -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_SQ_FC_DISABLED));
+
 	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:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c5255e743f63..9681be52f6e6 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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);
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 429c4cf90899..42b081f0ee51 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1035,6 +1035,10 @@ struct nvmf_disc_rsp_page_hdr {
 	struct nvmf_disc_rsp_page_entry entries[0];
 };
 
+enum {
+	NVME_CONNECT_SQ_FC_DISABLED	= (1 << 2),
+};
+
 struct nvmf_connect_command {
 	__u8		opcode;
 	__u8		resv1;
-- 
2.17.1

[PATCH rfc 1/4] nvmet: support fabrics sq flow control

[Hannes Reinecke]

On 10/2/18 2:14 AM, Sagi Grimberg wrote:
> 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.
> 
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

[PATCH rfc 2/4] nvmet: don't override treq upon modification.

[Sagi Grimberg]

Only override the allowed parts of it.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/configfs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index b37a8e3e3f80..53b88858ee97 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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;
 }
-- 
2.17.1

[PATCH rfc 2/4] nvmet: don't override treq upon modification.

[Hannes Reinecke]

On 10/2/18 2:14 AM, Sagi Grimberg wrote:
> Only override the allowed parts of it.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/target/configfs.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

[PATCH rfc 2/4] nvmet: don't override treq upon modification.

[Max Gurtovoy]

On 10/2/2018 3:14 AM, Sagi Grimberg wrote:
> Only override the allowed parts of it.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/target/configfs.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index b37a8e3e3f80..53b88858ee97 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -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) {

can you use some MACRO for 0x3 ? this will make the code more readable.

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

also for 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;
>   }
> 

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

[Sagi Grimberg]

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

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

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 53b88858ee97..e0827fce4d21 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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_SQ_FC_DISABLE;
 	config_group_init_type_name(&port->group, name, &nvmet_port_type);
 
 	config_group_init_type_name(&port->subsys_group,
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 42b081f0ee51..c7cb47507891 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -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_SQ_FC_DISABLE = (1 << 2),	/* Supports SQ flow control disable */
 };
 
 /* RDMA QP Service Type codes for Discovery Log Page entry TSAS
-- 
2.17.1

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

[Hannes Reinecke]

On 10/2/18 2:14 AM, Sagi Grimberg wrote:
> Technical Proposal introduces an indication for SQ flow control
> disable support. Expose it since we are able to operate in this mode.
> 
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes

[PATCH rfc 4/4] nvme: Ask for fabrics SQ flow control disable by default

[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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 38cf9d371953..ce61c3e1c22b 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -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);
 
+	/* Ask to disable SQ head pointer updates */
+	cmd.connect.cattr |= NVME_CONNECT_SQ_FC_DISABLED;
+
 	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);
 
+	/* Ask to disable SQ head pointer updates */
+	cmd.connect.cattr |= NVME_CONNECT_SQ_FC_DISABLED;
+
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.17.1

[PATCH rfc 4/4] nvme: Ask for fabrics SQ flow control disable by default

[Hannes Reinecke]

On 10/2/18 2:14 AM, Sagi Grimberg wrote:
> 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 38cf9d371953..ce61c3e1c22b 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -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);
>   
> +	/* Ask to disable SQ head pointer updates */
> +	cmd.connect.cattr |= NVME_CONNECT_SQ_FC_DISABLED;
> +
>   	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);
>   
> +	/* Ask to disable SQ head pointer updates */
> +	cmd.connect.cattr |= NVME_CONNECT_SQ_FC_DISABLED;
> +
>   	data = kzalloc(sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
> 
There are two issues with that:
- as the bit was reserved initially any target is free to reject the 
connect command on the grounds that some undefined bits are set.
- As per SQ flow control specification any target might _legitimately_ 
reject the connect command, namely if the target doesn't allow to 
disable SQ flow control. With this change we would never be able to 
connect to these targets.

Hence I would prefer to have an option '--disable-sqflow' for the 'nvme 
connect' command, allowing us to manually disable SQ Flow control.
That would not modify existing behaviour, and we can properly support 
arrays requiring SQ flow control.

Cheers,

Hannes

[PATCH rfc 4/4] nvme: Ask for fabrics SQ flow control disable by default

[Sagi Grimberg]

>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 38cf9d371953..ce61c3e1c22b 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -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);
>> +??? /* Ask to disable SQ head pointer updates */
>> +??? cmd.connect.cattr |= NVME_CONNECT_SQ_FC_DISABLED;
>> +
>> ????? 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);
>> +??? /* Ask to disable SQ head pointer updates */
>> +??? cmd.connect.cattr |= NVME_CONNECT_SQ_FC_DISABLED;
>> +
>> ????? data = kzalloc(sizeof(*data), GFP_KERNEL);
>> ????? if (!data)
>> ????????? return -ENOMEM;
>>
> There are two issues with that:
> - as the bit was reserved initially any target is free to reject the 
> connect command on the grounds that some undefined bits are set.

Yea... I guess we can't do it by default...

> - As per SQ flow control specification any target might _legitimately_ 
> reject the connect command, namely if the target doesn't allow to 
> disable SQ flow control. With this change we would never be able to 
> connect to these targets.

The TP does says that if the controller disallows disabling flow control
by sending sq_head that is not 0xffff to the connect command, _not_
reject the connect.

> Hence I would prefer to have an option '--disable-sqflow' for the 'nvme 
> connect' command, allowing us to manually disable SQ Flow control.
> That would not modify existing behaviour, and we can properly support 
> arrays requiring SQ flow control.

We could do that, and hook it to connect-all (btw, another reason why
adding a kernel discovery parser in addition to userspace is not a good
idea..)