[PATCHSET v3 0/4] Add support for list issue

[PATCHSET v3 0/4] Add support for list issue

[Jens Axboe]

Hi,

With the support in 5.16-rc1 for allocating and completing batches of
IO, the one missing piece is passing down a list of requests for issue.
Drivers can take advantage of this by defining an mq_ops->queue_rqs()
hook.

This implements it for NVMe, allowing copy of multiple commands in one
swoop.

This is good for around a 500K IOPS/core improvement in my testing,
which is around a 5-6% improvement in efficiency.

No changes since v3 outside of a comment addition.

Changes since v2:
- Add comment on why shared tags are currently bypassed
- Add reviewed-by's

-- 
Jens Axboe

[PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

If we have a list of requests in our plug list, send it to the driver in
one go, if possible. The driver must set mq_ops->queue_rqs() to support
this, if not the usual one-by-one path is used.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 26 +++++++++++++++++++++++---
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e02e7017db03..f24394cb2004 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2512,6 +2512,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
 	struct blk_mq_hw_ctx *this_hctx;
 	struct blk_mq_ctx *this_ctx;
+	struct request *rq;
 	unsigned int depth;
 	LIST_HEAD(list);
 
@@ -2520,7 +2521,28 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	plug->rq_count = 0;
 
 	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
-		struct request_queue *q = rq_list_peek(&plug->mq_list)->q;
+		struct request_queue *q;
+
+		rq = rq_list_peek(&plug->mq_list);
+		q = rq->q;
+
+		/*
+		 * Peek first request and see if we have a ->queue_rqs() hook.
+		 * If we do, we can dispatch the whole plug list in one go. We
+		 * already know at this point that all requests belong to the
+		 * same queue, caller must ensure that's the case.
+		 *
+		 * Since we pass off the full list to the driver at this point,
+		 * we do not increment the active request count for the queue.
+		 * Bypass shared tags for now because of that.
+		 */
+		if (q->mq_ops->queue_rqs &&
+		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
+			blk_mq_run_dispatch_ops(q,
+				q->mq_ops->queue_rqs(&plug->mq_list));
+			if (rq_list_empty(plug->mq_list))
+				return;
+		}
 
 		blk_mq_run_dispatch_ops(q,
 				blk_mq_plug_issue_direct(plug, false));
@@ -2532,8 +2554,6 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	this_ctx = NULL;
 	depth = 0;
 	do {
-		struct request *rq;
-
 		rq = rq_list_pop(&plug->mq_list);
 
 		if (!this_hctx) {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 6f858e05781e..1e1cd9cfbbea 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -493,6 +493,14 @@ struct blk_mq_ops {
 	 */
 	void (*commit_rqs)(struct blk_mq_hw_ctx *);
 
+	/**
+	 * @queue_rqs: Queue a list of new requests. Driver is guaranteed
+	 * that each request belongs to the same queue. If the driver doesn't
+	 * empty the @rqlist completely, then the rest will be queued
+	 * individually by the block layer upon return.
+	 */
+	void (*queue_rqs)(struct request **rqlist);
+
 	/**
 	 * @get_budget: Reserve budget before queue request, once .queue_rq is
 	 * run, it is driver's responsibility to release the
-- 
2.34.1

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Christoph Hellwig]

This whole series seems to miss a linux-block cc..

On Wed, Dec 15, 2021 at 09:24:18AM -0700, Jens Axboe wrote:
> If we have a list of requests in our plug list, send it to the driver in
> one go, if possible. The driver must set mq_ops->queue_rqs() to support
> this, if not the usual one-by-one path is used.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/blk-mq.c         | 26 +++++++++++++++++++++++---
>  include/linux/blk-mq.h |  8 ++++++++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e02e7017db03..f24394cb2004 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2512,6 +2512,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>  {
>  	struct blk_mq_hw_ctx *this_hctx;
>  	struct blk_mq_ctx *this_ctx;
> +	struct request *rq;
>  	unsigned int depth;
>  	LIST_HEAD(list);
>  
> @@ -2520,7 +2521,28 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>  	plug->rq_count = 0;
>  
>  	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
> +		struct request_queue *q;
> +
> +		rq = rq_list_peek(&plug->mq_list);
> +		q = rq->q;

Nit: I'd just drop the q local variable as it is rather pointless.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Keith Busch]

On Wed, Dec 15, 2021 at 09:24:18AM -0700, Jens Axboe wrote:
> +		/*
> +		 * Peek first request and see if we have a ->queue_rqs() hook.
> +		 * If we do, we can dispatch the whole plug list in one go. We
> +		 * already know at this point that all requests belong to the
> +		 * same queue, caller must ensure that's the case.
> +		 *
> +		 * Since we pass off the full list to the driver at this point,
> +		 * we do not increment the active request count for the queue.
> +		 * Bypass shared tags for now because of that.
> +		 */
> +		if (q->mq_ops->queue_rqs &&
> +		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
> +			blk_mq_run_dispatch_ops(q,
> +				q->mq_ops->queue_rqs(&plug->mq_list));

I think we still need to verify the queue isn't quiesced within
blk_mq_run_dispatch_ops()'s rcu protected area, prior to calling
.queue_rqs(). Something like below. Or is this supposed to be the
low-level drivers responsibility now?

---
+void __blk_mq_flush_plug_list(struct request_queue *q, struct blk_plug *plug)
+{
+	if (blk_queue_quiesced(q))
+		return;
+	q->mq_ops->queue_rqs(&plug->mq_list);
+}
+
 void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
 	struct blk_mq_hw_ctx *this_hctx;
@@ -2580,7 +2587,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 		if (q->mq_ops->queue_rqs &&
 		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
 			blk_mq_run_dispatch_ops(q,
-				q->mq_ops->queue_rqs(&plug->mq_list));
+				__blk_mq_flush_plug_list(q, plug));
 			if (rq_list_empty(plug->mq_list))
 				return;
 		}
--

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

On 12/20/21 1:36 PM, Keith Busch wrote:
> On Wed, Dec 15, 2021 at 09:24:18AM -0700, Jens Axboe wrote:
>> +		/*
>> +		 * Peek first request and see if we have a ->queue_rqs() hook.
>> +		 * If we do, we can dispatch the whole plug list in one go. We
>> +		 * already know at this point that all requests belong to the
>> +		 * same queue, caller must ensure that's the case.
>> +		 *
>> +		 * Since we pass off the full list to the driver at this point,
>> +		 * we do not increment the active request count for the queue.
>> +		 * Bypass shared tags for now because of that.
>> +		 */
>> +		if (q->mq_ops->queue_rqs &&
>> +		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
>> +			blk_mq_run_dispatch_ops(q,
>> +				q->mq_ops->queue_rqs(&plug->mq_list));
> 
> I think we still need to verify the queue isn't quiesced within
> blk_mq_run_dispatch_ops()'s rcu protected area, prior to calling
> .queue_rqs(). Something like below. Or is this supposed to be the
> low-level drivers responsibility now?

Yes, that seems very reasonable, and I'd much rather do that than punt it
to the driver. Care to send it as a real patch?

-- 
Jens Axboe

[PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

We'll need it for batched submit as well.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8637538f3fd5..09ea21f75439 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -500,6 +500,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
+static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
+				    struct nvme_command *cmd)
+{
+	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
+		sizeof(*cmd));
+	if (++nvmeq->sq_tail == nvmeq->q_depth)
+		nvmeq->sq_tail = 0;
+}
+
 /**
  * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
  * @nvmeq: The queue to use
@@ -510,10 +519,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
 			    bool write_sq)
 {
 	spin_lock(&nvmeq->sq_lock);
-	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
-	       cmd, sizeof(*cmd));
-	if (++nvmeq->sq_tail == nvmeq->q_depth)
-		nvmeq->sq_tail = 0;
+	nvme_sq_copy_cmd(nvmeq, cmd);
 	nvme_write_sq_db(nvmeq, write_sq);
 	spin_unlock(&nvmeq->sq_lock);
 }
-- 
2.34.1

Re: [PATCH 2/4] nvme: split command copy into a helper

[Christoph Hellwig]

Just like the two times before: NAK.  Please remove nvme_submit_cmd and
open code it in the two callers.

Re: [PATCH 2/4] nvme: split command copy into a helper

[Max Gurtovoy]

On 12/15/2021 6:24 PM, Jens Axboe wrote:
> We'll need it for batched submit as well.
>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   drivers/nvme/host/pci.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8637538f3fd5..09ea21f75439 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -500,6 +500,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
>   	nvmeq->last_sq_tail = nvmeq->sq_tail;
>   }
>   
> +static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
> +				    struct nvme_command *cmd)
> +{
> +	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
> +		sizeof(*cmd));
> +	if (++nvmeq->sq_tail == nvmeq->q_depth)
> +		nvmeq->sq_tail = 0;
> +}
> +
>   /**
>    * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
>    * @nvmeq: The queue to use
> @@ -510,10 +519,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
>   			    bool write_sq)
>   {
>   	spin_lock(&nvmeq->sq_lock);
> -	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> -	       cmd, sizeof(*cmd));
> -	if (++nvmeq->sq_tail == nvmeq->q_depth)
> -		nvmeq->sq_tail = 0;
> +	nvme_sq_copy_cmd(nvmeq, cmd);
>   	nvme_write_sq_db(nvmeq, write_sq);
>   	spin_unlock(&nvmeq->sq_lock);
>   }

Looks good,

Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>

[PATCH 3/4] nvme: separate command prep and issue

[Jens Axboe]

Add a nvme_prep_rq() helper to setup a command, and nvme_queue_rq() is
adapted to use this helper.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 57 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 09ea21f75439..6be6b1ab4285 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -918,52 +918,32 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
 	return BLK_STS_OK;
 }
 
-/*
- * NOTE: ns is NULL when called on the admin queue.
- */
-static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
-			 const struct blk_mq_queue_data *bd)
+static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
 {
-	struct nvme_ns *ns = hctx->queue->queuedata;
-	struct nvme_queue *nvmeq = hctx->driver_data;
-	struct nvme_dev *dev = nvmeq->dev;
-	struct request *req = bd->rq;
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-	struct nvme_command *cmnd = &iod->cmd;
 	blk_status_t ret;
 
 	iod->aborted = 0;
 	iod->npages = -1;
 	iod->nents = 0;
 
-	/*
-	 * We should not need to do this, but we're still using this to
-	 * ensure we can drain requests on a dying queue.
-	 */
-	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
-		return BLK_STS_IOERR;
-
-	if (!nvme_check_ready(&dev->ctrl, req, true))
-		return nvme_fail_nonready_command(&dev->ctrl, req);
-
-	ret = nvme_setup_cmd(ns, req);
+	ret = nvme_setup_cmd(req->q->queuedata, req);
 	if (ret)
 		return ret;
 
 	if (blk_rq_nr_phys_segments(req)) {
-		ret = nvme_map_data(dev, req, cmnd);
+		ret = nvme_map_data(dev, req, &iod->cmd);
 		if (ret)
 			goto out_free_cmd;
 	}
 
 	if (blk_integrity_rq(req)) {
-		ret = nvme_map_metadata(dev, req, cmnd);
+		ret = nvme_map_metadata(dev, req, &iod->cmd);
 		if (ret)
 			goto out_unmap_data;
 	}
 
 	blk_mq_start_request(req);
-	nvme_submit_cmd(nvmeq, cmnd, bd->last);
 	return BLK_STS_OK;
 out_unmap_data:
 	nvme_unmap_data(dev, req);
@@ -972,6 +952,35 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return ret;
 }
 
+/*
+ * NOTE: ns is NULL when called on the admin queue.
+ */
+static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
+			 const struct blk_mq_queue_data *bd)
+{
+	struct nvme_queue *nvmeq = hctx->driver_data;
+	struct nvme_dev *dev = nvmeq->dev;
+	struct request *req = bd->rq;
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	blk_status_t ret;
+
+	/*
+	 * We should not need to do this, but we're still using this to
+	 * ensure we can drain requests on a dying queue.
+	 */
+	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
+		return BLK_STS_IOERR;
+
+	if (unlikely(!nvme_check_ready(&dev->ctrl, req, true)))
+		return nvme_fail_nonready_command(&dev->ctrl, req);
+
+	ret = nvme_prep_rq(dev, req);
+	if (unlikely(ret))
+		return ret;
+	nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
+	return BLK_STS_OK;
+}
+
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-- 
2.34.1

Re: [PATCH 3/4] nvme: separate command prep and issue

[Christoph Hellwig]

On Wed, Dec 15, 2021 at 09:24:20AM -0700, Jens Axboe wrote:
> Add a nvme_prep_rq() helper to setup a command, and nvme_queue_rq() is
> adapted to use this helper.
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

This enables the block layer to send us a full plug list of requests
that need submitting. The block layer guarantees that they all belong
to the same queue, but we do have to check the hardware queue mapping
for each request.

If errors are encountered, leave them in the passed in list. Then the
block layer will handle them individually.

This is good for about a 4% improvement in peak performance, taking us
from 9.6M to 10M IOPS/core.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6be6b1ab4285..197aa45ef7ef 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -981,6 +981,66 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return BLK_STS_OK;
 }
 
+static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
+{
+	spin_lock(&nvmeq->sq_lock);
+	while (!rq_list_empty(*rqlist)) {
+		struct request *req = rq_list_pop(rqlist);
+		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+
+		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
+				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
+		if (++nvmeq->sq_tail == nvmeq->q_depth)
+			nvmeq->sq_tail = 0;
+	}
+	nvme_write_sq_db(nvmeq, true);
+	spin_unlock(&nvmeq->sq_lock);
+}
+
+static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
+{
+	/*
+	 * We should not need to do this, but we're still using this to
+	 * ensure we can drain requests on a dying queue.
+	 */
+	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
+		return false;
+	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
+		return false;
+
+	req->mq_hctx->tags->rqs[req->tag] = req;
+	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
+}
+
+static void nvme_queue_rqs(struct request **rqlist)
+{
+	struct request *req = rq_list_peek(rqlist), *prev = NULL;
+	struct request *requeue_list = NULL;
+
+	do {
+		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
+
+		if (!nvme_prep_rq_batch(nvmeq, req)) {
+			/* detach 'req' and add to remainder list */
+			if (prev)
+				prev->rq_next = req->rq_next;
+			rq_list_add(&requeue_list, req);
+		} else {
+			prev = req;
+		}
+
+		req = rq_list_next(req);
+		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
+			/* detach rest of list, and submit */
+			prev->rq_next = NULL;
+			nvme_submit_cmds(nvmeq, rqlist);
+			*rqlist = req;
+		}
+	} while (req);
+
+	*rqlist = requeue_list;
+}
+
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
@@ -1678,6 +1738,7 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
 
 static const struct blk_mq_ops nvme_mq_ops = {
 	.queue_rq	= nvme_queue_rq,
+	.queue_rqs	= nvme_queue_rqs,
 	.complete	= nvme_pci_complete_rq,
 	.commit_rqs	= nvme_commit_rqs,
 	.init_hctx	= nvme_init_hctx,
-- 
2.34.1

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Keith Busch]

On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
> +static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
> +{
> +	/*
> +	 * We should not need to do this, but we're still using this to
> +	 * ensure we can drain requests on a dying queue.
> +	 */
> +	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
> +		return false;

The patch looks good:

Reviewed-by: Keith Busch <kbusch@kernel.org>

Now a side comment on the above snippet:

I was going to mention in v2 that you shouldn't need to do this for each
request since the queue enabling/disabling only happens while quiesced,
so the state doesn't change once you start a batch. But I realized
multiple hctx's can be in a single batch, so we have to check each of
them instead of just once. :(

I tried to remove this check entirely ("We should not need to do this",
after all), but that's not looking readily possible without just
creating an equivalent check in blk-mq: we can't end a particular
request in failure without draining whatever list it may be linked
within, and we don't know what list it's in when iterating allocated
hctx tags.

Do you happen to have any thoughts on how we could remove this check?
The API I was thinking of is something like "blk_mq_hctx_dead()" in
order to fail pending requests on that hctx without sending them to the
low-level driver so that it wouldn't need these kinds of per-IO checks.

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/15/21 10:29 AM, Keith Busch wrote:
> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>> +static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
>> +{
>> +	/*
>> +	 * We should not need to do this, but we're still using this to
>> +	 * ensure we can drain requests on a dying queue.
>> +	 */
>> +	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
>> +		return false;
> 
> The patch looks good:
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>

Thanks Keith!

> Now a side comment on the above snippet:
> 
> I was going to mention in v2 that you shouldn't need to do this for each
> request since the queue enabling/disabling only happens while quiesced,
> so the state doesn't change once you start a batch. But I realized
> multiple hctx's can be in a single batch, so we have to check each of
> them instead of just once. :(
> 
> I tried to remove this check entirely ("We should not need to do this",
> after all), but that's not looking readily possible without just
> creating an equivalent check in blk-mq: we can't end a particular
> request in failure without draining whatever list it may be linked
> within, and we don't know what list it's in when iterating allocated
> hctx tags.
> 
> Do you happen to have any thoughts on how we could remove this check?
> The API I was thinking of is something like "blk_mq_hctx_dead()" in
> order to fail pending requests on that hctx without sending them to the
> low-level driver so that it wouldn't need these kinds of per-IO checks.

That's a good question, and something I thought about as well while
doing the change. The req based test following it is a bit annoying as
well, but probably harder to get rid of. I didn't pursue this one in
particular, as the single test_bit() is pretty cheap.

Care to take a stab at doing a blk_mq_hctx_dead() addition?

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Christoph Hellwig]

On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
> +	spin_lock(&nvmeq->sq_lock);
> +	while (!rq_list_empty(*rqlist)) {
> +		struct request *req = rq_list_pop(rqlist);
> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> +
> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
> +			nvmeq->sq_tail = 0;

So this doesn't even use the new helper added in patch 2?  I think this
should call nvme_sq_copy_cmd().

The rest looks identical to the incremental patch I posted, so I guess
the performance degration measured on the first try was a measurement
error?

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>> +	spin_lock(&nvmeq->sq_lock);
>> +	while (!rq_list_empty(*rqlist)) {
>> +		struct request *req = rq_list_pop(rqlist);
>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>> +
>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>> +			nvmeq->sq_tail = 0;
> So this doesn't even use the new helper added in patch 2?  I think this
> should call nvme_sq_copy_cmd().

I also noticed that.

So need to decide if to open code it or use the helper function.

Inline helper sounds reasonable if you have 3 places that will use it.

> The rest looks identical to the incremental patch I posted, so I guess
> the performance degration measured on the first try was a measurement
> error?

giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.

But how do you moderate it ? what is the batch_sz <--> time_to_wait 
algorithm ?

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 6:06 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>> +	spin_lock(&nvmeq->sq_lock);
>>> +	while (!rq_list_empty(*rqlist)) {
>>> +		struct request *req = rq_list_pop(rqlist);
>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>> +
>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>> +			nvmeq->sq_tail = 0;
>> So this doesn't even use the new helper added in patch 2?  I think this
>> should call nvme_sq_copy_cmd().
> 
> I also noticed that.
> 
> So need to decide if to open code it or use the helper function.
> 
> Inline helper sounds reasonable if you have 3 places that will use it.

Yes agree, that's been my stance too :-)

>> The rest looks identical to the incremental patch I posted, so I guess
>> the performance degration measured on the first try was a measurement
>> error?
> 
> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
> 
> But how do you moderate it ? what is the batch_sz <--> time_to_wait 
> algorithm ?

The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
in total. I do agree that if we ever made it much larger, then we might
want to cap it differently. But 32 seems like a pretty reasonable number
to get enough gain from the batching done in various areas, while still
not making it so large that we have a potential latency issue. That
batch count is already used consistently for other items too (like tag
allocation), so it's not specific to just this one case.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 5:48 PM, Jens Axboe wrote:
> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>> +	spin_lock(&nvmeq->sq_lock);
>>>> +	while (!rq_list_empty(*rqlist)) {
>>>> +		struct request *req = rq_list_pop(rqlist);
>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>> +
>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>> +			nvmeq->sq_tail = 0;
>>> So this doesn't even use the new helper added in patch 2?  I think this
>>> should call nvme_sq_copy_cmd().
>> I also noticed that.
>>
>> So need to decide if to open code it or use the helper function.
>>
>> Inline helper sounds reasonable if you have 3 places that will use it.
> Yes agree, that's been my stance too :-)
>
>>> The rest looks identical to the incremental patch I posted, so I guess
>>> the performance degration measured on the first try was a measurement
>>> error?
>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>
>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>> algorithm ?
> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
> in total. I do agree that if we ever made it much larger, then we might
> want to cap it differently. But 32 seems like a pretty reasonable number
> to get enough gain from the batching done in various areas, while still
> not making it so large that we have a potential latency issue. That
> batch count is already used consistently for other items too (like tag
> allocation), so it's not specific to just this one case.

I'm saying that the you can wait to the batch_max_count too long and it 
won't be efficient from latency POV.

So it's better to limit the block layar to wait for the first to come: x 
usecs or batch_max_count before issue queue_rqs.

Also, This batch is per HW queue or SW queue or the entire request queue ?

>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:00 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>> +
>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>> +			nvmeq->sq_tail = 0;
>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>> should call nvme_sq_copy_cmd().
>>> I also noticed that.
>>>
>>> So need to decide if to open code it or use the helper function.
>>>
>>> Inline helper sounds reasonable if you have 3 places that will use it.
>> Yes agree, that's been my stance too :-)
>>
>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>> the performance degration measured on the first try was a measurement
>>>> error?
>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>
>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>> algorithm ?
>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>> in total. I do agree that if we ever made it much larger, then we might
>> want to cap it differently. But 32 seems like a pretty reasonable number
>> to get enough gain from the batching done in various areas, while still
>> not making it so large that we have a potential latency issue. That
>> batch count is already used consistently for other items too (like tag
>> allocation), so it's not specific to just this one case.
> 
> I'm saying that the you can wait to the batch_max_count too long and it 
> won't be efficient from latency POV.
> 
> So it's better to limit the block layar to wait for the first to come: x 
> usecs or batch_max_count before issue queue_rqs.

There's no waiting specifically for this, it's just based on the plug.
We just won't do more than 32 in that plug. This is really just an
artifact of the plugging, and if that should be limited based on "max of
32 or xx time", then that should be done there.

But in general I think it's saner and enough to just limit the total
size. If we spend more than xx usec building up the plug list, we're
doing something horribly wrong. That really should not happen with 32
requests, and we'll never eg wait on requests if we're out of tags. That
will result in a plug flush to begin with.

> Also, This batch is per HW queue or SW queue or the entire request queue ?

It's per submitter, so whatever the submitter ends up queueing IO
against. In general it'll be per-queue.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 6:05 PM, Jens Axboe wrote:
> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>> +
>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>> +			nvmeq->sq_tail = 0;
>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>> should call nvme_sq_copy_cmd().
>>>> I also noticed that.
>>>>
>>>> So need to decide if to open code it or use the helper function.
>>>>
>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>> Yes agree, that's been my stance too :-)
>>>
>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>> the performance degration measured on the first try was a measurement
>>>>> error?
>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>
>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>> algorithm ?
>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>> in total. I do agree that if we ever made it much larger, then we might
>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>> to get enough gain from the batching done in various areas, while still
>>> not making it so large that we have a potential latency issue. That
>>> batch count is already used consistently for other items too (like tag
>>> allocation), so it's not specific to just this one case.
>> I'm saying that the you can wait to the batch_max_count too long and it
>> won't be efficient from latency POV.
>>
>> So it's better to limit the block layar to wait for the first to come: x
>> usecs or batch_max_count before issue queue_rqs.
> There's no waiting specifically for this, it's just based on the plug.
> We just won't do more than 32 in that plug. This is really just an
> artifact of the plugging, and if that should be limited based on "max of
> 32 or xx time", then that should be done there.
>
> But in general I think it's saner and enough to just limit the total
> size. If we spend more than xx usec building up the plug list, we're
> doing something horribly wrong. That really should not happen with 32
> requests, and we'll never eg wait on requests if we're out of tags. That
> will result in a plug flush to begin with.

I'm not aware of the plug. I hope to get to it soon.

My concern is if the user application submitted only 28 requests and 
then you'll wait forever ? or for very long time.

I guess not, but I'm asking how do you know how to batch and when to 
stop in case 32 commands won't arrive anytime soon.

>
>> Also, This batch is per HW queue or SW queue or the entire request queue ?
> It's per submitter, so whatever the submitter ends up queueing IO
> against. In general it'll be per-queue.

struct request_queue ?

I think the best is to batch per struct blk_mq_hw_ctx.

I see that you check this in the nvme_pci driver but shouldn't it go to 
the block layer ?

>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:19 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>> +
>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>> should call nvme_sq_copy_cmd().
>>>>> I also noticed that.
>>>>>
>>>>> So need to decide if to open code it or use the helper function.
>>>>>
>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>> Yes agree, that's been my stance too :-)
>>>>
>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>> the performance degration measured on the first try was a measurement
>>>>>> error?
>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>
>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>> algorithm ?
>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>> in total. I do agree that if we ever made it much larger, then we might
>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>> to get enough gain from the batching done in various areas, while still
>>>> not making it so large that we have a potential latency issue. That
>>>> batch count is already used consistently for other items too (like tag
>>>> allocation), so it's not specific to just this one case.
>>> I'm saying that the you can wait to the batch_max_count too long and it
>>> won't be efficient from latency POV.
>>>
>>> So it's better to limit the block layar to wait for the first to come: x
>>> usecs or batch_max_count before issue queue_rqs.
>> There's no waiting specifically for this, it's just based on the plug.
>> We just won't do more than 32 in that plug. This is really just an
>> artifact of the plugging, and if that should be limited based on "max of
>> 32 or xx time", then that should be done there.
>>
>> But in general I think it's saner and enough to just limit the total
>> size. If we spend more than xx usec building up the plug list, we're
>> doing something horribly wrong. That really should not happen with 32
>> requests, and we'll never eg wait on requests if we're out of tags. That
>> will result in a plug flush to begin with.
> 
> I'm not aware of the plug. I hope to get to it soon.
> 
> My concern is if the user application submitted only 28 requests and 
> then you'll wait forever ? or for very long time.
> 
> I guess not, but I'm asking how do you know how to batch and when to 
> stop in case 32 commands won't arrive anytime soon.

The plug is in the stack of the task, so that condition can never
happen. If the application originally asks for 32 but then only submits
28, then once that last one is submitted the plug is flushed and
requests are issued.

>>> Also, This batch is per HW queue or SW queue or the entire request queue ?
>> It's per submitter, so whatever the submitter ends up queueing IO
>> against. In general it'll be per-queue.
> 
> struct request_queue ?
> 
> I think the best is to batch per struct blk_mq_hw_ctx.
> 
> I see that you check this in the nvme_pci driver but shouldn't it go to 
> the block layer ?

That's not how plugging works. In general, unless your task bounces
around, then it'll be a single queue and a single hw queue as well.
Adding code to specifically check the mappings and flush at that point
would be a net loss, rather than just deal with it if it happens for
some cases.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 6:25 PM, Jens Axboe wrote:
> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>> +
>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>> should call nvme_sq_copy_cmd().
>>>>>> I also noticed that.
>>>>>>
>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>
>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>> Yes agree, that's been my stance too :-)
>>>>>
>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>> error?
>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>
>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>> algorithm ?
>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>> to get enough gain from the batching done in various areas, while still
>>>>> not making it so large that we have a potential latency issue. That
>>>>> batch count is already used consistently for other items too (like tag
>>>>> allocation), so it's not specific to just this one case.
>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>> won't be efficient from latency POV.
>>>>
>>>> So it's better to limit the block layar to wait for the first to come: x
>>>> usecs or batch_max_count before issue queue_rqs.
>>> There's no waiting specifically for this, it's just based on the plug.
>>> We just won't do more than 32 in that plug. This is really just an
>>> artifact of the plugging, and if that should be limited based on "max of
>>> 32 or xx time", then that should be done there.
>>>
>>> But in general I think it's saner and enough to just limit the total
>>> size. If we spend more than xx usec building up the plug list, we're
>>> doing something horribly wrong. That really should not happen with 32
>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>> will result in a plug flush to begin with.
>> I'm not aware of the plug. I hope to get to it soon.
>>
>> My concern is if the user application submitted only 28 requests and
>> then you'll wait forever ? or for very long time.
>>
>> I guess not, but I'm asking how do you know how to batch and when to
>> stop in case 32 commands won't arrive anytime soon.
> The plug is in the stack of the task, so that condition can never
> happen. If the application originally asks for 32 but then only submits
> 28, then once that last one is submitted the plug is flushed and
> requests are issued.

So if I'm running fio with --iodepth=28 what will plug do ? send batches 
of 28 ? or 1 by 1 ?

>>>> Also, This batch is per HW queue or SW queue or the entire request queue ?
>>> It's per submitter, so whatever the submitter ends up queueing IO
>>> against. In general it'll be per-queue.
>> struct request_queue ?
>>
>> I think the best is to batch per struct blk_mq_hw_ctx.
>>
>> I see that you check this in the nvme_pci driver but shouldn't it go to
>> the block layer ?
> That's not how plugging works. In general, unless your task bounces
> around, then it'll be a single queue and a single hw queue as well.
> Adding code to specifically check the mappings and flush at that point
> would be a net loss, rather than just deal with it if it happens for
> some cases.
>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:34 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>> +
>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>> I also noticed that.
>>>>>>>
>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>
>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>> Yes agree, that's been my stance too :-)
>>>>>>
>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>> error?
>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>
>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>> algorithm ?
>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>> not making it so large that we have a potential latency issue. That
>>>>>> batch count is already used consistently for other items too (like tag
>>>>>> allocation), so it's not specific to just this one case.
>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>> won't be efficient from latency POV.
>>>>>
>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>> usecs or batch_max_count before issue queue_rqs.
>>>> There's no waiting specifically for this, it's just based on the plug.
>>>> We just won't do more than 32 in that plug. This is really just an
>>>> artifact of the plugging, and if that should be limited based on "max of
>>>> 32 or xx time", then that should be done there.
>>>>
>>>> But in general I think it's saner and enough to just limit the total
>>>> size. If we spend more than xx usec building up the plug list, we're
>>>> doing something horribly wrong. That really should not happen with 32
>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>> will result in a plug flush to begin with.
>>> I'm not aware of the plug. I hope to get to it soon.
>>>
>>> My concern is if the user application submitted only 28 requests and
>>> then you'll wait forever ? or for very long time.
>>>
>>> I guess not, but I'm asking how do you know how to batch and when to
>>> stop in case 32 commands won't arrive anytime soon.
>> The plug is in the stack of the task, so that condition can never
>> happen. If the application originally asks for 32 but then only submits
>> 28, then once that last one is submitted the plug is flushed and
>> requests are issued.
> 
> So if I'm running fio with --iodepth=28 what will plug do ? send batches 
> of 28 ? or 1 by 1 ?

--iodepth just controls the overall depth, the batch submit count
dictates what happens further down. If you run queue depth 28 and submit
one at the time, then you'll get one at the time further down too. Hence
the batching is directly driven by what the application is already
doing.


-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 6:36 PM, Jens Axboe wrote:
> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>> +
>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>> I also noticed that.
>>>>>>>>
>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>
>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>
>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>> error?
>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>
>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>> algorithm ?
>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>> allocation), so it's not specific to just this one case.
>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>> won't be efficient from latency POV.
>>>>>>
>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>> 32 or xx time", then that should be done there.
>>>>>
>>>>> But in general I think it's saner and enough to just limit the total
>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>> doing something horribly wrong. That really should not happen with 32
>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>> will result in a plug flush to begin with.
>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>
>>>> My concern is if the user application submitted only 28 requests and
>>>> then you'll wait forever ? or for very long time.
>>>>
>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>> stop in case 32 commands won't arrive anytime soon.
>>> The plug is in the stack of the task, so that condition can never
>>> happen. If the application originally asks for 32 but then only submits
>>> 28, then once that last one is submitted the plug is flushed and
>>> requests are issued.
>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>> of 28 ? or 1 by 1 ?
> --iodepth just controls the overall depth, the batch submit count
> dictates what happens further down. If you run queue depth 28 and submit
> one at the time, then you'll get one at the time further down too. Hence
> the batching is directly driven by what the application is already
> doing.

I see. Thanks for the explanation.

So it works only for io_uring based applications ?

Don't you think it will be a good idea to not depend on applications and 
batch according to some kernel mechanism ?

Wait till X requests or Y usecs (first condition to be fulfilled) before 
submitting the batch to LLD.

Like we do with adaptive completion coalescing/moderation for capable 
devices.


>
>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:57 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>> +
>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>> I also noticed that.
>>>>>>>>>
>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>
>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>
>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>> error?
>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>
>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>> algorithm ?
>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>> won't be efficient from latency POV.
>>>>>>>
>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>> 32 or xx time", then that should be done there.
>>>>>>
>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>> will result in a plug flush to begin with.
>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>
>>>>> My concern is if the user application submitted only 28 requests and
>>>>> then you'll wait forever ? or for very long time.
>>>>>
>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>> stop in case 32 commands won't arrive anytime soon.
>>>> The plug is in the stack of the task, so that condition can never
>>>> happen. If the application originally asks for 32 but then only submits
>>>> 28, then once that last one is submitted the plug is flushed and
>>>> requests are issued.
>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>> of 28 ? or 1 by 1 ?
>> --iodepth just controls the overall depth, the batch submit count
>> dictates what happens further down. If you run queue depth 28 and submit
>> one at the time, then you'll get one at the time further down too. Hence
>> the batching is directly driven by what the application is already
>> doing.
> 
> I see. Thanks for the explanation.
> 
> So it works only for io_uring based applications ?

It's only enabled for io_uring right now, but it's generically available
for anyone that wants to use it... Would be trivial to do for aio, and
other spots that currently use blk_start_plug() and has an idea of how
many IOs will be submitted.

> Don't you think it will be a good idea to not depend on applications and 
> batch according to some kernel mechanism ?
> 
> Wait till X requests or Y usecs (first condition to be fulfilled) before 
> submitting the batch to LLD.
> 
> Like we do with adaptive completion coalescing/moderation for capable 
> devices.

This is how plugging used to work way back in the day. The problem is
that you then introduce per-device state, which can cause contention.
That's why the plug is a pure stack based entity now.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 7:16 PM, Jens Axboe wrote:
> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>> +
>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>> I also noticed that.
>>>>>>>>>>
>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>
>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>
>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>> error?
>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>
>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>> algorithm ?
>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>> won't be efficient from latency POV.
>>>>>>>>
>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>
>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>> will result in a plug flush to begin with.
>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>
>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>> then you'll wait forever ? or for very long time.
>>>>>>
>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>> The plug is in the stack of the task, so that condition can never
>>>>> happen. If the application originally asks for 32 but then only submits
>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>> requests are issued.
>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>> of 28 ? or 1 by 1 ?
>>> --iodepth just controls the overall depth, the batch submit count
>>> dictates what happens further down. If you run queue depth 28 and submit
>>> one at the time, then you'll get one at the time further down too. Hence
>>> the batching is directly driven by what the application is already
>>> doing.
>> I see. Thanks for the explanation.
>>
>> So it works only for io_uring based applications ?
> It's only enabled for io_uring right now, but it's generically available
> for anyone that wants to use it... Would be trivial to do for aio, and
> other spots that currently use blk_start_plug() and has an idea of how
> many IOs will be submitted

Can you please share an example application (or is it fio patches) that 
can submit batches ? The same that was used to test this patchset is 
fine too.

I would like to test it with our NVMe SNAP controllers and also to 
develop NVMe/RDMA queue_rqs code and test the perf with it.

> .
>
>> Don't you think it will be a good idea to not depend on applications and
>> batch according to some kernel mechanism ?
>>
>> Wait till X requests or Y usecs (first condition to be fulfilled) before
>> submitting the batch to LLD.
>>
>> Like we do with adaptive completion coalescing/moderation for capable
>> devices.
> This is how plugging used to work way back in the day. The problem is
> that you then introduce per-device state, which can cause contention.
> That's why the plug is a pure stack based entity now.
>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/19/21 5:14 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>
>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>
>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>
>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>> error?
>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>
>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>> algorithm ?
>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>
>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>
>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>> will result in a plug flush to begin with.
>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>
>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>
>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>> requests are issued.
>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>> of 28 ? or 1 by 1 ?
>>>> --iodepth just controls the overall depth, the batch submit count
>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>> one at the time, then you'll get one at the time further down too. Hence
>>>> the batching is directly driven by what the application is already
>>>> doing.
>>> I see. Thanks for the explanation.
>>>
>>> So it works only for io_uring based applications ?
>> It's only enabled for io_uring right now, but it's generically available
>> for anyone that wants to use it... Would be trivial to do for aio, and
>> other spots that currently use blk_start_plug() and has an idea of how
>> many IOs will be submitted
> 
> Can you please share an example application (or is it fio patches) that 
> can submit batches ? The same that was used to test this patchset is 
> fine too.
> 
> I would like to test it with our NVMe SNAP controllers and also to 
> develop NVMe/RDMA queue_rqs code and test the perf with it.

You should just be able to use iodepth_batch with fio. For my peak
testing, I use t/io_uring from the fio repo. By default, it'll run QD of
and do batches of 32 for complete and submit. You can just run:

t/io_uring <dev or file>

maybe adding -p0 for IRQ driven rather than polled IO.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/19/2021 4:48 PM, Jens Axboe wrote:
> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>
>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>
>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>
>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>> error?
>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>
>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>> algorithm ?
>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>
>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>
>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>
>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>
>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>> requests are issued.
>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>> of 28 ? or 1 by 1 ?
>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>> the batching is directly driven by what the application is already
>>>>> doing.
>>>> I see. Thanks for the explanation.
>>>>
>>>> So it works only for io_uring based applications ?
>>> It's only enabled for io_uring right now, but it's generically available
>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>> other spots that currently use blk_start_plug() and has an idea of how
>>> many IOs will be submitted
>> Can you please share an example application (or is it fio patches) that
>> can submit batches ? The same that was used to test this patchset is
>> fine too.
>>
>> I would like to test it with our NVMe SNAP controllers and also to
>> develop NVMe/RDMA queue_rqs code and test the perf with it.
> You should just be able to use iodepth_batch with fio. For my peak
> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
> and do batches of 32 for complete and submit. You can just run:
>
> t/io_uring <dev or file>
>
> maybe adding -p0 for IRQ driven rather than polled IO.

I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA 
but it was never called using the t/io_uring test nor fio with 
iodepth_batch=32 flag with io_uring engine.

Any idea what might be the issue ?

I installed fio from sources..

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/20/21 3:11 AM, Max Gurtovoy wrote:
> 
> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>
>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>> error?
>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>
>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>
>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>
>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>
>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>> requests are issued.
>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>> of 28 ? or 1 by 1 ?
>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>> the batching is directly driven by what the application is already
>>>>>> doing.
>>>>> I see. Thanks for the explanation.
>>>>>
>>>>> So it works only for io_uring based applications ?
>>>> It's only enabled for io_uring right now, but it's generically available
>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>> many IOs will be submitted
>>> Can you please share an example application (or is it fio patches) that
>>> can submit batches ? The same that was used to test this patchset is
>>> fine too.
>>>
>>> I would like to test it with our NVMe SNAP controllers and also to
>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>> You should just be able to use iodepth_batch with fio. For my peak
>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>> and do batches of 32 for complete and submit. You can just run:
>>
>> t/io_uring <dev or file>
>>
>> maybe adding -p0 for IRQ driven rather than polled IO.
> 
> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA 
> but it was never called using the t/io_uring test nor fio with 
> iodepth_batch=32 flag with io_uring engine.
> 
> Any idea what might be the issue ?
> 
> I installed fio from sources..

The two main restrictions right now are a scheduler and shared tags, are
you using any of those?

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/20/21 7:19 AM, Jens Axboe wrote:
>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>> You should just be able to use iodepth_batch with fio. For my peak
>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>> and do batches of 32 for complete and submit. You can just run:
>>>
>>> t/io_uring <dev or file>
>>>
>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>
>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA 
>> but it was never called using the t/io_uring test nor fio with 
>> iodepth_batch=32 flag with io_uring engine.
>>
>> Any idea what might be the issue ?
>>
>> I installed fio from sources..
> 
> The two main restrictions right now are a scheduler and shared tags, are
> you using any of those?

Here's a sample run, which is 2 threads, each driving 2 devices and
using 31 (-s31) batch submit count, with 16 batch completions (-c16).
Block size is 512b. Ignore most other options, they don't really matter,
the defaults are 128 QD, 32 submit batch, 32 complete batch.

$ sudo taskset -c 10,11 t/io_uring -d256 -b512 -s31 -c16 -p1 -F1 -B1 -n2 /dev/nvme0n1 /dev/nvme3n1 /dev/nvme2n1 /dev/nvme4n1
Added file /dev/nvme0n1 (submitter 0)
Added file /dev/nvme3n1 (submitter 1)
Added file /dev/nvme2n1 (submitter 0)
Added file /dev/nvme4n1 (submitter 1)
polled=1, fixedbufs=1/1, register_files=1, buffered=0, QD=256
Engine=io_uring, sq_ring=256, cq_ring=256
submitter=0, tid=91490
submitter=1, tid=91491
IOPS=13038K, BW=6366MiB/s, IOS/call=30/30, inflight=(128 5 120 128)
IOPS=13042K, BW=6368MiB/s, IOS/call=30/30, inflight=(128 96 128 15)
IOPS=13049K, BW=6371MiB/s, IOS/call=30/30, inflight=(128 122 120 128)
IOPS=13045K, BW=6369MiB/s, IOS/call=30/30, inflight=(110 128 99 128)

That's driving 13M IOPS, using a single CPU core (10/11 are thread
siblings). Top of profile for that:

+    6.41%  io_uring  [kernel.vmlinux]  [k] __blk_mq_alloc_requests
+    5.46%  io_uring  [kernel.vmlinux]  [k] blkdev_direct_IO.part.0
+    5.36%  io_uring  [kernel.vmlinux]  [k] blk_mq_end_request_batch
+    5.24%  io_uring  io_uring          [.] submitter_uring_fn
+    5.18%  io_uring  [kernel.vmlinux]  [k] io_submit_sqes
+    5.12%  io_uring  [kernel.vmlinux]  [k] bio_alloc_kiocb
+    4.75%  io_uring  [nvme]            [k] nvme_poll
+    4.67%  io_uring  [kernel.vmlinux]  [k] __io_import_iovec
+    4.58%  io_uring  [nvme]            [k] nvme_queue_rqs
+    4.49%  io_uring  [kernel.vmlinux]  [k] blk_mq_submit_bio
+    4.32%  io_uring  [nvme]            [k] nvme_map_data
+    3.02%  io_uring  [kernel.vmlinux]  [k] io_issue_sqe
+    2.89%  io_uring  [nvme_core]       [k] nvme_setup_cmd
+    2.60%  io_uring  [kernel.vmlinux]  [k] io_prep_rw
+    2.59%  io_uring  [kernel.vmlinux]  [k] submit_bio_noacct.part.0


-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/20/2021 4:19 PM, Jens Axboe wrote:
> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>
>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>
>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>
>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>
>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>> requests are issued.
>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>> the batching is directly driven by what the application is already
>>>>>>> doing.
>>>>>> I see. Thanks for the explanation.
>>>>>>
>>>>>> So it works only for io_uring based applications ?
>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>> many IOs will be submitted
>>>> Can you please share an example application (or is it fio patches) that
>>>> can submit batches ? The same that was used to test this patchset is
>>>> fine too.
>>>>
>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>> You should just be able to use iodepth_batch with fio. For my peak
>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>> and do batches of 32 for complete and submit. You can just run:
>>>
>>> t/io_uring <dev or file>
>>>
>>> maybe adding -p0 for IRQ driven rather than polled IO.
>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>> but it was never called using the t/io_uring test nor fio with
>> iodepth_batch=32 flag with io_uring engine.
>>
>> Any idea what might be the issue ?
>>
>> I installed fio from sources..
> The two main restrictions right now are a scheduler and shared tags, are
> you using any of those?

No.

But maybe I'm missing the .commit_rqs callback. is it mandatory for this 
feature ?

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/20/21 8:29 AM, Max Gurtovoy wrote:
> 
> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>
>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>
>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>
>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>> requests are issued.
>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>> doing.
>>>>>>> I see. Thanks for the explanation.
>>>>>>>
>>>>>>> So it works only for io_uring based applications ?
>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>> many IOs will be submitted
>>>>> Can you please share an example application (or is it fio patches) that
>>>>> can submit batches ? The same that was used to test this patchset is
>>>>> fine too.
>>>>>
>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>> and do batches of 32 for complete and submit. You can just run:
>>>>
>>>> t/io_uring <dev or file>
>>>>
>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>> but it was never called using the t/io_uring test nor fio with
>>> iodepth_batch=32 flag with io_uring engine.
>>>
>>> Any idea what might be the issue ?
>>>
>>> I installed fio from sources..
>> The two main restrictions right now are a scheduler and shared tags, are
>> you using any of those?
> 
> No.
> 
> But maybe I'm missing the .commit_rqs callback. is it mandatory for this 
> feature ?

I've only tested with nvme pci which does have it, but I don't think so.
Unless there's some check somewhere that makes it necessary. Can you
share the patch you're currently using on top?

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

[-- Attachment #1: Type: text/plain, Size: 7583 bytes --]


On 12/20/2021 6:34 PM, Jens Axboe wrote:
> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>
>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>
>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>> requests are issued.
>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>> doing.
>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>
>>>>>>>> So it works only for io_uring based applications ?
>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>> many IOs will be submitted
>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>> fine too.
>>>>>>
>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>
>>>>> t/io_uring <dev or file>
>>>>>
>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>> but it was never called using the t/io_uring test nor fio with
>>>> iodepth_batch=32 flag with io_uring engine.
>>>>
>>>> Any idea what might be the issue ?
>>>>
>>>> I installed fio from sources..
>>> The two main restrictions right now are a scheduler and shared tags, are
>>> you using any of those?
>> No.
>>
>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>> feature ?
> I've only tested with nvme pci which does have it, but I don't think so.
> Unless there's some check somewhere that makes it necessary. Can you
> share the patch you're currently using on top?

The attached POC patches apply cleanly on block/for-next branch

commit 7925bb75e8effa5de85b1cf8425cd5c21f212b1d (block/for-next)
Merge: eb12bde9eba8 3427f2b2c533
Author: Jens Axboe <axboe@kernel.dk>
Date:   Fri Dec 17 09:51:05 2021 -0700

     Merge branch 'for-5.17/drivers' into for-next

     * for-5.17/drivers:
       block: remove the rsxx driver
       rsxx: Drop PCI legacy power management
       mtip32xx: convert to generic power management
       mtip32xx: remove pointless drvdata lookups
       mtip32xx: remove pointless drvdata checking
       drbd: Use struct_group() to zero algs
       loop: make autoclear operation asynchronous
       null_blk: cast command status to integer
       pktdvd: stop using bdi congestion framework.


[-- Attachment #2: 0001-nvme-rdma-prepare-for-queue_rqs-implementation.patch --]
[-- Type: text/plain, Size: 6618 bytes --]

From 0de2836ce21df6801db580e154296544a741b6c4 Mon Sep 17 00:00:00 2001
From: Max Gurtovoy <mgurtovoy@nvidia.com>
Date: Thu, 2 Dec 2021 19:59:00 +0200
Subject: [PATCH 1/2] nvme-rdma: prepare for queue_rqs implementation

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/rdma.c | 127 ++++++++++++++++++++++++++-------------
 1 file changed, 84 insertions(+), 43 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 850f84d204d0..2d608cb48392 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -70,6 +70,7 @@ struct nvme_rdma_request {
 	struct ib_sge		sge[1 + NVME_RDMA_MAX_INLINE_SEGMENTS];
 	u32			num_sge;
 	struct ib_reg_wr	reg_wr;
+	struct ib_send_wr	send_wr;
 	struct ib_cqe		reg_cqe;
 	struct nvme_rdma_queue  *queue;
 	struct nvme_rdma_sgl	data_sgl;
@@ -1635,33 +1636,31 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc)
 		nvme_rdma_end_request(req);
 }
 
-static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
+static void nvme_rdma_prep_send(struct nvme_rdma_queue *queue,
 		struct nvme_rdma_qe *qe, struct ib_sge *sge, u32 num_sge,
-		struct ib_send_wr *first)
+		struct ib_send_wr *wr)
 {
-	struct ib_send_wr wr;
-	int ret;
-
-	sge->addr   = qe->dma;
+	sge->addr = qe->dma;
 	sge->length = sizeof(struct nvme_command);
-	sge->lkey   = queue->device->pd->local_dma_lkey;
+	sge->lkey = queue->device->pd->local_dma_lkey;
 
-	wr.next       = NULL;
-	wr.wr_cqe     = &qe->cqe;
-	wr.sg_list    = sge;
-	wr.num_sge    = num_sge;
-	wr.opcode     = IB_WR_SEND;
-	wr.send_flags = IB_SEND_SIGNALED;
+	wr->next = NULL;
+	wr->wr_cqe = &qe->cqe;
+	wr->sg_list = sge;
+	wr->num_sge = num_sge;
+	wr->opcode = IB_WR_SEND;
+	wr->send_flags = IB_SEND_SIGNALED;
+}
 
-	if (first)
-		first->next = &wr;
-	else
-		first = &wr;
+static int nvme_rdma_post_send(struct nvme_rdma_queue *queue,
+		struct ib_send_wr *wr)
+{
+	int ret;
 
-	ret = ib_post_send(queue->qp, first, NULL);
+	ret = ib_post_send(queue->qp, wr, NULL);
 	if (unlikely(ret)) {
 		dev_err(queue->ctrl->ctrl.device,
-			     "%s failed with error code %d\n", __func__, ret);
+			"%s failed with error code %d\n", __func__, ret);
 	}
 	return ret;
 }
@@ -1715,6 +1714,7 @@ static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg)
 	struct nvme_rdma_qe *sqe = &ctrl->async_event_sqe;
 	struct nvme_command *cmd = sqe->data;
 	struct ib_sge sge;
+	struct ib_send_wr wr;
 	int ret;
 
 	ib_dma_sync_single_for_cpu(dev, sqe->dma, sizeof(*cmd), DMA_TO_DEVICE);
@@ -1730,7 +1730,8 @@ static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg)
 	ib_dma_sync_single_for_device(dev, sqe->dma, sizeof(*cmd),
 			DMA_TO_DEVICE);
 
-	ret = nvme_rdma_post_send(queue, sqe, &sge, 1, NULL);
+	nvme_rdma_prep_send(queue, sqe, &sge, 1, &wr);
+	ret = nvme_rdma_post_send(queue, &wr);
 	WARN_ON_ONCE(ret);
 }
 
@@ -2034,27 +2035,35 @@ nvme_rdma_timeout(struct request *rq, bool reserved)
 	return BLK_EH_RESET_TIMER;
 }
 
-static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
-		const struct blk_mq_queue_data *bd)
+static blk_status_t nvme_rdma_cleanup_rq(struct nvme_rdma_queue *queue,
+		struct request *rq, int err)
 {
-	struct nvme_ns *ns = hctx->queue->queuedata;
-	struct nvme_rdma_queue *queue = hctx->driver_data;
-	struct request *rq = bd->rq;
+	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
+	blk_status_t ret;
+
+	nvme_rdma_unmap_data(queue, rq);
+	if (err == -EIO)
+		ret = nvme_host_path_error(rq);
+	else if (err == -ENOMEM || err == -EAGAIN)
+		ret = BLK_STS_RESOURCE;
+	else
+		ret = BLK_STS_IOERR;
+	nvme_cleanup_cmd(rq);
+	ib_dma_unmap_single(queue->device->dev, req->sqe.dma,
+			    sizeof(struct nvme_command), DMA_TO_DEVICE);
+	return ret;
+}
+
+static blk_status_t nvme_rdma_prep_rq(struct nvme_rdma_queue *queue,
+		struct request *rq, struct nvme_ns *ns)
+{
+	struct ib_device *dev = queue->device->dev;
 	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
 	struct nvme_rdma_qe *sqe = &req->sqe;
 	struct nvme_command *c = nvme_req(rq)->cmd;
-	struct ib_device *dev;
-	bool queue_ready = test_bit(NVME_RDMA_Q_LIVE, &queue->flags);
 	blk_status_t ret;
 	int err;
 
-	WARN_ON_ONCE(rq->tag < 0);
-
-	if (!nvme_check_ready(&queue->ctrl->ctrl, rq, queue_ready))
-		return nvme_fail_nonready_command(&queue->ctrl->ctrl, rq);
-
-	dev = queue->device->dev;
-
 	req->sqe.dma = ib_dma_map_single(dev, req->sqe.data,
 					 sizeof(struct nvme_command),
 					 DMA_TO_DEVICE);
@@ -2083,8 +2092,8 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	err = nvme_rdma_map_data(queue, rq, c);
 	if (unlikely(err < 0)) {
 		dev_err(queue->ctrl->ctrl.device,
-			     "Failed to map data (%d)\n", err);
-		goto err;
+			"Failed to map data (%d)\n", err);
+		goto out_err;
 	}
 
 	sqe->cqe.done = nvme_rdma_send_done;
@@ -2092,16 +2101,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	ib_dma_sync_single_for_device(dev, sqe->dma,
 			sizeof(struct nvme_command), DMA_TO_DEVICE);
 
-	err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
-			req->mr ? &req->reg_wr.wr : NULL);
-	if (unlikely(err))
-		goto err_unmap;
+	nvme_rdma_prep_send(queue, sqe, req->sge, req->num_sge, &req->send_wr);
+	if (req->mr)
+		req->reg_wr.wr.next = &req->send_wr;
 
 	return BLK_STS_OK;
 
-err_unmap:
-	nvme_rdma_unmap_data(queue, rq);
-err:
+out_err:
 	if (err == -EIO)
 		ret = nvme_host_path_error(rq);
 	else if (err == -ENOMEM || err == -EAGAIN)
@@ -2115,6 +2121,41 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return ret;
 }
 
+static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
+		const struct blk_mq_queue_data *bd)
+{
+	struct nvme_ns *ns = hctx->queue->queuedata;
+	struct nvme_rdma_queue *queue = hctx->driver_data;
+	struct request *rq = bd->rq;
+	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
+	bool queue_ready = test_bit(NVME_RDMA_Q_LIVE, &queue->flags);
+	struct ib_send_wr *wr;
+	int err;
+
+	WARN_ON_ONCE(rq->tag < 0);
+
+	if (!nvme_check_ready(&queue->ctrl->ctrl, rq, queue_ready))
+		return nvme_fail_nonready_command(&queue->ctrl->ctrl, rq);
+
+	err = nvme_rdma_prep_rq(queue, rq, ns);
+	if (unlikely(err))
+		return err;
+
+	if (req->mr)
+		wr = &req->reg_wr.wr;
+	else
+		wr = &req->send_wr;
+
+	err = nvme_rdma_post_send(queue, wr);
+	if (unlikely(err))
+		goto out_cleanup_rq;
+
+	return BLK_STS_OK;
+
+out_cleanup_rq:
+	return nvme_rdma_cleanup_rq(queue, rq, err);
+}
+
 static int nvme_rdma_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
 {
 	struct nvme_rdma_queue *queue = hctx->driver_data;
-- 
2.18.1


[-- Attachment #3: 0002-nvme-rdma-add-support-for-mq_ops-queue_rqs.patch --]
[-- Type: text/plain, Size: 2952 bytes --]

From 851a1f35420206f7b631d5d12b135e5a7c84b912 Mon Sep 17 00:00:00 2001
From: Max Gurtovoy <mgurtovoy@nvidia.com>
Date: Mon, 20 Dec 2021 20:42:49 +0200
Subject: [PATCH 2/2] nvme-rdma: add support for mq_ops->queue_rqs()

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/rdma.c | 75 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 2d608cb48392..765bb57f0a55 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2121,6 +2121,80 @@ static blk_status_t nvme_rdma_prep_rq(struct nvme_rdma_queue *queue,
 	return ret;
 }
 
+static bool nvme_rdma_prep_rq_batch(struct nvme_rdma_queue *queue,
+		struct request *rq)
+{
+	bool queue_ready = test_bit(NVME_RDMA_Q_LIVE, &queue->flags);
+
+	if (unlikely(!nvme_check_ready(&queue->ctrl->ctrl, rq, queue_ready)))
+		return false;
+
+	rq->mq_hctx->tags->rqs[rq->tag] = rq;
+	return nvme_rdma_prep_rq(queue, rq, rq->q->queuedata) == BLK_STS_OK;
+}
+
+static void nvme_rdma_submit_cmds(struct nvme_rdma_queue *queue,
+		struct request **rqlist)
+{
+	struct request *first_rq = rq_list_peek(rqlist);
+	struct nvme_rdma_request *nreq = blk_mq_rq_to_pdu(first_rq);
+	struct ib_send_wr *first, *last = NULL;
+	int ret;
+
+	if (nreq->mr)
+		first = &nreq->reg_wr.wr;
+	else
+		first = &nreq->send_wr;
+
+	while (!rq_list_empty(*rqlist)) {
+		struct request *rq = rq_list_pop(rqlist);
+		struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
+		struct ib_send_wr *tmp;
+
+		tmp = last;
+		last = &req->send_wr;
+		if (tmp) {
+			if (req->mr)
+				tmp->next = &req->reg_wr.wr;
+			else
+				tmp->next = &req->send_wr;
+		}
+	}
+
+	ret = nvme_rdma_post_send(queue, first);
+	WARN_ON_ONCE(ret);
+}
+
+static void nvme_rdma_queue_rqs(struct request **rqlist)
+{
+	struct request *req = rq_list_peek(rqlist), *prev = NULL;
+	struct request *requeue_list = NULL;
+
+	do {
+		struct nvme_rdma_queue *queue = req->mq_hctx->driver_data;
+
+		if (!nvme_rdma_prep_rq_batch(queue, req)) {
+			/* detach 'req' and add to remainder list */
+			if (prev)
+				prev->rq_next = req->rq_next;
+			rq_list_add(&requeue_list, req);
+		} else {
+			prev = req;
+		}
+
+		req = rq_list_next(req);
+		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
+			/* detach rest of list, and submit */
+			if (prev)
+				prev->rq_next = NULL;
+			nvme_rdma_submit_cmds(queue, rqlist);
+			*rqlist = req;
+		}
+	} while (req);
+
+	*rqlist = requeue_list;
+}
+
 static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 		const struct blk_mq_queue_data *bd)
 {
@@ -2258,6 +2332,7 @@ static int nvme_rdma_map_queues(struct blk_mq_tag_set *set)
 
 static const struct blk_mq_ops nvme_rdma_mq_ops = {
 	.queue_rq	= nvme_rdma_queue_rq,
+	.queue_rqs	= nvme_rdma_queue_rqs,
 	.complete	= nvme_rdma_complete_rq,
 	.init_request	= nvme_rdma_init_request,
 	.exit_request	= nvme_rdma_exit_request,
-- 
2.18.1

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/20/21 11:48 AM, Max Gurtovoy wrote:
> 
> On 12/20/2021 6:34 PM, Jens Axboe wrote:
>> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>>
>>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>>> requests are issued.
>>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>>> doing.
>>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>>
>>>>>>>>> So it works only for io_uring based applications ?
>>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>>> many IOs will be submitted
>>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>>> fine too.
>>>>>>>
>>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>>
>>>>>> t/io_uring <dev or file>
>>>>>>
>>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>>> but it was never called using the t/io_uring test nor fio with
>>>>> iodepth_batch=32 flag with io_uring engine.
>>>>>
>>>>> Any idea what might be the issue ?
>>>>>
>>>>> I installed fio from sources..
>>>> The two main restrictions right now are a scheduler and shared tags, are
>>>> you using any of those?
>>> No.
>>>
>>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>>> feature ?
>> I've only tested with nvme pci which does have it, but I don't think so.
>> Unless there's some check somewhere that makes it necessary. Can you
>> share the patch you're currently using on top?
> 
> The attached POC patches apply cleanly on block/for-next branch

Looks reasonable to me from a quick glance. Not sure why you're not
seeing it hit, maybe try and instrument
block/blk-mq.c:blk_mq_flush_plug_list() and find out why it isn't being
called? As mentioned, no elevator or shared tags, should work for
anything else basically.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/20/2021 8:58 PM, Jens Axboe wrote:
> On 12/20/21 11:48 AM, Max Gurtovoy wrote:
>> On 12/20/2021 6:34 PM, Jens Axboe wrote:
>>> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>>>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>>>> requests are issued.
>>>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>>>> doing.
>>>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>>>
>>>>>>>>>> So it works only for io_uring based applications ?
>>>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>>>> many IOs will be submitted
>>>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>>>> fine too.
>>>>>>>>
>>>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>>>
>>>>>>> t/io_uring <dev or file>
>>>>>>>
>>>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>>>> but it was never called using the t/io_uring test nor fio with
>>>>>> iodepth_batch=32 flag with io_uring engine.
>>>>>>
>>>>>> Any idea what might be the issue ?
>>>>>>
>>>>>> I installed fio from sources..
>>>>> The two main restrictions right now are a scheduler and shared tags, are
>>>>> you using any of those?
>>>> No.
>>>>
>>>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>>>> feature ?
>>> I've only tested with nvme pci which does have it, but I don't think so.
>>> Unless there's some check somewhere that makes it necessary. Can you
>>> share the patch you're currently using on top?
>> The attached POC patches apply cleanly on block/for-next branch
> Looks reasonable to me from a quick glance. Not sure why you're not
> seeing it hit, maybe try and instrument
> block/blk-mq.c:blk_mq_flush_plug_list() and find out why it isn't being
> called? As mentioned, no elevator or shared tags, should work for
> anything else basically.

Yes. I saw that the blk layer converted the original non-shared tagset 
of NVMe/RDMA to a shared one because of the nvmf connect request queue 
that is using the same tagset (uses only the reserved tag).

So I guess this is the reason that the I couldn't reach the new code of 
queue_rqs.

The question is how we can overcome this ?

Should we create new tagset for the NVMf fabrics connect_q ? or maybe 
not mark the tagset as shared for reserved ids ?

Christoph, any suggestion here ?

>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/21/21 3:20 AM, Max Gurtovoy wrote:
> 
> On 12/20/2021 8:58 PM, Jens Axboe wrote:
>> On 12/20/21 11:48 AM, Max Gurtovoy wrote:
>>> On 12/20/2021 6:34 PM, Jens Axboe wrote:
>>>> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>>>>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>>>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>>>>> requests are issued.
>>>>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>>>>> doing.
>>>>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>>>>
>>>>>>>>>>> So it works only for io_uring based applications ?
>>>>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>>>>> many IOs will be submitted
>>>>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>>>>> fine too.
>>>>>>>>>
>>>>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>>>>
>>>>>>>> t/io_uring <dev or file>
>>>>>>>>
>>>>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>>>>> but it was never called using the t/io_uring test nor fio with
>>>>>>> iodepth_batch=32 flag with io_uring engine.
>>>>>>>
>>>>>>> Any idea what might be the issue ?
>>>>>>>
>>>>>>> I installed fio from sources..
>>>>>> The two main restrictions right now are a scheduler and shared tags, are
>>>>>> you using any of those?
>>>>> No.
>>>>>
>>>>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>>>>> feature ?
>>>> I've only tested with nvme pci which does have it, but I don't think so.
>>>> Unless there's some check somewhere that makes it necessary. Can you
>>>> share the patch you're currently using on top?
>>> The attached POC patches apply cleanly on block/for-next branch
>> Looks reasonable to me from a quick glance. Not sure why you're not
>> seeing it hit, maybe try and instrument
>> block/blk-mq.c:blk_mq_flush_plug_list() and find out why it isn't being
>> called? As mentioned, no elevator or shared tags, should work for
>> anything else basically.
> 
> Yes. I saw that the blk layer converted the original non-shared tagset 
> of NVMe/RDMA to a shared one because of the nvmf connect request queue 
> that is using the same tagset (uses only the reserved tag).
> 
> So I guess this is the reason that the I couldn't reach the new code of 
> queue_rqs.
> 
> The question is how we can overcome this ?

Do we need to mark it shared for just the reserved tags? I wouldn't
think so...

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/21/2021 5:23 PM, Jens Axboe wrote:
> On 12/21/21 3:20 AM, Max Gurtovoy wrote:
>> On 12/20/2021 8:58 PM, Jens Axboe wrote:
>>> On 12/20/21 11:48 AM, Max Gurtovoy wrote:
>>>> On 12/20/2021 6:34 PM, Jens Axboe wrote:
>>>>> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>>>>>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>>>>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>>>>>> requests are issued.
>>>>>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>>>>>> doing.
>>>>>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>>>>>
>>>>>>>>>>>> So it works only for io_uring based applications ?
>>>>>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>>>>>> many IOs will be submitted
>>>>>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>>>>>> fine too.
>>>>>>>>>>
>>>>>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>>>>>
>>>>>>>>> t/io_uring <dev or file>
>>>>>>>>>
>>>>>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>>>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>>>>>> but it was never called using the t/io_uring test nor fio with
>>>>>>>> iodepth_batch=32 flag with io_uring engine.
>>>>>>>>
>>>>>>>> Any idea what might be the issue ?
>>>>>>>>
>>>>>>>> I installed fio from sources..
>>>>>>> The two main restrictions right now are a scheduler and shared tags, are
>>>>>>> you using any of those?
>>>>>> No.
>>>>>>
>>>>>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>>>>>> feature ?
>>>>> I've only tested with nvme pci which does have it, but I don't think so.
>>>>> Unless there's some check somewhere that makes it necessary. Can you
>>>>> share the patch you're currently using on top?
>>>> The attached POC patches apply cleanly on block/for-next branch
>>> Looks reasonable to me from a quick glance. Not sure why you're not
>>> seeing it hit, maybe try and instrument
>>> block/blk-mq.c:blk_mq_flush_plug_list() and find out why it isn't being
>>> called? As mentioned, no elevator or shared tags, should work for
>>> anything else basically.
>> Yes. I saw that the blk layer converted the original non-shared tagset
>> of NVMe/RDMA to a shared one because of the nvmf connect request queue
>> that is using the same tagset (uses only the reserved tag).
>>
>> So I guess this is the reason that the I couldn't reach the new code of
>> queue_rqs.
>>
>> The question is how we can overcome this ?
> Do we need to mark it shared for just the reserved tags? I wouldn't
> think so...

We don't mark it. The block layer does it in blk_mq_add_queue_tag_set:

if (!list_empty(&set->tag_list) &&
             !(set->flags & BLK_MQ_F_TAG_QUEUE_SHARED))

>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/21/21 8:29 AM, Max Gurtovoy wrote:
> 
> On 12/21/2021 5:23 PM, Jens Axboe wrote:
>> On 12/21/21 3:20 AM, Max Gurtovoy wrote:
>>> On 12/20/2021 8:58 PM, Jens Axboe wrote:
>>>> On 12/20/21 11:48 AM, Max Gurtovoy wrote:
>>>>> On 12/20/2021 6:34 PM, Jens Axboe wrote:
>>>>>> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>>>>>>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>>>>>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>>>>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>>>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>>>>>>> requests are issued.
>>>>>>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>>>>>>> doing.
>>>>>>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So it works only for io_uring based applications ?
>>>>>>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>>>>>>> many IOs will be submitted
>>>>>>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>>>>>>> fine too.
>>>>>>>>>>>
>>>>>>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>>>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>>>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>>>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>>>>>>
>>>>>>>>>> t/io_uring <dev or file>
>>>>>>>>>>
>>>>>>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>>>>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>>>>>>> but it was never called using the t/io_uring test nor fio with
>>>>>>>>> iodepth_batch=32 flag with io_uring engine.
>>>>>>>>>
>>>>>>>>> Any idea what might be the issue ?
>>>>>>>>>
>>>>>>>>> I installed fio from sources..
>>>>>>>> The two main restrictions right now are a scheduler and shared tags, are
>>>>>>>> you using any of those?
>>>>>>> No.
>>>>>>>
>>>>>>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>>>>>>> feature ?
>>>>>> I've only tested with nvme pci which does have it, but I don't think so.
>>>>>> Unless there's some check somewhere that makes it necessary. Can you
>>>>>> share the patch you're currently using on top?
>>>>> The attached POC patches apply cleanly on block/for-next branch
>>>> Looks reasonable to me from a quick glance. Not sure why you're not
>>>> seeing it hit, maybe try and instrument
>>>> block/blk-mq.c:blk_mq_flush_plug_list() and find out why it isn't being
>>>> called? As mentioned, no elevator or shared tags, should work for
>>>> anything else basically.
>>> Yes. I saw that the blk layer converted the original non-shared tagset
>>> of NVMe/RDMA to a shared one because of the nvmf connect request queue
>>> that is using the same tagset (uses only the reserved tag).
>>>
>>> So I guess this is the reason that the I couldn't reach the new code of
>>> queue_rqs.
>>>
>>> The question is how we can overcome this ?
>> Do we need to mark it shared for just the reserved tags? I wouldn't
>> think so...
> 
> We don't mark it. The block layer does it in blk_mq_add_queue_tag_set:
> 
> if (!list_empty(&set->tag_list) &&
>              !(set->flags & BLK_MQ_F_TAG_QUEUE_SHARED))

Yes, that's what I meant, do we need to mark it as such for just the
reserved tags?

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/21/2021 5:33 PM, Jens Axboe wrote:
> On 12/21/21 8:29 AM, Max Gurtovoy wrote:
>> On 12/21/2021 5:23 PM, Jens Axboe wrote:
>>> On 12/21/21 3:20 AM, Max Gurtovoy wrote:
>>>> On 12/20/2021 8:58 PM, Jens Axboe wrote:
>>>>> On 12/20/21 11:48 AM, Max Gurtovoy wrote:
>>>>>> On 12/20/2021 6:34 PM, Jens Axboe wrote:
>>>>>>> On 12/20/21 8:29 AM, Max Gurtovoy wrote:
>>>>>>>> On 12/20/2021 4:19 PM, Jens Axboe wrote:
>>>>>>>>> On 12/20/21 3:11 AM, Max Gurtovoy wrote:
>>>>>>>>>> On 12/19/2021 4:48 PM, Jens Axboe wrote:
>>>>>>>>>>> On 12/19/21 5:14 AM, Max Gurtovoy wrote:
>>>>>>>>>>>> On 12/16/2021 7:16 PM, Jens Axboe wrote:
>>>>>>>>>>>>> On 12/16/21 9:57 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>> On 12/16/2021 6:36 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>> On 12/16/21 9:34 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>> On 12/16/2021 6:25 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>> On 12/16/21 9:19 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>> On 12/16/2021 6:05 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>> On 12/16/21 9:00 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>>>> On 12/16/2021 5:48 PM, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>>> On 12/16/21 6:06 AM, Max Gurtovoy wrote:
>>>>>>>>>>>>>>>>>>>>>> On 12/16/2021 11:08 AM, Christoph Hellwig wrote:
>>>>>>>>>>>>>>>>>>>>>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>>>>>>>>>>>>>>>>>>>>>> +	spin_lock(&nvmeq->sq_lock);
>>>>>>>>>>>>>>>>>>>>>>>> +	while (!rq_list_empty(*rqlist)) {
>>>>>>>>>>>>>>>>>>>>>>>> +		struct request *req = rq_list_pop(rqlist);
>>>>>>>>>>>>>>>>>>>>>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>>>>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>>>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>>>>>>>>>>>>>>>>>>>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>>>>>>>>>>>>>>>>>>>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>>>>>>>>>>>>>>>>>>>>>> +			nvmeq->sq_tail = 0;
>>>>>>>>>>>>>>>>>>>>>>> So this doesn't even use the new helper added in patch 2?  I think this
>>>>>>>>>>>>>>>>>>>>>>> should call nvme_sq_copy_cmd().
>>>>>>>>>>>>>>>>>>>>>> I also noticed that.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> So need to decide if to open code it or use the helper function.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Inline helper sounds reasonable if you have 3 places that will use it.
>>>>>>>>>>>>>>>>>>>>> Yes agree, that's been my stance too :-)
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> The rest looks identical to the incremental patch I posted, so I guess
>>>>>>>>>>>>>>>>>>>>>>> the performance degration measured on the first try was a measurement
>>>>>>>>>>>>>>>>>>>>>>> error?
>>>>>>>>>>>>>>>>>>>>>> giving 1 dbr for a batch of N commands sounds good idea. Also for RDMA host.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> But how do you moderate it ? what is the batch_sz <--> time_to_wait
>>>>>>>>>>>>>>>>>>>>>> algorithm ?
>>>>>>>>>>>>>>>>>>>>> The batching is naturally limited at BLK_MAX_REQUEST_COUNT, which is 32
>>>>>>>>>>>>>>>>>>>>> in total. I do agree that if we ever made it much larger, then we might
>>>>>>>>>>>>>>>>>>>>> want to cap it differently. But 32 seems like a pretty reasonable number
>>>>>>>>>>>>>>>>>>>>> to get enough gain from the batching done in various areas, while still
>>>>>>>>>>>>>>>>>>>>> not making it so large that we have a potential latency issue. That
>>>>>>>>>>>>>>>>>>>>> batch count is already used consistently for other items too (like tag
>>>>>>>>>>>>>>>>>>>>> allocation), so it's not specific to just this one case.
>>>>>>>>>>>>>>>>>>>> I'm saying that the you can wait to the batch_max_count too long and it
>>>>>>>>>>>>>>>>>>>> won't be efficient from latency POV.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> So it's better to limit the block layar to wait for the first to come: x
>>>>>>>>>>>>>>>>>>>> usecs or batch_max_count before issue queue_rqs.
>>>>>>>>>>>>>>>>>>> There's no waiting specifically for this, it's just based on the plug.
>>>>>>>>>>>>>>>>>>> We just won't do more than 32 in that plug. This is really just an
>>>>>>>>>>>>>>>>>>> artifact of the plugging, and if that should be limited based on "max of
>>>>>>>>>>>>>>>>>>> 32 or xx time", then that should be done there.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> But in general I think it's saner and enough to just limit the total
>>>>>>>>>>>>>>>>>>> size. If we spend more than xx usec building up the plug list, we're
>>>>>>>>>>>>>>>>>>> doing something horribly wrong. That really should not happen with 32
>>>>>>>>>>>>>>>>>>> requests, and we'll never eg wait on requests if we're out of tags. That
>>>>>>>>>>>>>>>>>>> will result in a plug flush to begin with.
>>>>>>>>>>>>>>>>>> I'm not aware of the plug. I hope to get to it soon.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> My concern is if the user application submitted only 28 requests and
>>>>>>>>>>>>>>>>>> then you'll wait forever ? or for very long time.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I guess not, but I'm asking how do you know how to batch and when to
>>>>>>>>>>>>>>>>>> stop in case 32 commands won't arrive anytime soon.
>>>>>>>>>>>>>>>>> The plug is in the stack of the task, so that condition can never
>>>>>>>>>>>>>>>>> happen. If the application originally asks for 32 but then only submits
>>>>>>>>>>>>>>>>> 28, then once that last one is submitted the plug is flushed and
>>>>>>>>>>>>>>>>> requests are issued.
>>>>>>>>>>>>>>>> So if I'm running fio with --iodepth=28 what will plug do ? send batches
>>>>>>>>>>>>>>>> of 28 ? or 1 by 1 ?
>>>>>>>>>>>>>>> --iodepth just controls the overall depth, the batch submit count
>>>>>>>>>>>>>>> dictates what happens further down. If you run queue depth 28 and submit
>>>>>>>>>>>>>>> one at the time, then you'll get one at the time further down too. Hence
>>>>>>>>>>>>>>> the batching is directly driven by what the application is already
>>>>>>>>>>>>>>> doing.
>>>>>>>>>>>>>> I see. Thanks for the explanation.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So it works only for io_uring based applications ?
>>>>>>>>>>>>> It's only enabled for io_uring right now, but it's generically available
>>>>>>>>>>>>> for anyone that wants to use it... Would be trivial to do for aio, and
>>>>>>>>>>>>> other spots that currently use blk_start_plug() and has an idea of how
>>>>>>>>>>>>> many IOs will be submitted
>>>>>>>>>>>> Can you please share an example application (or is it fio patches) that
>>>>>>>>>>>> can submit batches ? The same that was used to test this patchset is
>>>>>>>>>>>> fine too.
>>>>>>>>>>>>
>>>>>>>>>>>> I would like to test it with our NVMe SNAP controllers and also to
>>>>>>>>>>>> develop NVMe/RDMA queue_rqs code and test the perf with it.
>>>>>>>>>>> You should just be able to use iodepth_batch with fio. For my peak
>>>>>>>>>>> testing, I use t/io_uring from the fio repo. By default, it'll run QD of
>>>>>>>>>>> and do batches of 32 for complete and submit. You can just run:
>>>>>>>>>>>
>>>>>>>>>>> t/io_uring <dev or file>
>>>>>>>>>>>
>>>>>>>>>>> maybe adding -p0 for IRQ driven rather than polled IO.
>>>>>>>>>> I used your block/for-next branch and implemented queue_rqs in NVMe/RDMA
>>>>>>>>>> but it was never called using the t/io_uring test nor fio with
>>>>>>>>>> iodepth_batch=32 flag with io_uring engine.
>>>>>>>>>>
>>>>>>>>>> Any idea what might be the issue ?
>>>>>>>>>>
>>>>>>>>>> I installed fio from sources..
>>>>>>>>> The two main restrictions right now are a scheduler and shared tags, are
>>>>>>>>> you using any of those?
>>>>>>>> No.
>>>>>>>>
>>>>>>>> But maybe I'm missing the .commit_rqs callback. is it mandatory for this
>>>>>>>> feature ?
>>>>>>> I've only tested with nvme pci which does have it, but I don't think so.
>>>>>>> Unless there's some check somewhere that makes it necessary. Can you
>>>>>>> share the patch you're currently using on top?
>>>>>> The attached POC patches apply cleanly on block/for-next branch
>>>>> Looks reasonable to me from a quick glance. Not sure why you're not
>>>>> seeing it hit, maybe try and instrument
>>>>> block/blk-mq.c:blk_mq_flush_plug_list() and find out why it isn't being
>>>>> called? As mentioned, no elevator or shared tags, should work for
>>>>> anything else basically.
>>>> Yes. I saw that the blk layer converted the original non-shared tagset
>>>> of NVMe/RDMA to a shared one because of the nvmf connect request queue
>>>> that is using the same tagset (uses only the reserved tag).
>>>>
>>>> So I guess this is the reason that the I couldn't reach the new code of
>>>> queue_rqs.
>>>>
>>>> The question is how we can overcome this ?
>>> Do we need to mark it shared for just the reserved tags? I wouldn't
>>> think so...
>> We don't mark it. The block layer does it in blk_mq_add_queue_tag_set:
>>
>> if (!list_empty(&set->tag_list) &&
>>               !(set->flags & BLK_MQ_F_TAG_QUEUE_SHARED))
> Yes, that's what I meant, do we need to mark it as such for just the
> reserved tags?

I'm afraid it doesn't related only to reserved tags.

If you have nvme device with 2 namespaces it will get to this code and 
mark it as shared set. And then the queue_rqs() won't be called for NVMe 
PCI as well.


>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 2:08 AM, Christoph Hellwig wrote:
> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>> +	spin_lock(&nvmeq->sq_lock);
>> +	while (!rq_list_empty(*rqlist)) {
>> +		struct request *req = rq_list_pop(rqlist);
>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>> +
>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>> +			nvmeq->sq_tail = 0;
> 
> So this doesn't even use the new helper added in patch 2?  I think this
> should call nvme_sq_copy_cmd().

But you NAK'ed that one? It definitely should use that helper, so I take it
you are fine with it then if we do it here too? That would make 3 call sites,
and I still do think the helper makes sense...

> The rest looks identical to the incremental patch I posted, so I guess
> the performance degration measured on the first try was a measurement
> error?

It may have been a measurement error, I'm honestly not quite sure. I
reshuffled and modified a few bits here and there, and verified the
end result. Wish I had a better answer, but...

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Christoph Hellwig]

On Thu, Dec 16, 2021 at 08:45:46AM -0700, Jens Axboe wrote:
> On 12/16/21 2:08 AM, Christoph Hellwig wrote:
> > On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
> >> +	spin_lock(&nvmeq->sq_lock);
> >> +	while (!rq_list_empty(*rqlist)) {
> >> +		struct request *req = rq_list_pop(rqlist);
> >> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> >> +
> >> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> >> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
> >> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
> >> +			nvmeq->sq_tail = 0;
> > 
> > So this doesn't even use the new helper added in patch 2?  I think this
> > should call nvme_sq_copy_cmd().
> 
> But you NAK'ed that one? It definitely should use that helper, so I take it
> you are fine with it then if we do it here too? That would make 3 call sites,
> and I still do think the helper makes sense...

I explained two times that the new helpers is fine as long as you open
code nvme_submit_cmd in its two callers as it now is a trivial wrapper.

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:15 AM, Christoph Hellwig wrote:
> On Thu, Dec 16, 2021 at 08:45:46AM -0700, Jens Axboe wrote:
>> On 12/16/21 2:08 AM, Christoph Hellwig wrote:
>>> On Wed, Dec 15, 2021 at 09:24:21AM -0700, Jens Axboe wrote:
>>>> +	spin_lock(&nvmeq->sq_lock);
>>>> +	while (!rq_list_empty(*rqlist)) {
>>>> +		struct request *req = rq_list_pop(rqlist);
>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>> +
>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>> +			nvmeq->sq_tail = 0;
>>>
>>> So this doesn't even use the new helper added in patch 2?  I think this
>>> should call nvme_sq_copy_cmd().
>>
>> But you NAK'ed that one? It definitely should use that helper, so I take it
>> you are fine with it then if we do it here too? That would make 3 call sites,
>> and I still do think the helper makes sense...
> 
> I explained two times that the new helpers is fine as long as you open
> code nvme_submit_cmd in its two callers as it now is a trivial wrapper.

OK, I misunderstood which one you referred to then. So this incremental,
I'll send out a new series...


diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 58d97660374a..51a903d91d92 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -509,21 +509,6 @@ static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
 		nvmeq->sq_tail = 0;
 }
 
-/**
- * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
- * @nvmeq: The queue to use
- * @cmd: The command to send
- * @write_sq: whether to write to the SQ doorbell
- */
-static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
-			    bool write_sq)
-{
-	spin_lock(&nvmeq->sq_lock);
-	nvme_sq_copy_cmd(nvmeq, cmd);
-	nvme_write_sq_db(nvmeq, write_sq);
-	spin_unlock(&nvmeq->sq_lock);
-}
-
 static void nvme_commit_rqs(struct blk_mq_hw_ctx *hctx)
 {
 	struct nvme_queue *nvmeq = hctx->driver_data;
@@ -977,7 +962,10 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	ret = nvme_prep_rq(dev, req);
 	if (unlikely(ret))
 		return ret;
-	nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
+	spin_lock(&nvmeq->sq_lock);
+	nvme_sq_copy_cmd(nvmeq, &iod->cmd);
+	nvme_write_sq_db(nvmeq, bd->last);
+	spin_unlock(&nvmeq->sq_lock);
 	return BLK_STS_OK;
 }
 
@@ -1213,7 +1201,11 @@ static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
 
 	c.common.opcode = nvme_admin_async_event;
 	c.common.command_id = NVME_AQ_BLK_MQ_DEPTH;
-	nvme_submit_cmd(nvmeq, &c, true);
+
+	spin_lock(&nvmeq->sq_lock);
+	nvme_sq_copy_cmd(nvmeq, &c);
+	nvme_write_sq_db(nvmeq, true);
+	spin_unlock(&nvmeq->sq_lock);
 }
 
 static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Christoph Hellwig]

On Thu, Dec 16, 2021 at 09:27:18AM -0700, Jens Axboe wrote:
> OK, I misunderstood which one you referred to then. So this incremental,

Yes, that's the preferred version.

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:30 AM, Christoph Hellwig wrote:
> On Thu, Dec 16, 2021 at 09:27:18AM -0700, Jens Axboe wrote:
>> OK, I misunderstood which one you referred to then. So this incremental,
> 
> Yes, that's the preferred version.

Respun it, will send out an updated one.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/15/2021 6:24 PM, Jens Axboe wrote:
> This enables the block layer to send us a full plug list of requests
> that need submitting. The block layer guarantees that they all belong
> to the same queue, but we do have to check the hardware queue mapping
> for each request.
>
> If errors are encountered, leave them in the passed in list. Then the
> block layer will handle them individually.
>
> This is good for about a 4% improvement in peak performance, taking us
> from 9.6M to 10M IOPS/core.
>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   drivers/nvme/host/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 61 insertions(+)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 6be6b1ab4285..197aa45ef7ef 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -981,6 +981,66 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>   	return BLK_STS_OK;
>   }
>   
> +static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
> +{
> +	spin_lock(&nvmeq->sq_lock);
> +	while (!rq_list_empty(*rqlist)) {
> +		struct request *req = rq_list_pop(rqlist);
> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> +
> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
> +			nvmeq->sq_tail = 0;
> +	}
> +	nvme_write_sq_db(nvmeq, true);
> +	spin_unlock(&nvmeq->sq_lock);
> +}
> +
> +static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
> +{
> +	/*
> +	 * We should not need to do this, but we're still using this to
> +	 * ensure we can drain requests on a dying queue.
> +	 */
> +	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
> +		return false;
> +	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
> +		return false;
> +
> +	req->mq_hctx->tags->rqs[req->tag] = req;
> +	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
> +}
> +
> +static void nvme_queue_rqs(struct request **rqlist)
> +{
> +	struct request *req = rq_list_peek(rqlist), *prev = NULL;
> +	struct request *requeue_list = NULL;
> +
> +	do {
> +		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
> +
> +		if (!nvme_prep_rq_batch(nvmeq, req)) {
> +			/* detach 'req' and add to remainder list */
> +			if (prev)
> +				prev->rq_next = req->rq_next;
> +			rq_list_add(&requeue_list, req);
> +		} else {
> +			prev = req;
> +		}
> +
> +		req = rq_list_next(req);
> +		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
> +			/* detach rest of list, and submit */
> +			prev->rq_next = NULL;

if req == NULL and prev == NULL we'll get a NULL deref here.

I think this can happen in the first iteration.

Correct me if I'm wrong..

> +			nvme_submit_cmds(nvmeq, rqlist);
> +			*rqlist = req;
> +		}
> +	} while (req);
> +
> +	*rqlist = requeue_list;
> +}
> +
>   static __always_inline void nvme_pci_unmap_rq(struct request *req)
>   {
>   	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> @@ -1678,6 +1738,7 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
>   
>   static const struct blk_mq_ops nvme_mq_ops = {
>   	.queue_rq	= nvme_queue_rq,
> +	.queue_rqs	= nvme_queue_rqs,
>   	.complete	= nvme_pci_complete_rq,
>   	.commit_rqs	= nvme_commit_rqs,
>   	.init_hctx	= nvme_init_hctx,

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 6:02 AM, Max Gurtovoy wrote:
> 
> On 12/15/2021 6:24 PM, Jens Axboe wrote:
>> This enables the block layer to send us a full plug list of requests
>> that need submitting. The block layer guarantees that they all belong
>> to the same queue, but we do have to check the hardware queue mapping
>> for each request.
>>
>> If errors are encountered, leave them in the passed in list. Then the
>> block layer will handle them individually.
>>
>> This is good for about a 4% improvement in peak performance, taking us
>> from 9.6M to 10M IOPS/core.
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   drivers/nvme/host/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 6be6b1ab4285..197aa45ef7ef 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -981,6 +981,66 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>>   	return BLK_STS_OK;
>>   }
>>   
>> +static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
>> +{
>> +	spin_lock(&nvmeq->sq_lock);
>> +	while (!rq_list_empty(*rqlist)) {
>> +		struct request *req = rq_list_pop(rqlist);
>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>> +
>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>> +			nvmeq->sq_tail = 0;
>> +	}
>> +	nvme_write_sq_db(nvmeq, true);
>> +	spin_unlock(&nvmeq->sq_lock);
>> +}
>> +
>> +static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
>> +{
>> +	/*
>> +	 * We should not need to do this, but we're still using this to
>> +	 * ensure we can drain requests on a dying queue.
>> +	 */
>> +	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
>> +		return false;
>> +	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
>> +		return false;
>> +
>> +	req->mq_hctx->tags->rqs[req->tag] = req;
>> +	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
>> +}
>> +
>> +static void nvme_queue_rqs(struct request **rqlist)
>> +{
>> +	struct request *req = rq_list_peek(rqlist), *prev = NULL;
>> +	struct request *requeue_list = NULL;
>> +
>> +	do {
>> +		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
>> +
>> +		if (!nvme_prep_rq_batch(nvmeq, req)) {
>> +			/* detach 'req' and add to remainder list */
>> +			if (prev)
>> +				prev->rq_next = req->rq_next;
>> +			rq_list_add(&requeue_list, req);
>> +		} else {
>> +			prev = req;
>> +		}
>> +
>> +		req = rq_list_next(req);
>> +		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
>> +			/* detach rest of list, and submit */
>> +			prev->rq_next = NULL;
> 
> if req == NULL and prev == NULL we'll get a NULL deref here.
> 
> I think this can happen in the first iteration.
> 
> Correct me if I'm wrong..

First iteration we know the list isn't empty, so req can't be NULL
there.

-- 
Jens Axboe

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Max Gurtovoy]

On 12/16/2021 5:59 PM, Jens Axboe wrote:
> On 12/16/21 6:02 AM, Max Gurtovoy wrote:
>> On 12/15/2021 6:24 PM, Jens Axboe wrote:
>>> This enables the block layer to send us a full plug list of requests
>>> that need submitting. The block layer guarantees that they all belong
>>> to the same queue, but we do have to check the hardware queue mapping
>>> for each request.
>>>
>>> If errors are encountered, leave them in the passed in list. Then the
>>> block layer will handle them individually.
>>>
>>> This is good for about a 4% improvement in peak performance, taking us
>>> from 9.6M to 10M IOPS/core.
>>>
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>    drivers/nvme/host/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>>> index 6be6b1ab4285..197aa45ef7ef 100644
>>> --- a/drivers/nvme/host/pci.c
>>> +++ b/drivers/nvme/host/pci.c
>>> @@ -981,6 +981,66 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>    	return BLK_STS_OK;
>>>    }
>>>    
>>> +static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
>>> +{
>>> +	spin_lock(&nvmeq->sq_lock);
>>> +	while (!rq_list_empty(*rqlist)) {
>>> +		struct request *req = rq_list_pop(rqlist);
>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>> +
>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>> +			nvmeq->sq_tail = 0;
>>> +	}
>>> +	nvme_write_sq_db(nvmeq, true);
>>> +	spin_unlock(&nvmeq->sq_lock);
>>> +}
>>> +
>>> +static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
>>> +{
>>> +	/*
>>> +	 * We should not need to do this, but we're still using this to
>>> +	 * ensure we can drain requests on a dying queue.
>>> +	 */
>>> +	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
>>> +		return false;
>>> +	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
>>> +		return false;
>>> +
>>> +	req->mq_hctx->tags->rqs[req->tag] = req;
>>> +	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
>>> +}
>>> +
>>> +static void nvme_queue_rqs(struct request **rqlist)
>>> +{
>>> +	struct request *req = rq_list_peek(rqlist), *prev = NULL;
>>> +	struct request *requeue_list = NULL;
>>> +
>>> +	do {
>>> +		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
>>> +
>>> +		if (!nvme_prep_rq_batch(nvmeq, req)) {
>>> +			/* detach 'req' and add to remainder list */
>>> +			if (prev)
>>> +				prev->rq_next = req->rq_next;
>>> +			rq_list_add(&requeue_list, req);
>>> +		} else {
>>> +			prev = req;
>>> +		}
>>> +
>>> +		req = rq_list_next(req);
>>> +		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
>>> +			/* detach rest of list, and submit */
>>> +			prev->rq_next = NULL;
>> if req == NULL and prev == NULL we'll get a NULL deref here.
>>
>> I think this can happen in the first iteration.
>>
>> Correct me if I'm wrong..
> First iteration we know the list isn't empty, so req can't be NULL
> there.

but you set "req = rq_list_next(req);"

So can't req be NULL ? after the above line ?

>

Re: [PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

On 12/16/21 9:06 AM, Max Gurtovoy wrote:
> 
> On 12/16/2021 5:59 PM, Jens Axboe wrote:
>> On 12/16/21 6:02 AM, Max Gurtovoy wrote:
>>> On 12/15/2021 6:24 PM, Jens Axboe wrote:
>>>> This enables the block layer to send us a full plug list of requests
>>>> that need submitting. The block layer guarantees that they all belong
>>>> to the same queue, but we do have to check the hardware queue mapping
>>>> for each request.
>>>>
>>>> If errors are encountered, leave them in the passed in list. Then the
>>>> block layer will handle them individually.
>>>>
>>>> This is good for about a 4% improvement in peak performance, taking us
>>>> from 9.6M to 10M IOPS/core.
>>>>
>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>    drivers/nvme/host/pci.c | 61 +++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 61 insertions(+)
>>>>
>>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>>>> index 6be6b1ab4285..197aa45ef7ef 100644
>>>> --- a/drivers/nvme/host/pci.c
>>>> +++ b/drivers/nvme/host/pci.c
>>>> @@ -981,6 +981,66 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>    	return BLK_STS_OK;
>>>>    }
>>>>    
>>>> +static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
>>>> +{
>>>> +	spin_lock(&nvmeq->sq_lock);
>>>> +	while (!rq_list_empty(*rqlist)) {
>>>> +		struct request *req = rq_list_pop(rqlist);
>>>> +		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>>>> +
>>>> +		memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>>>> +				absolute_pointer(&iod->cmd), sizeof(iod->cmd));
>>>> +		if (++nvmeq->sq_tail == nvmeq->q_depth)
>>>> +			nvmeq->sq_tail = 0;
>>>> +	}
>>>> +	nvme_write_sq_db(nvmeq, true);
>>>> +	spin_unlock(&nvmeq->sq_lock);
>>>> +}
>>>> +
>>>> +static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
>>>> +{
>>>> +	/*
>>>> +	 * We should not need to do this, but we're still using this to
>>>> +	 * ensure we can drain requests on a dying queue.
>>>> +	 */
>>>> +	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
>>>> +		return false;
>>>> +	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
>>>> +		return false;
>>>> +
>>>> +	req->mq_hctx->tags->rqs[req->tag] = req;
>>>> +	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
>>>> +}
>>>> +
>>>> +static void nvme_queue_rqs(struct request **rqlist)
>>>> +{
>>>> +	struct request *req = rq_list_peek(rqlist), *prev = NULL;
>>>> +	struct request *requeue_list = NULL;
>>>> +
>>>> +	do {
>>>> +		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
>>>> +
>>>> +		if (!nvme_prep_rq_batch(nvmeq, req)) {
>>>> +			/* detach 'req' and add to remainder list */
>>>> +			if (prev)
>>>> +				prev->rq_next = req->rq_next;
>>>> +			rq_list_add(&requeue_list, req);
>>>> +		} else {
>>>> +			prev = req;
>>>> +		}
>>>> +
>>>> +		req = rq_list_next(req);
>>>> +		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
>>>> +			/* detach rest of list, and submit */
>>>> +			prev->rq_next = NULL;
>>> if req == NULL and prev == NULL we'll get a NULL deref here.
>>>
>>> I think this can happen in the first iteration.
>>>
>>> Correct me if I'm wrong..
>> First iteration we know the list isn't empty, so req can't be NULL
>> there.
> 
> but you set "req = rq_list_next(req);"
> 
> So can't req be NULL ? after the above line ?

I guess if we hit the prep failure path for the first request that could
be a concern. Probably best to add an if (prev) before that detach,
thanks.

-- 
Jens Axboe

[PATCHSET v5 0/4] Add support for list issue

[Jens Axboe]

Hi,

With the support in 5.16-rc1 for allocating and completing batches of
IO, the one missing piece is passing down a list of requests for issue.
Drivers can take advantage of this by defining an mq_ops->queue_rqs()
hook.

This implements it for NVMe, allowing copy of multiple commands in one
swoop.

This is good for around a 500K IOPS/core improvement in my testing,
which is around a 5-6% improvement in efficiency.

Note to Christoph - I kept the copy helper, since it's used in 3
spots and I _think_ you ended up being fine with that...

Changes since v4:
- Get rid of nvme_submit_cmd()
- Check for prev == NULL for batched issue

[PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

We'll need it for batched submit as well. Since we now have a copy
helper, get rid of the nvme_submit_cmd() wrapper.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8637538f3fd5..9d2a36de228a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -500,22 +500,13 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
-/**
- * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
- * @nvmeq: The queue to use
- * @cmd: The command to send
- * @write_sq: whether to write to the SQ doorbell
- */
-static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
-			    bool write_sq)
+static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
+				    struct nvme_command *cmd)
 {
-	spin_lock(&nvmeq->sq_lock);
-	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
-	       cmd, sizeof(*cmd));
+	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
+		sizeof(*cmd));
 	if (++nvmeq->sq_tail == nvmeq->q_depth)
 		nvmeq->sq_tail = 0;
-	nvme_write_sq_db(nvmeq, write_sq);
-	spin_unlock(&nvmeq->sq_lock);
 }
 
 static void nvme_commit_rqs(struct blk_mq_hw_ctx *hctx)
@@ -957,7 +948,10 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	}
 
 	blk_mq_start_request(req);
-	nvme_submit_cmd(nvmeq, cmnd, bd->last);
+	spin_lock(&nvmeq->sq_lock);
+	nvme_sq_copy_cmd(nvmeq, &iod->cmd);
+	nvme_write_sq_db(nvmeq, bd->last);
+	spin_unlock(&nvmeq->sq_lock);
 	return BLK_STS_OK;
 out_unmap_data:
 	nvme_unmap_data(dev, req);
@@ -1140,7 +1134,11 @@ static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
 
 	c.common.opcode = nvme_admin_async_event;
 	c.common.command_id = NVME_AQ_BLK_MQ_DEPTH;
-	nvme_submit_cmd(nvmeq, &c, true);
+
+	spin_lock(&nvmeq->sq_lock);
+	nvme_sq_copy_cmd(nvmeq, &c);
+	nvme_write_sq_db(nvmeq, true);
+	spin_unlock(&nvmeq->sq_lock);
 }
 
 static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)
-- 
2.34.1

Re: [PATCH 2/4] nvme: split command copy into a helper

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHSET v4 0/4] Add support for list issue

[Jens Axboe]

With the support in 5.16-rc1 for allocating and completing batches of
IO, the one missing piece is passing down a list of requests for issue.
Drivers can take advantage of this by defining an mq_ops->queue_rqs()
hook.

This implements it for NVMe, allowing copy of multiple commands in one
swoop.

This is good for around a 500K IOPS/core improvement in my testing,
which is around a 5-6% improvement in efficiency.

Note to Christoph - I kept the copy helper, since it's used in 3
spots and I _think_ you ended up being fine with that...

Changes since v3:
- Use nvme_sq_copy_cmd() in nvme_submit_cmds()
- Add reviewed-by's

-- 
Jens Axboe

[PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

We'll need it for batched submit as well.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8637538f3fd5..09ea21f75439 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -500,6 +500,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
+static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
+				    struct nvme_command *cmd)
+{
+	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
+		sizeof(*cmd));
+	if (++nvmeq->sq_tail == nvmeq->q_depth)
+		nvmeq->sq_tail = 0;
+}
+
 /**
  * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
  * @nvmeq: The queue to use
@@ -510,10 +519,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
 			    bool write_sq)
 {
 	spin_lock(&nvmeq->sq_lock);
-	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
-	       cmd, sizeof(*cmd));
-	if (++nvmeq->sq_tail == nvmeq->q_depth)
-		nvmeq->sq_tail = 0;
+	nvme_sq_copy_cmd(nvmeq, cmd);
 	nvme_write_sq_db(nvmeq, write_sq);
 	spin_unlock(&nvmeq->sq_lock);
 }
-- 
2.34.1

[PATCHSET v2 0/4] Add support for list issue

[Jens Axboe]

Hi,

With the support in 5.16-rc1 for allocating and completing batches of
IO, the one missing piece is passing down a list of requests for issue.
Drivers can take advantage of this by defining an mq_ops->queue_rqs()
hook.

This implements it for NVMe, allowing copy of multiple commands in one
swoop.

This is good for around a 500K IOPS/core improvement in my testing,
which is around a 5-6% improvement in efficiency.

Changes since v1:

- Addressed review comments
- Rebase on top of Ming's hctx lock change
- Clean ups
- Bypass for shared tags

-- 
Jens Axboe

[PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

We'll need it for batched submit as well.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8637538f3fd5..09ea21f75439 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -500,6 +500,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
+static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
+				    struct nvme_command *cmd)
+{
+	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
+		sizeof(*cmd));
+	if (++nvmeq->sq_tail == nvmeq->q_depth)
+		nvmeq->sq_tail = 0;
+}
+
 /**
  * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
  * @nvmeq: The queue to use
@@ -510,10 +519,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
 			    bool write_sq)
 {
 	spin_lock(&nvmeq->sq_lock);
-	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
-	       cmd, sizeof(*cmd));
-	if (++nvmeq->sq_tail == nvmeq->q_depth)
-		nvmeq->sq_tail = 0;
+	nvme_sq_copy_cmd(nvmeq, cmd);
 	nvme_write_sq_db(nvmeq, write_sq);
 	spin_unlock(&nvmeq->sq_lock);
 }
-- 
2.34.1

Re: [PATCH 2/4] nvme: split command copy into a helper

[Hannes Reinecke]

On 12/3/21 10:45 PM, Jens Axboe wrote:
> We'll need it for batched submit as well.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   drivers/nvme/host/pci.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8637538f3fd5..09ea21f75439 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -500,6 +500,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
>   	nvmeq->last_sq_tail = nvmeq->sq_tail;
>   }
>   
> +static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
> +				    struct nvme_command *cmd)
> +{
> +	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
> +		sizeof(*cmd));
> +	if (++nvmeq->sq_tail == nvmeq->q_depth)
> +		nvmeq->sq_tail = 0;
> +}
> +
>   /**
>    * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
>    * @nvmeq: The queue to use
> @@ -510,10 +519,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
>   			    bool write_sq)
>   {
>   	spin_lock(&nvmeq->sq_lock);
> -	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> -	       cmd, sizeof(*cmd));
> -	if (++nvmeq->sq_tail == nvmeq->q_depth)
> -		nvmeq->sq_tail = 0;
> +	nvme_sq_copy_cmd(nvmeq, cmd);
>   	nvme_write_sq_db(nvmeq, write_sq);
>   	spin_unlock(&nvmeq->sq_lock);
>   }
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/4] nvme: split command copy into a helper

[Christoph Hellwig]

As mentioned last time I really don't like the proliferation of
tiny helpers with just two callers.  See my patch I responded with
for my alternative.

Re: [PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

On 12/6/21 12:38 AM, Christoph Hellwig wrote:
> As mentioned last time I really don't like the proliferation of
> tiny helpers with just two callers.  See my patch I responded with
> for my alternative.

I tend to agree with you, but for this particular case I really don't
think it's better to not have the helper. I did try that version too,
but you end up with several sites duplicating it. So while I agree in
general, I don't think this one applies.

-- 
Jens Axboe

[PATCHSET 0/4] Add support for list issue

[Jens Axboe]

Hi,

With the support in 5.16-rc1 for allocating and completing batches of
IO, the one missing piece is passing down a list of requests for issue.
Drivers can take advantage of this by defining an mq_ops->queue_rqs()
hook.

This implements it for NVMe, allowing copy of multiple commands in one
swoop.

This is good for around a 500K IOPS/core improvement in my testing,
which is around a 5-6% improvement in efficiency.

-- 
Jens Axboe

[PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

We'll need it for batched submit as well.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index c89f74ea00d4..c33cd1177b37 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -501,6 +501,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
+static inline void nvme_copy_cmd(struct nvme_queue *nvmeq,
+				 struct nvme_command *cmd)
+{
+	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
+		sizeof(*cmd));
+	if (++nvmeq->sq_tail == nvmeq->q_depth)
+		nvmeq->sq_tail = 0;
+}
+
 /**
  * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
  * @nvmeq: The queue to use
@@ -511,10 +520,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
 			    bool write_sq)
 {
 	spin_lock(&nvmeq->sq_lock);
-	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
-	       cmd, sizeof(*cmd));
-	if (++nvmeq->sq_tail == nvmeq->q_depth)
-		nvmeq->sq_tail = 0;
+	nvme_copy_cmd(nvmeq, cmd);
 	nvme_write_sq_db(nvmeq, write_sq);
 	spin_unlock(&nvmeq->sq_lock);
 }
-- 
2.33.1

Re: [PATCH 2/4] nvme: split command copy into a helper

[Christoph Hellwig]

On Tue, Nov 16, 2021 at 08:38:05PM -0700, Jens Axboe wrote:
>  /**
>   * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
>   * @nvmeq: The queue to use
> @@ -511,10 +520,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
>  			    bool write_sq)
>  {
>  	spin_lock(&nvmeq->sq_lock);
> -	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
> -	       cmd, sizeof(*cmd));
> -	if (++nvmeq->sq_tail == nvmeq->q_depth)
> -		nvmeq->sq_tail = 0;
> +	nvme_copy_cmd(nvmeq, cmd);
>  	nvme_write_sq_db(nvmeq, write_sq);
>  	spin_unlock(&nvmeq->sq_lock);

Given that nvme_submit_cmd only has two callers, I'd be tempted to just
open code in the callers rather than creating a deep callchain here.

Re: [PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

On 11/16/21 11:15 PM, Christoph Hellwig wrote:
> On Tue, Nov 16, 2021 at 08:38:05PM -0700, Jens Axboe wrote:
>>  /**
>>   * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
>>   * @nvmeq: The queue to use
>> @@ -511,10 +520,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
>>  			    bool write_sq)
>>  {
>>  	spin_lock(&nvmeq->sq_lock);
>> -	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
>> -	       cmd, sizeof(*cmd));
>> -	if (++nvmeq->sq_tail == nvmeq->q_depth)
>> -		nvmeq->sq_tail = 0;
>> +	nvme_copy_cmd(nvmeq, cmd);
>>  	nvme_write_sq_db(nvmeq, write_sq);
>>  	spin_unlock(&nvmeq->sq_lock);
> 
> Given that nvme_submit_cmd only has two callers, I'd be tempted to just
> open code in the callers rather than creating a deep callchain here.

It's inlined, but if you prefer duplicating the code, then I can just
drop this patch.

-- 
Jens Axboe

Re: [PATCH 2/4] nvme: split command copy into a helper

[Chaitanya Kulkarni]

On 11/16/21 19:38, Jens Axboe wrote:
> External email: Use caution opening links or attachments
> 
> 
> We'll need it for batched submit as well.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---

maybe use nvme_sq_add_cmd() or nvme_sq_copy_cmd()
instead of nvme_copy_cmd() makes is easier to grep
XXX_sq_XXX(), either way looks good :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>