[PATCH V3 0/3] blk-mq: improve IO perf in case of none io sched

[PATCH V3 0/3] blk-mq: improve IO perf in case of none io sched

[Ming Lei]

Hi,

The 1st 2 patch improves ctx->lock uses, and it is observed that IOPS
may be improved by ~5% in rand IO test on MegaRaid SAS run by Kashyap.

The 3rd patch fixes rand IO performance regression on MegaRaid SAS
test, still reported by Kashyap.

V3:
	- export dispatch busy from debugfs as suggested by Jens
	- add comment on blk_mq_update_hctx_busy() as suggested by Christoph

V2:
	- fix list corruption in patch 1/3


Ming Lei (3):
  blk-mq: use list_splice_tail_init() to insert requests
  blk-mq: only attempt to merge bio if there is rq in sw queue
  blk-mq: dequeue request one by one from sw queue iff hctx is busy

 block/blk-mq-debugfs.c |  9 +++++++++
 block/blk-mq-sched.c   | 14 ++++----------
 block/blk-mq.c         | 44 ++++++++++++++++++++++++++++++++++++--------
 include/linux/blk-mq.h |  3 ++-
 4 files changed, 51 insertions(+), 19 deletions(-)

Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.de>

-- 
2.9.5

[PATCH V3 1/3] blk-mq: use list_splice_tail_init() to insert requests

[Ming Lei]

list_splice_tail_init() is much more faster than inserting each
request one by one, given all requets in 'list' belong to
same sw queue and ctx->lock is required to insert requests.

Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 95919268564b..174637d09923 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1544,19 +1544,19 @@ void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
 			    struct list_head *list)
 
 {
+	struct request *rq;
+
 	/*
 	 * preemption doesn't flush plug list, so it's possible ctx->cpu is
 	 * offline now
 	 */
-	spin_lock(&ctx->lock);
-	while (!list_empty(list)) {
-		struct request *rq;
-
-		rq = list_first_entry(list, struct request, queuelist);
+	list_for_each_entry(rq, list, queuelist) {
 		BUG_ON(rq->mq_ctx != ctx);
-		list_del_init(&rq->queuelist);
-		__blk_mq_insert_req_list(hctx, rq, false);
+		trace_block_rq_insert(hctx->queue, rq);
 	}
+
+	spin_lock(&ctx->lock);
+	list_splice_tail_init(list, &ctx->rq_list);
 	blk_mq_hctx_mark_pending(hctx, ctx);
 	spin_unlock(&ctx->lock);
 }
-- 
2.9.5

[PATCH V3 2/3] blk-mq: only attempt to merge bio if there is rq in sw queue

[Ming Lei]

Only attempt to merge bio iff the ctx->rq_list isn't empty, because:

1) for high-performance SSD, most of times dispatch may succeed, then
there may be nothing left in ctx->rq_list, so don't try to merge over
sw queue if it is empty, then we can save one acquiring of ctx->lock

2) we can't expect good merge performance on per-cpu sw queue, and missing
one merge on sw queue won't be a big deal since tasks can be scheduled from
one CPU to another.

Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Reported-by: Kashyap Desai <kashyap.desai@broadcom.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 56c493c6cd90..f5745acc2d98 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -339,7 +339,8 @@ bool __blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio)
 		return e->type->ops.mq.bio_merge(hctx, bio);
 	}
 
-	if (hctx->flags & BLK_MQ_F_SHOULD_MERGE) {
+	if ((hctx->flags & BLK_MQ_F_SHOULD_MERGE) &&
+			!list_empty_careful(&ctx->rq_list)) {
 		/* default per sw-queue merge */
 		spin_lock(&ctx->lock);
 		ret = blk_mq_attempt_merge(q, ctx, bio);
-- 
2.9.5

[PATCH V3 3/3] blk-mq: dequeue request one by one from sw queue iff hctx is busy

[Ming Lei]

It won't be efficient to dequeue request one by one from sw queue,
but we have to do that when queue is busy for better merge performance.

This patch takes EWMA to figure out if queue is busy, then only dequeue
request one by one from sw queue when queue is busy.

Fixes: b347689ffbca ("blk-mq-sched: improve dispatching from sw queue")
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.de>
Reported-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c |  9 +++++++++
 block/blk-mq-sched.c   | 11 ++---------
 block/blk-mq.c         | 30 +++++++++++++++++++++++++++++-
 include/linux/blk-mq.h |  3 ++-
 4 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 1c4532e92938..dd87c274a6b8 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -637,6 +637,14 @@ static int hctx_active_show(void *data, struct seq_file *m)
 	return 0;
 }
 
+static int hctx_dispatch_busy_show(void *data, struct seq_file *m)
+{
+	struct blk_mq_hw_ctx *hctx = data;
+
+	seq_printf(m, "%u\n", hctx->dispatch_busy);
+	return 0;
+}
+
 static void *ctx_rq_list_start(struct seq_file *m, loff_t *pos)
 	__acquires(&ctx->lock)
 {
@@ -798,6 +806,7 @@ static const struct blk_mq_debugfs_attr blk_mq_debugfs_hctx_attrs[] = {
 	{"queued", 0600, hctx_queued_show, hctx_queued_write},
 	{"run", 0600, hctx_run_show, hctx_run_write},
 	{"active", 0400, hctx_active_show},
+	{"dispatch_busy", 0400, hctx_dispatch_busy_show},
 	{},
 };
 
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index f5745acc2d98..7856dc5db0eb 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -219,15 +219,8 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
 		}
 	} else if (has_sched_dispatch) {
 		blk_mq_do_dispatch_sched(hctx);
-	} else if (q->mq_ops->get_budget) {
-		/*
-		 * If we need to get budget before queuing request, we
-		 * dequeue request one by one from sw queue for avoiding
-		 * to mess up I/O merge when dispatch runs out of resource.
-		 *
-		 * TODO: get more budgets, and dequeue more requests in
-		 * one time.
-		 */
+	} else if (READ_ONCE(hctx->dispatch_busy)) {
+		/* dequeue request one by one from sw queue if queue is busy */
 		blk_mq_do_dispatch_ctx(hctx);
 	} else {
 		blk_mq_flush_busy_ctxs(hctx, &rq_list);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 174637d09923..22db9c897f84 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1073,6 +1073,31 @@ static bool blk_mq_mark_tag_wait(struct blk_mq_hw_ctx **hctx,
 	return true;
 }
 
+/*
+ * Update dispatch busy with EWMA:
+ * - EWMA is one simple way to compute running average value
+ * - weight(7/8 and 1/8) is applied so that it can decrease exponentially
+ * - take 16 as current busy value for avoiding to get too small(0) result,
+ *   and this factor doesn't matter given EWMA decreases exponentially
+ */
+static void blk_mq_update_hctx_busy(struct blk_mq_hw_ctx *hctx, bool busy)
+{
+	const unsigned weight = 8;
+	unsigned int ewma;
+
+	if (hctx->queue->elevator)
+		return;
+
+	ewma = READ_ONCE(hctx->dispatch_busy);
+
+	ewma *= weight - 1;
+	if (busy)
+		ewma += 16;
+	ewma /= weight;
+
+	WRITE_ONCE(hctx->dispatch_busy, ewma);
+}
+
 #define BLK_MQ_RESOURCE_DELAY	3		/* ms units */
 
 /*
@@ -1209,8 +1234,11 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
 		else if (needs_restart && (ret == BLK_STS_RESOURCE))
 			blk_mq_delay_run_hw_queue(hctx, BLK_MQ_RESOURCE_DELAY);
 
+		blk_mq_update_hctx_busy(hctx, true);
+
 		return false;
-	}
+	} else
+		blk_mq_update_hctx_busy(hctx, false);
 
 	/*
 	 * If the host/device is unable to accept more work, inform the
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index e3147eb74222..399e0a610ea3 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -35,9 +35,10 @@ struct blk_mq_hw_ctx {
 	struct sbitmap		ctx_map;
 
 	struct blk_mq_ctx	*dispatch_from;
+	unsigned int		dispatch_busy;
 
-	struct blk_mq_ctx	**ctxs;
 	unsigned int		nr_ctx;
+	struct blk_mq_ctx	**ctxs;
 
 	wait_queue_entry_t	dispatch_wait;
 	atomic_t		wait_index;
-- 
2.9.5

Re: [PATCH V3 3/3] blk-mq: dequeue request one by one from sw queue iff hctx is busy

[Christoph Hellwig]

> +/*
> + * Update dispatch busy with EWMA:

Please expand the EWMA acronym.

> +static void blk_mq_update_hctx_busy(struct blk_mq_hw_ctx *hctx, bool busy)
> +{
> +	const unsigned weight = 8;
> +	unsigned int ewma;
> +
> +	if (hctx->queue->elevator)
> +		return;
> +
> +	ewma = READ_ONCE(hctx->dispatch_busy);
> +
> +	ewma *= weight - 1;
> +	if (busy)
> +		ewma += 16;

plese use descriptive all upper case #defines for the WEIGHT and FACTOR
so that they stick out.

Re: [PATCH V3 3/3] blk-mq: dequeue request one by one from sw queue iff hctx is busy

[Jens Axboe]

On 7/2/18 3:36 AM, Ming Lei wrote:
> It won't be efficient to dequeue request one by one from sw queue,
> but we have to do that when queue is busy for better merge performance.
> 
> This patch takes EWMA to figure out if queue is busy, then only dequeue
> request one by one from sw queue when queue is busy.

Just one minor comment, since you're going to be updating this one
anyway:

> @@ -1209,8 +1234,11 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  		else if (needs_restart && (ret == BLK_STS_RESOURCE))
>  			blk_mq_delay_run_hw_queue(hctx, BLK_MQ_RESOURCE_DELAY);
>  
> +		blk_mq_update_hctx_busy(hctx, true);
> +
>  		return false;

Kill that newline between update and return.

Rest looks fine to me now, though I do agree with Christophs comments on
making the weight and factor in caps.

Applying 1-2/3 so far.

-- 
Jens Axboe

Re: [PATCH V3 3/3] blk-mq: dequeue request one by one from sw queue iff hctx is busy

[Ming Lei]

On Mon, Jul 02, 2018 at 11:30:17AM -0600, Jens Axboe wrote:
> On 7/2/18 3:36 AM, Ming Lei wrote:
> > It won't be efficient to dequeue request one by one from sw queue,
> > but we have to do that when queue is busy for better merge performance.
> > 
> > This patch takes EWMA to figure out if queue is busy, then only dequeue
> > request one by one from sw queue when queue is busy.
> 
> Just one minor comment, since you're going to be updating this one
> anyway:
> 
> > @@ -1209,8 +1234,11 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> >  		else if (needs_restart && (ret == BLK_STS_RESOURCE))
> >  			blk_mq_delay_run_hw_queue(hctx, BLK_MQ_RESOURCE_DELAY);
> >  
> > +		blk_mq_update_hctx_busy(hctx, true);
> > +
> >  		return false;
> 
> Kill that newline between update and return.
> 
> Rest looks fine to me now, though I do agree with Christophs comments on
> making the weight and factor in caps.
> 
> Applying 1-2/3 so far.

Hi Jens,

Not sure if 3 is applied now given your for-4.19/block isn't public yet.
So please let me know if you need me to re-send 3.

Thanks,
Ming

Re: [PATCH V3 3/3] blk-mq: dequeue request one by one from sw queue iff hctx is busy

[Jens Axboe]

On Jul 2, 2018, at 7:23 PM, Ming Lei <ming.lei@redhat.com> wrote:
>=20
>> On Mon, Jul 02, 2018 at 11:30:17AM -0600, Jens Axboe wrote:
>>> On 7/2/18 3:36 AM, Ming Lei wrote:
>>> It won't be efficient to dequeue request one by one from sw queue,
>>> but we have to do that when queue is busy for better merge performance.
>>>=20
>>> This patch takes EWMA to figure out if queue is busy, then only dequeue
>>> request one by one from sw queue when queue is busy.
>>=20
>> Just one minor comment, since you're going to be updating this one
>> anyway:
>>=20
>>> @@ -1209,8 +1234,11 @@ bool blk_mq_dispatch_rq_list(struct request_queue=
 *q, struct list_head *list,
>>>        else if (needs_restart && (ret =3D=3D BLK_STS_RESOURCE))
>>>            blk_mq_delay_run_hw_queue(hctx, BLK_MQ_RESOURCE_DELAY);
>>>=20
>>> +        blk_mq_update_hctx_busy(hctx, true);
>>> +
>>>        return false;
>>=20
>> Kill that newline between update and return.
>>=20
>> Rest looks fine to me now, though I do agree with Christophs comments on
>> making the weight and factor in caps.
>>=20
>> Applying 1-2/3 so far.
>=20
> Hi Jens,
>=20
> Not sure if 3 is applied now given your for-4.19/block isn't public yet.
> So please let me know if you need me to re-send 3.

Please send a new 3/3, didn=E2=80=99t apply it yet. I haven=E2=80=99t pushed=
 out my 4.19 branch yet, since I may have to rebase it. It=E2=80=99ll come o=
ut soon.=20

Re: [PATCH V3 0/3] blk-mq: improve IO perf in case of none io sched

[Laurence Oberman]

On Mon, 2018-07-02 at 17:35 +0800, Ming Lei wrote:
> Hi,
> 
> The 1st 2 patch improves ctx->lock uses, and it is observed that IOPS
> may be improved by ~5% in rand IO test on MegaRaid SAS run by
> Kashyap.
> 
> The 3rd patch fixes rand IO performance regression on MegaRaid SAS
> test, still reported by Kashyap.
> 
> V3:
> 	- export dispatch busy from debugfs as suggested by Jens
> 	- add comment on blk_mq_update_hctx_busy() as suggested by
> Christoph
> 
> V2:
> 	- fix list corruption in patch 1/3
> 
> 
> Ming Lei (3):
>   blk-mq: use list_splice_tail_init() to insert requests
>   blk-mq: only attempt to merge bio if there is rq in sw queue
>   blk-mq: dequeue request one by one from sw queue iff hctx is busy


> 
>  block/blk-mq-debugfs.c |  9 +++++++++
>  block/blk-mq-sched.c   | 14 ++++----------
>  block/blk-mq.c         | 44 ++++++++++++++++++++++++++++++++++++--
> ------
>  include/linux/blk-mq.h |  3 ++-
>  4 files changed, 51 insertions(+), 19 deletions(-)
> 
> Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> Cc: Laurence Oberman <loberman@redhat.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.de>
> 

Thanks Ming, I repaired my MSA50 shelf so will functional test.
I have 6 SSD drives working in there now. 
I know its already been tested by Kashyap.

Regards
Laurence

RE: [PATCH V3 0/3] blk-mq: improve IO perf in case of none io sched

[Kashyap Desai]

> -----Original Message-----
> From: Laurence Oberman [mailto:loberman@redhat.com]
> Sent: Monday, July 2, 2018 5:11 PM
> To: Ming Lei; Jens Axboe
> Cc: linux-block@vger.kernel.org; Kashyap Desai; Omar Sandoval; Christoph
> Hellwig; Bart Van Assche; Hannes Reinecke
> Subject: Re: [PATCH V3 0/3] blk-mq: improve IO perf in case of none io
> sched
>
> On Mon, 2018-07-02 at 17:35 +0800, Ming Lei wrote:
> > Hi,
> >
> > The 1st 2 patch improves ctx->lock uses, and it is observed that IOPS
> > may be improved by ~5% in rand IO test on MegaRaid SAS run by
> > Kashyap.
> >
> > The 3rd patch fixes rand IO performance regression on MegaRaid SAS
> > test, still reported by Kashyap.
> >
> > V3:
> > 	- export dispatch busy from debugfs as suggested by Jens
> > 	- add comment on blk_mq_update_hctx_busy() as suggested by
> > Christoph
> >
> > V2:
> > 	- fix list corruption in patch 1/3
> >
> >
> > Ming Lei (3):
> >   blk-mq: use list_splice_tail_init() to insert requests
> >   blk-mq: only attempt to merge bio if there is rq in sw queue
> >   blk-mq: dequeue request one by one from sw queue iff hctx is busy
>
>
> >
> >  block/blk-mq-debugfs.c |  9 +++++++++
> >  block/blk-mq-sched.c   | 14 ++++----------
> >  block/blk-mq.c         | 44 ++++++++++++++++++++++++++++++++++++--
> > ------
> >  include/linux/blk-mq.h |  3 ++-
> >  4 files changed, 51 insertions(+), 19 deletions(-)
> >
> > Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> > Cc: Laurence Oberman <loberman@redhat.com>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Hannes Reinecke <hare@suse.de>
> >
>
> Thanks Ming, I repaired my MSA50 shelf so will functional test.
> I have 6 SSD drives working in there now.
> I know its already been tested by Kashyap.

Performance is back to 1.6M IOPS which was earlier 1.3M IOPs on SSD based VD
@MegaRaid controller setup.

Tested-by: Kashyap Desai <kashyap.desai@broadcom.com>

>
> Regards
> Laurence