[PATCH v2 0/5] sqsize fixes

[PATCH v2 0/5] sqsize fixes

[Jay Freyensee]

This is the stable release of the sqsize fixes, based on making
sure sqsize is defined as a zero-based value throughout the code,
per NVMe-over-Fabrics spec.

Patch 1 assigns the admin sqsize to 32, minimum required per spec.
Patch 2-4 takes care of adjusting the code base to use sqsize as 0-based
Patch 5 adjusts hrqsize plus 1

Thus one can take their pick and decide if patch 5 is required based
on NVMe-over-Fabrics spec discussions...you can have your cake
and eat it too :-).

Changes from v1:
 - moved +1/+2 to outside le16_to_cpu() in patch 3 and 5

Changes from v0:
 - found all the sqsize dependencies and adjusted them accordingly
 - nvmf_connect_admin_queue() always uses NVMF_AQ_DEPTH for sqsize
 - final patch to adjust hrqsize only, so the series can be easily
   tested w/hrqsize == hrsqsize (patches 1-4) and hrqsize == hrsqsize+1
   (patch 5)

Jay Freyensee (5):
  fabrics: define admin sqsize min default, per spec
  nvme-rdma: fix sqsize/hsqsize per spec
  nvmet-rdma: +1 to *queue_size from hsqsize/hrqsize
  nvme-loop: set sqsize to 0-based value, per spec
  nvme-rdma: adjust hrqsize to plus 1

 drivers/nvme/host/fabrics.c |  9 ++++++++-
 drivers/nvme/host/rdma.c    | 19 ++++++++++++++-----
 drivers/nvme/target/loop.c  |  4 ++--
 drivers/nvme/target/rdma.c  |  9 +++++----
 4 files changed, 29 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] fabrics: define admin sqsize min default, per spec

[Jay Freyensee]

Upon admin queue connect(), the rdma qp was being
set based on NVMF_AQ_DEPTH.  However, the fabrics layer was
using the sqsize field value set for I/O queues for the admin
queue, which threw the nvme layer and rdma layer off-whack:

root at fedora23-fabrics-host1 nvmf]# dmesg
[ 3507.798642] nvme_fabrics: nvmf_connect_admin_queue():admin sqsize
being sent is: 128
[ 3507.798858] nvme nvme0: creating 16 I/O queues.
[ 3507.896407] nvme nvme0: new ctrl: NQN "nullside-nqn", addr
192.168.1.3:4420

Thus, to have a different admin queue value, we use
NVMF_AQ_DEPTH for connect() and RDMA private data
as the minimum depth specified in the NVMe-over-Fabrics 1.0 spec.

Reported-by: Daniel Verkamp <daniel.verkamp at intel.com>
Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp at intel.com>
---
 drivers/nvme/host/fabrics.c |  9 ++++++++-
 drivers/nvme/host/rdma.c    | 13 +++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index dc99676..020302c 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -363,7 +363,14 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 	cmd.connect.opcode = nvme_fabrics_command;
 	cmd.connect.fctype = nvme_fabrics_type_connect;
 	cmd.connect.qid = 0;
-	cmd.connect.sqsize = cpu_to_le16(ctrl->sqsize);
+
+	/*
+	 * fabrics spec sets a minimum of depth 32 for admin queue,
+	 * so set the queue with this depth always until
+	 * justification otherwise.
+	 */
+	cmd.connect.sqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
+
 	/*
 	 * Set keep-alive timeout in seconds granularity (ms * 1000)
 	 * and add a grace period for controller kato enforcement
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 3e3ce2b..168cd23 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1284,8 +1284,17 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 
 	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
 	priv.qid = cpu_to_le16(nvme_rdma_queue_idx(queue));
-	priv.hrqsize = cpu_to_le16(queue->queue_size);
-	priv.hsqsize = cpu_to_le16(queue->queue_size);
+	/*
+	 * set the admin queue depth to the minimum size
+	 * specified by the Fabrics standard.
+	 */
+	if (priv.qid == 0) {
+		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
+		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
+	} else {
+		priv.hrqsize = cpu_to_le16(queue->queue_size);
+		priv.hsqsize = cpu_to_le16(queue->queue_size);
+	}
 
 	ret = rdma_connect(queue->cm_id, &param);
 	if (ret) {
-- 
2.7.4

[PATCH v2 2/5] nvme-rdma: fix sqsize/hsqsize per spec

[Jay Freyensee]

Per NVMe-over-Fabrics 1.0 spec, sqsize is represented as
a 0-based value.

Also per spec, the RDMA binding values shall be set
to sqsize, which makes hsqsize 0-based values.

Thus, the sqsize during NVMf connect() is now:

[root at fedora23-fabrics-host1 for-48]# dmesg
[  318.720645] nvme_fabrics: nvmf_connect_admin_queue(): sqsize for
admin queue: 31
[  318.720884] nvme nvme0: creating 16 I/O queues.
[  318.810114] nvme_fabrics: nvmf_connect_io_queue(): sqsize for i/o
queue: 127

Reported-by: Daniel Verkamp <daniel.verkamp at intel.com>
Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 drivers/nvme/host/rdma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 168cd23..6aa913e 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -649,7 +649,7 @@ static int nvme_rdma_init_io_queues(struct nvme_rdma_ctrl *ctrl)
 	int i, ret;
 
 	for (i = 1; i < ctrl->queue_count; i++) {
-		ret = nvme_rdma_init_queue(ctrl, i, ctrl->ctrl.sqsize);
+		ret = nvme_rdma_init_queue(ctrl, i, ctrl->ctrl.sqsize + 1);
 		if (ret) {
 			dev_info(ctrl->ctrl.device,
 				"failed to initialize i/o queue: %d\n", ret);
@@ -1292,8 +1292,8 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
 		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
 	} else {
-		priv.hrqsize = cpu_to_le16(queue->queue_size);
-		priv.hsqsize = cpu_to_le16(queue->queue_size);
+		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
+		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
 	}
 
 	ret = rdma_connect(queue->cm_id, &param);
@@ -1818,7 +1818,7 @@ static int nvme_rdma_create_io_queues(struct nvme_rdma_ctrl *ctrl)
 
 	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
 	ctrl->tag_set.ops = &nvme_rdma_mq_ops;
-	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize;
+	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize + 1;
 	ctrl->tag_set.reserved_tags = 1; /* fabric connect */
 	ctrl->tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
@@ -1916,7 +1916,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	spin_lock_init(&ctrl->lock);
 
 	ctrl->queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
-	ctrl->ctrl.sqsize = opts->queue_size;
+	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
 
 	ret = -ENOMEM;
-- 
2.7.4

[PATCH v2 3/5] nvmet-rdma: +1 to *queue_size from hsqsize/hrqsize

[Jay Freyensee]

Now that the host will be sending sqsize 0-based values,
the target need to be adjusted as well.

Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 drivers/nvme/target/rdma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index e06d504..68b7b04 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1004,11 +1004,11 @@ nvmet_rdma_parse_cm_connect_req(struct rdma_conn_param *conn,
 	queue->host_qid = le16_to_cpu(req->qid);
 
 	/*
-	 * req->hsqsize corresponds to our recv queue size
-	 * req->hrqsize corresponds to our send queue size
+	 * req->hsqsize corresponds to our recv queue size plus 1
+	 * req->hrqsize corresponds to our send queue size plus 1
 	 */
-	queue->recv_queue_size = le16_to_cpu(req->hsqsize);
-	queue->send_queue_size = le16_to_cpu(req->hrqsize);
+	queue->recv_queue_size = le16_to_cpu(req->hsqsize) + 1;
+	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 1;
 
 	if (!queue->host_qid && queue->recv_queue_size > NVMF_AQ_DEPTH)
 		return NVME_RDMA_CM_INVALID_HSQSIZE;
-- 
2.7.4

[PATCH v2 4/5] nvme-loop: set sqsize to 0-based value, per spec

[Jay Freyensee]

Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 drivers/nvme/target/loop.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 94e7829..ce22d4b0 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -558,7 +558,7 @@ static int nvme_loop_create_io_queues(struct nvme_loop_ctrl *ctrl)
 
 	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
 	ctrl->tag_set.ops = &nvme_loop_mq_ops;
-	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize;
+	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize + 1;
 	ctrl->tag_set.reserved_tags = 1; /* fabric connect */
 	ctrl->tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
@@ -622,7 +622,7 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 
 	ret = -ENOMEM;
 
-	ctrl->ctrl.sqsize = opts->queue_size;
+	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
 
 	ctrl->queues = kcalloc(opts->nr_io_queues + 1, sizeof(*ctrl->queues),
-- 
2.7.4

[PATCH v2 5/5] nvme-rdma: adjust hrqsize to plus 1

[Jay Freyensee]

Currently under debate due to spec confusion, increase hrqsize
to one more than hsqsize.

Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
---
 drivers/nvme/host/rdma.c   | 4 ++--
 drivers/nvme/target/rdma.c | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6aa913e..524c2b3 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1289,10 +1289,10 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 	 * specified by the Fabrics standard.
 	 */
 	if (priv.qid == 0) {
-		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
+		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH);
 		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
 	} else {
-		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
+		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize + 1);
 		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
 	}
 
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 68b7b04..3557980 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1004,11 +1004,12 @@ nvmet_rdma_parse_cm_connect_req(struct rdma_conn_param *conn,
 	queue->host_qid = le16_to_cpu(req->qid);
 
 	/*
-	 * req->hsqsize corresponds to our recv queue size plus 1
-	 * req->hrqsize corresponds to our send queue size plus 1
+	 * req->hsqsize corresponds to our recv queue size plus 1 as
+	 * this value is based on sqsize, a 0-based value.
+	 * req->hrqsize corresponds to our send queue size plus 2
 	 */
 	queue->recv_queue_size = le16_to_cpu(req->hsqsize) + 1;
-	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 1;
+	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 2;
 
 	if (!queue->host_qid && queue->recv_queue_size > NVMF_AQ_DEPTH)
 		return NVME_RDMA_CM_INVALID_HSQSIZE;
-- 
2.7.4

[PATCH v2 3/5] nvmet-rdma: +1 to *queue_size from hsqsize/hrqsize

[Sagi Grimberg]

> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index e06d504..68b7b04 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1004,11 +1004,11 @@ nvmet_rdma_parse_cm_connect_req(struct rdma_conn_param *conn,
>  	queue->host_qid = le16_to_cpu(req->qid);
>
>  	/*
> -	 * req->hsqsize corresponds to our recv queue size
> -	 * req->hrqsize corresponds to our send queue size
> +	 * req->hsqsize corresponds to our recv queue size plus 1
> +	 * req->hrqsize corresponds to our send queue size plus 1
>  	 */
> -	queue->recv_queue_size = le16_to_cpu(req->hsqsize);
> -	queue->send_queue_size = le16_to_cpu(req->hrqsize);
> +	queue->recv_queue_size = le16_to_cpu(req->hsqsize) + 1;
> +	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 1;
>
>  	if (!queue->host_qid && queue->recv_queue_size > NVMF_AQ_DEPTH)
>  		return NVME_RDMA_CM_INVALID_HSQSIZE;
>

In order to keep bisectability this patch should come first. Otherwise
in prior patches the host sends smaller queue then it actually uses.

So this patch should come first, and only then we make the host
send sqsize-1.

[PATCH v2 2/5] nvme-rdma: fix sqsize/hsqsize per spec

[Sagi Grimberg]

On 15/08/16 19:47, Jay Freyensee wrote:
> Per NVMe-over-Fabrics 1.0 spec, sqsize is represented as
> a 0-based value.
>
> Also per spec, the RDMA binding values shall be set
> to sqsize, which makes hsqsize 0-based values.
>
> Thus, the sqsize during NVMf connect() is now:
>
> [root at fedora23-fabrics-host1 for-48]# dmesg
> [  318.720645] nvme_fabrics: nvmf_connect_admin_queue(): sqsize for
> admin queue: 31
> [  318.720884] nvme nvme0: creating 16 I/O queues.
> [  318.810114] nvme_fabrics: nvmf_connect_io_queue(): sqsize for i/o
> queue: 127
>
> Reported-by: Daniel Verkamp <daniel.verkamp at intel.com>
> Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> ---
>  drivers/nvme/host/rdma.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 168cd23..6aa913e 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -649,7 +649,7 @@ static int nvme_rdma_init_io_queues(struct nvme_rdma_ctrl *ctrl)
>  	int i, ret;
>
>  	for (i = 1; i < ctrl->queue_count; i++) {
> -		ret = nvme_rdma_init_queue(ctrl, i, ctrl->ctrl.sqsize);
> +		ret = nvme_rdma_init_queue(ctrl, i, ctrl->ctrl.sqsize + 1);

Just use opts->queue_size here.

>  		if (ret) {
>  			dev_info(ctrl->ctrl.device,
>  				"failed to initialize i/o queue: %d\n", ret);
> @@ -1292,8 +1292,8 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
>  		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
>  		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
>  	} else {
> -		priv.hrqsize = cpu_to_le16(queue->queue_size);
> -		priv.hsqsize = cpu_to_le16(queue->queue_size);
> +		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
> +		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
>  	}
>
>  	ret = rdma_connect(queue->cm_id, &param);
> @@ -1818,7 +1818,7 @@ static int nvme_rdma_create_io_queues(struct nvme_rdma_ctrl *ctrl)
>
>  	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
>  	ctrl->tag_set.ops = &nvme_rdma_mq_ops;
> -	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize;
> +	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize + 1;

Same here.

>  	ctrl->tag_set.reserved_tags = 1; /* fabric connect */
>  	ctrl->tag_set.numa_node = NUMA_NO_NODE;
>  	ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> @@ -1916,7 +1916,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>  	spin_lock_init(&ctrl->lock);
>
>  	ctrl->queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
> -	ctrl->ctrl.sqsize = opts->queue_size;
> +	ctrl->ctrl.sqsize = opts->queue_size - 1;
>  	ctrl->ctrl.kato = opts->kato;
>
>  	ret = -ENOMEM;
>

[PATCH v2 1/5] fabrics: define admin sqsize min default, per spec

[Sagi Grimberg]

On 15/08/16 19:47, Jay Freyensee wrote:
> Upon admin queue connect(), the rdma qp was being
> set based on NVMF_AQ_DEPTH.  However, the fabrics layer was
> using the sqsize field value set for I/O queues for the admin
> queue, which threw the nvme layer and rdma layer off-whack:
>
> root at fedora23-fabrics-host1 nvmf]# dmesg
> [ 3507.798642] nvme_fabrics: nvmf_connect_admin_queue():admin sqsize
> being sent is: 128
> [ 3507.798858] nvme nvme0: creating 16 I/O queues.
> [ 3507.896407] nvme nvme0: new ctrl: NQN "nullside-nqn", addr
> 192.168.1.3:4420
>
> Thus, to have a different admin queue value, we use
> NVMF_AQ_DEPTH for connect() and RDMA private data
> as the minimum depth specified in the NVMe-over-Fabrics 1.0 spec.
>
> Reported-by: Daniel Verkamp <daniel.verkamp at intel.com>
> Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> Reviewed-by: Daniel Verkamp <daniel.verkamp at intel.com>
> ---
>  drivers/nvme/host/fabrics.c |  9 ++++++++-
>  drivers/nvme/host/rdma.c    | 13 +++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index dc99676..020302c 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -363,7 +363,14 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
>  	cmd.connect.opcode = nvme_fabrics_command;
>  	cmd.connect.fctype = nvme_fabrics_type_connect;
>  	cmd.connect.qid = 0;
> -	cmd.connect.sqsize = cpu_to_le16(ctrl->sqsize);
> +
> +	/*
> +	 * fabrics spec sets a minimum of depth 32 for admin queue,
> +	 * so set the queue with this depth always until
> +	 * justification otherwise.
> +	 */
> +	cmd.connect.sqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> +

Better to keep this part as a stand-alone patch for fabrics.

>  	/*
>  	 * Set keep-alive timeout in seconds granularity (ms * 1000)
>  	 * and add a grace period for controller kato enforcement
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 3e3ce2b..168cd23 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1284,8 +1284,17 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
>
>  	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
>  	priv.qid = cpu_to_le16(nvme_rdma_queue_idx(queue));
> -	priv.hrqsize = cpu_to_le16(queue->queue_size);
> -	priv.hsqsize = cpu_to_le16(queue->queue_size);
> +	/*
> +	 * set the admin queue depth to the minimum size
> +	 * specified by the Fabrics standard.
> +	 */
> +	if (priv.qid == 0) {
> +		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> +		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> +	} else {
> +		priv.hrqsize = cpu_to_le16(queue->queue_size);
> +		priv.hsqsize = cpu_to_le16(queue->queue_size);
> +	}

This should be squashed with the next patch.

[PATCH v2 4/5] nvme-loop: set sqsize to 0-based value, per spec

[Sagi Grimberg]

On 15/08/16 19:47, Jay Freyensee wrote:
> Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> ---
>  drivers/nvme/target/loop.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index 94e7829..ce22d4b0 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -558,7 +558,7 @@ static int nvme_loop_create_io_queues(struct nvme_loop_ctrl *ctrl)
>
>  	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
>  	ctrl->tag_set.ops = &nvme_loop_mq_ops;
> -	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize;
> +	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize + 1;

Just use opts->queue_size.

[PATCH v2 5/5] nvme-rdma: adjust hrqsize to plus 1

[Sagi Grimberg]

On 15/08/16 19:47, Jay Freyensee wrote:
> Currently under debate due to spec confusion, increase hrqsize
> to one more than hsqsize.
>
> Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> ---
>  drivers/nvme/host/rdma.c   | 4 ++--
>  drivers/nvme/target/rdma.c | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6aa913e..524c2b3 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1289,10 +1289,10 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
>  	 * specified by the Fabrics standard.
>  	 */
>  	if (priv.qid == 0) {
> -		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> +		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH);
>  		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
>  	} else {
> -		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
> +		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize + 1);
>  		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
>  	}
>
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 68b7b04..3557980 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1004,11 +1004,12 @@ nvmet_rdma_parse_cm_connect_req(struct rdma_conn_param *conn,
>  	queue->host_qid = le16_to_cpu(req->qid);
>
>  	/*
> -	 * req->hsqsize corresponds to our recv queue size plus 1
> -	 * req->hrqsize corresponds to our send queue size plus 1
> +	 * req->hsqsize corresponds to our recv queue size plus 1 as
> +	 * this value is based on sqsize, a 0-based value.
> +	 * req->hrqsize corresponds to our send queue size plus 2
>  	 */
>  	queue->recv_queue_size = le16_to_cpu(req->hsqsize) + 1;
> -	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 1;
> +	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 2;
>
>  	if (!queue->host_qid && queue->recv_queue_size > NVMF_AQ_DEPTH)
>  		return NVME_RDMA_CM_INVALID_HSQSIZE;
>

Something doesn't look right here.

 From my understanding of the WG discussion. hsqsize is 0's based and
hrqsize isn't.

So this host seems to do the right thing. but why does the target
inc hrqsize+2? you just allocated 2 more recv queue entries when the
host will never send you more than hrqsize.

Am I missing something?

[PATCH v2 1/5] fabrics: define admin sqsize min default, per spec

[J Freyensee]

On Tue, 2016-08-16@11:59 +0300, Sagi Grimberg wrote:
> 
> On 15/08/16 19:47, Jay Freyensee wrote:
> > 
> > Upon admin queue connect(), the rdma qp was being
> > set based on NVMF_AQ_DEPTH.??However, the fabrics layer was
> > using the sqsize field value set for I/O queues for the admin
> > queue, which threw the nvme layer and rdma layer off-whack:
> > 
> > root at fedora23-fabrics-host1 nvmf]# dmesg
> > [ 3507.798642] nvme_fabrics: nvmf_connect_admin_queue():admin
> > sqsize
> > being sent is: 128
> > [ 3507.798858] nvme nvme0: creating 16 I/O queues.
> > [ 3507.896407] nvme nvme0: new ctrl: NQN "nullside-nqn", addr
> > 192.168.1.3:4420
> > 
> > Thus, to have a different admin queue value, we use
> > NVMF_AQ_DEPTH for connect() and RDMA private data
> > as the minimum depth specified in the NVMe-over-Fabrics 1.0 spec.
> > 
> > Reported-by: Daniel Verkamp <daniel.verkamp at intel.com>
> > Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> > Reviewed-by: Daniel Verkamp <daniel.verkamp at intel.com>
> > ---
> > ?drivers/nvme/host/fabrics.c |??9 ++++++++-
> > ?drivers/nvme/host/rdma.c????| 13 +++++++++++--
> > ?2 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/fabrics.c
> > b/drivers/nvme/host/fabrics.c
> > index dc99676..020302c 100644
> > --- a/drivers/nvme/host/fabrics.c
> > +++ b/drivers/nvme/host/fabrics.c
> > @@ -363,7 +363,14 @@ int nvmf_connect_admin_queue(struct nvme_ctrl
> > *ctrl)
> > ?	cmd.connect.opcode = nvme_fabrics_command;
> > ?	cmd.connect.fctype = nvme_fabrics_type_connect;
> > ?	cmd.connect.qid = 0;
> > -	cmd.connect.sqsize = cpu_to_le16(ctrl->sqsize);
> > +
> > +	/*
> > +	?* fabrics spec sets a minimum of depth 32 for admin
> > queue,
> > +	?* so set the queue with this depth always until
> > +	?* justification otherwise.
> > +	?*/
> > +	cmd.connect.sqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > +
> 
> Better to keep this part as a stand-alone patch for fabrics.

I disagree because this is a series to fix all of sqsize. ?Doesn't make
sense to have a stand-alone patch to fix the Admin queue to a zero-
based sqsize value when the I/O queues and it's sqsize value is 1-
based. ?

> 
> > 
> > ?	/*
> > ?	?* Set keep-alive timeout in seconds granularity (ms *
> > 1000)
> > ?	?* and add a grace period for controller kato enforcement
> > diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> > index 3e3ce2b..168cd23 100644
> > --- a/drivers/nvme/host/rdma.c
> > +++ b/drivers/nvme/host/rdma.c
> > @@ -1284,8 +1284,17 @@ static int nvme_rdma_route_resolved(struct
> > nvme_rdma_queue *queue)
> > 
> > ?	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
> > ?	priv.qid = cpu_to_le16(nvme_rdma_queue_idx(queue));
> > -	priv.hrqsize = cpu_to_le16(queue->queue_size);
> > -	priv.hsqsize = cpu_to_le16(queue->queue_size);
> > +	/*
> > +	?* set the admin queue depth to the minimum size
> > +	?* specified by the Fabrics standard.
> > +	?*/
> > +	if (priv.qid == 0) {
> > +		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > +		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > +	} else {
> > +		priv.hrqsize = cpu_to_le16(queue->queue_size);
> > +		priv.hsqsize = cpu_to_le16(queue->queue_size);
> > +	}
> 
> This should be squashed with the next patch.

>From what I understood from Christoph's comments last time, this goes
against what Christoph wanted so this code part should remain in this
patch:

http://lists.infradead.org/pipermail/linux-nvme/2016-August/005779.html
"And while we're at it - the fix to use the separate AQ values should
go into the first patch."

[PATCH v2 2/5] nvme-rdma: fix sqsize/hsqsize per spec

[J Freyensee]

On Tue, 2016-08-16@11:57 +0300, Sagi Grimberg wrote:
> 
> On 15/08/16 19:47, Jay Freyensee wrote:
> > 
> > Per NVMe-over-Fabrics 1.0 spec, sqsize is represented as
> > a 0-based value.
> > 
> > Also per spec, the RDMA binding values shall be set
> > to sqsize, which makes hsqsize 0-based values.
> > 
> > Thus, the sqsize during NVMf connect() is now:
> > 
> > [root at fedora23-fabrics-host1 for-48]# dmesg
> > [??318.720645] nvme_fabrics: nvmf_connect_admin_queue(): sqsize for
> > admin queue: 31
> > [??318.720884] nvme nvme0: creating 16 I/O queues.
> > [??318.810114] nvme_fabrics: nvmf_connect_io_queue(): sqsize for
> > i/o
> > queue: 127
> > 
> > Reported-by: Daniel Verkamp <daniel.verkamp at intel.com>
> > Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> > ---
> > ?drivers/nvme/host/rdma.c | 10 +++++-----
> > ?1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> > index 168cd23..6aa913e 100644
> > --- a/drivers/nvme/host/rdma.c
> > +++ b/drivers/nvme/host/rdma.c
> > @@ -649,7 +649,7 @@ static int nvme_rdma_init_io_queues(struct
> > nvme_rdma_ctrl *ctrl)
> > ?	int i, ret;
> > 
> > ?	for (i = 1; i < ctrl->queue_count; i++) {
> > -		ret = nvme_rdma_init_queue(ctrl, i, ctrl-
> > >ctrl.sqsize);
> > +		ret = nvme_rdma_init_queue(ctrl, i, ctrl-
> > >ctrl.sqsize + 1);
> 
> Just use opts->queue_size here.

OK, no problem.

> 
> > 
> > ?		if (ret) {
> > ?			dev_info(ctrl->ctrl.device,
> > ?				"failed to initialize i/o queue:
> > %d\n", ret);
> > @@ -1292,8 +1292,8 @@ static int nvme_rdma_route_resolved(struct
> > nvme_rdma_queue *queue)
> > ?		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > ?		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > ?	} else {
> > -		priv.hrqsize = cpu_to_le16(queue->queue_size);
> > -		priv.hsqsize = cpu_to_le16(queue->queue_size);
> > +		priv.hrqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.sqsize);
> > +		priv.hsqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.sqsize);
> > ?	}
> > 
> > ?	ret = rdma_connect(queue->cm_id, &param);
> > @@ -1818,7 +1818,7 @@ static int nvme_rdma_create_io_queues(struct
> > nvme_rdma_ctrl *ctrl)
> > 
> > ?	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
> > ?	ctrl->tag_set.ops = &nvme_rdma_mq_ops;
> > -	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize;
> > +	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize + 1;
> 
> Same here.

Dido.

Thanks,
J

[PATCH v2 3/5] nvmet-rdma: +1 to *queue_size from hsqsize/hrqsize

[J Freyensee]

On Tue, 2016-08-16@11:56 +0300, Sagi Grimberg wrote:
> > 
> > diff --git a/drivers/nvme/target/rdma.c
> > b/drivers/nvme/target/rdma.c
> > index e06d504..68b7b04 100644
> > --- a/drivers/nvme/target/rdma.c
> > +++ b/drivers/nvme/target/rdma.c
> > @@ -1004,11 +1004,11 @@ nvmet_rdma_parse_cm_connect_req(struct
> > rdma_conn_param *conn,
> > ?	queue->host_qid = le16_to_cpu(req->qid);
> > 
> > ?	/*
> > -	?* req->hsqsize corresponds to our recv queue size
> > -	?* req->hrqsize corresponds to our send queue size
> > +	?* req->hsqsize corresponds to our recv queue size plus 1
> > +	?* req->hrqsize corresponds to our send queue size plus 1
> > ?	?*/
> > -	queue->recv_queue_size = le16_to_cpu(req->hsqsize);
> > -	queue->send_queue_size = le16_to_cpu(req->hrqsize);
> > +	queue->recv_queue_size = le16_to_cpu(req->hsqsize) + 1;
> > +	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 1;
> > 
> > ?	if (!queue->host_qid && queue->recv_queue_size >
> > NVMF_AQ_DEPTH)
> > ?		return NVME_RDMA_CM_INVALID_HSQSIZE;
> > 
> 
> In order to keep bisectability this patch should come first.
> Otherwise
> in prior patches the host sends smaller queue then it actually uses.

Bisectability is a pretty big word for me :-P. ?Joke aside, I
understand, I can make this the first patch.

> 
> So this patch should come first, and only then we make the host
> send sqsize-1.

[PATCH v2 4/5] nvme-loop: set sqsize to 0-based value, per spec

[J Freyensee]

On Tue, 2016-08-16@11:59 +0300, Sagi Grimberg wrote:
> 
> On 15/08/16 19:47, Jay Freyensee wrote:
> > 
> > Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> > ---
> > ?drivers/nvme/target/loop.c | 4 ++--
> > ?1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/nvme/target/loop.c
> > b/drivers/nvme/target/loop.c
> > index 94e7829..ce22d4b0 100644
> > --- a/drivers/nvme/target/loop.c
> > +++ b/drivers/nvme/target/loop.c
> > @@ -558,7 +558,7 @@ static int nvme_loop_create_io_queues(struct
> > nvme_loop_ctrl *ctrl)
> > 
> > ?	memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
> > ?	ctrl->tag_set.ops = &nvme_loop_mq_ops;
> > -	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize;
> > +	ctrl->tag_set.queue_depth = ctrl->ctrl.sqsize + 1;
> 
> Just use opts->queue_size.

OK, will do.

Thanks,
J

[PATCH v2 5/5] nvme-rdma: adjust hrqsize to plus 1

[J Freyensee]

On Tue, 2016-08-16@12:05 +0300, Sagi Grimberg wrote:
> 
> On 15/08/16 19:47, Jay Freyensee wrote:
> > 
> > Currently under debate due to spec confusion, increase hrqsize
> > to one more than hsqsize.
> > 
> > Signed-off-by: Jay Freyensee <james_p_freyensee at linux.intel.com>
> > ---
> > ?drivers/nvme/host/rdma.c???| 4 ++--
> > ?drivers/nvme/target/rdma.c | 7 ++++---
> > ?2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> > index 6aa913e..524c2b3 100644
> > --- a/drivers/nvme/host/rdma.c
> > +++ b/drivers/nvme/host/rdma.c
> > @@ -1289,10 +1289,10 @@ static int nvme_rdma_route_resolved(struct
> > nvme_rdma_queue *queue)
> > ?	?* specified by the Fabrics standard.
> > ?	?*/
> > ?	if (priv.qid == 0) {
> > -		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > +		priv.hrqsize = cpu_to_le16(NVMF_AQ_DEPTH);
> > ?		priv.hsqsize = cpu_to_le16(NVMF_AQ_DEPTH - 1);
> > ?	} else {
> > -		priv.hrqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.sqsize);
> > +		priv.hrqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.sqsize + 1);
> > ?		priv.hsqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.sqsize);
> > ?	}
> > 
> > diff --git a/drivers/nvme/target/rdma.c
> > b/drivers/nvme/target/rdma.c
> > index 68b7b04..3557980 100644
> > --- a/drivers/nvme/target/rdma.c
> > +++ b/drivers/nvme/target/rdma.c
> > @@ -1004,11 +1004,12 @@ nvmet_rdma_parse_cm_connect_req(struct
> > rdma_conn_param *conn,
> > ?	queue->host_qid = le16_to_cpu(req->qid);
> > 
> > ?	/*
> > -	?* req->hsqsize corresponds to our recv queue size plus 1
> > -	?* req->hrqsize corresponds to our send queue size plus 1
> > +	?* req->hsqsize corresponds to our recv queue size plus 1
> > as
> > +	?* this value is based on sqsize, a 0-based value.
> > +	?* req->hrqsize corresponds to our send queue size plus 2
> > ?	?*/
> > ?	queue->recv_queue_size = le16_to_cpu(req->hsqsize) + 1;
> > -	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 1;
> > +	queue->send_queue_size = le16_to_cpu(req->hrqsize) + 2;
> > 
> > ?	if (!queue->host_qid && queue->recv_queue_size >
> > NVMF_AQ_DEPTH)
> > ?		return NVME_RDMA_CM_INVALID_HSQSIZE;
> > 
> 
> Something doesn't look right here.
> 
> ?From my understanding of the WG discussion. hsqsize is 0's based and
> hrqsize isn't.

Not really, hrqsize being 0's based or 1's based is a bit of moot point
to the fundamental problem, the spec really doesn't explain how to use
hrqsize. ?More following.

> 
> So this host seems to do the right thing. but why does the target
> inc hrqsize+2? you just allocated 2 more recv queue entries when the
> host will never send you more than hrqsize.
> 
> Am I missing something?

Yah, the spec is missing the explanation of how to actually use
hrqsize. Dave Minturn agreed the NVMe committee/consortium needs to
explain how to use this field better in the fabrics spec.

Here is his summary he sent to the NVMe organization:

"The HRQSIZE description needs to explain that the host uses HRQSIZE to
resource the RDMA QP resources for the NVMe CQ, both on the host and on
the controller.??The value of HRQSIZE is based on the sizing of the
NVMe CQ which may be sized to match the size of the NVMe SQ or up to
MAXCMD, if MAXCMD is enabled and the host is using SQHD flow control."

This came out after I submitted this patch series. ?So from his
explanation, I think we can drop this patch all together and just make
NVMe SQ == hsqsize == hrqsize.

J