[PATCH 0/3] blk_mq_rq_ctx_init() optimisations

[PATCH 0/3] blk_mq_rq_ctx_init() optimisations

[Pavel Begunkov]

Restore the original version of the patches. Mostly the same as was
sent out by Jens, but those were squashed and a couple of details
is missing.

Pavel Begunkov (3):
  block: skip elevator fields init for non-elv queue
  block: blk_mq_rq_ctx_init cache ctx/q/hctx
  block: cache rq_flags inside blk_mq_rq_ctx_init()

 block/blk-mq.c | 54 ++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

-- 
2.33.1

[PATCH 1/3] block: skip elevator fields init for non-elv queue

[Pavel Begunkov]

Don't init rq->hash and rq->rb_node in blk_mq_rq_ctx_init() if there is
no elevator. Also, move some other initialisers that imply barriers to
the end, so the compiler is free to rearrange and optimise other the
rest of them.

note: fold in a change from Jens leaving queue_list unconditional, as
it might lead to problems otherwise.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 28eb1f3c6f76..1d2e2fd4043e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -325,6 +325,10 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		rq->internal_tag = BLK_MQ_NO_TAG;
 	}
 
+	if (blk_mq_need_time_stamp(rq))
+		rq->start_time_ns = ktime_get_ns();
+	else
+		rq->start_time_ns = 0;
 	/* csd/requeue_work/fifo_time is initialized before use */
 	rq->q = data->q;
 	rq->mq_ctx = data->ctx;
@@ -334,41 +338,37 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		rq->rq_flags |= RQF_PM;
 	if (blk_queue_io_stat(data->q))
 		rq->rq_flags |= RQF_IO_STAT;
-	INIT_LIST_HEAD(&rq->queuelist);
-	INIT_HLIST_NODE(&rq->hash);
-	RB_CLEAR_NODE(&rq->rb_node);
 	rq->rq_disk = NULL;
 	rq->part = NULL;
 #ifdef CONFIG_BLK_RQ_ALLOC_TIME
 	rq->alloc_time_ns = alloc_time_ns;
 #endif
-	if (blk_mq_need_time_stamp(rq))
-		rq->start_time_ns = ktime_get_ns();
-	else
-		rq->start_time_ns = 0;
 	rq->io_start_time_ns = 0;
 	rq->stats_sectors = 0;
 	rq->nr_phys_segments = 0;
 #if defined(CONFIG_BLK_DEV_INTEGRITY)
 	rq->nr_integrity_segments = 0;
 #endif
-	blk_crypto_rq_set_defaults(rq);
-	/* tag was already set */
-	WRITE_ONCE(rq->deadline, 0);
-
 	rq->timeout = 0;
-
 	rq->end_io = NULL;
 	rq->end_io_data = NULL;
 
 	data->ctx->rq_dispatched[op_is_sync(data->cmd_flags)]++;
+	blk_crypto_rq_set_defaults(rq);
+	INIT_LIST_HEAD(&rq->queuelist);
+	/* tag was already set */
+	WRITE_ONCE(rq->deadline, 0);
 	refcount_set(&rq->ref, 1);
 
-	if (!op_is_flush(data->cmd_flags) && (rq->rq_flags & RQF_ELV)) {
+	if (rq->rq_flags & RQF_ELV) {
 		struct elevator_queue *e = data->q->elevator;
 
 		rq->elv.icq = NULL;
-		if (e->type->ops.prepare_request) {
+		INIT_HLIST_NODE(&rq->hash);
+		RB_CLEAR_NODE(&rq->rb_node);
+
+		if (!op_is_flush(data->cmd_flags) &&
+		    e->type->ops.prepare_request) {
 			if (e->type->icq_cache)
 				blk_mq_sched_assign_ioc(rq);
 
-- 
2.33.1

[PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx

[Pavel Begunkov]

We should have enough of registers in blk_mq_rq_ctx_init(), store them
in local vars, so we don't keep reloading them.

note: keeping q->elevator may look unnecessary, but it's also used
inside inlined blk_mq_tags_from_data().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 1d2e2fd4043e..fa4de25c3bcb 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -312,10 +312,14 @@ static inline bool blk_mq_need_time_stamp(struct request *rq)
 static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		unsigned int tag, u64 alloc_time_ns)
 {
+	struct blk_mq_ctx *ctx = data->ctx;
+	struct blk_mq_hw_ctx *hctx = data->hctx;
+	struct request_queue *q = data->q;
+	struct elevator_queue *e = q->elevator;
 	struct blk_mq_tags *tags = blk_mq_tags_from_data(data);
 	struct request *rq = tags->static_rqs[tag];
 
-	if (data->q->elevator) {
+	if (e) {
 		rq->rq_flags = RQF_ELV;
 		rq->tag = BLK_MQ_NO_TAG;
 		rq->internal_tag = tag;
@@ -330,13 +334,13 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	else
 		rq->start_time_ns = 0;
 	/* csd/requeue_work/fifo_time is initialized before use */
-	rq->q = data->q;
-	rq->mq_ctx = data->ctx;
-	rq->mq_hctx = data->hctx;
+	rq->q = q;
+	rq->mq_ctx = ctx;
+	rq->mq_hctx = hctx;
 	rq->cmd_flags = data->cmd_flags;
 	if (data->flags & BLK_MQ_REQ_PM)
 		rq->rq_flags |= RQF_PM;
-	if (blk_queue_io_stat(data->q))
+	if (blk_queue_io_stat(q))
 		rq->rq_flags |= RQF_IO_STAT;
 	rq->rq_disk = NULL;
 	rq->part = NULL;
-- 
2.33.1

[PATCH 3/3] block: cache rq_flags inside blk_mq_rq_ctx_init()

[Pavel Begunkov]

Add a local variable for rq_flags, it helps to compile out some of
rq_flags reloads.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-mq.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index fa4de25c3bcb..633d73580712 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -318,17 +318,23 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	struct elevator_queue *e = q->elevator;
 	struct blk_mq_tags *tags = blk_mq_tags_from_data(data);
 	struct request *rq = tags->static_rqs[tag];
+	unsigned int rq_flags = 0;
 
 	if (e) {
-		rq->rq_flags = RQF_ELV;
+		rq_flags = RQF_ELV;
 		rq->tag = BLK_MQ_NO_TAG;
 		rq->internal_tag = tag;
 	} else {
-		rq->rq_flags = 0;
 		rq->tag = tag;
 		rq->internal_tag = BLK_MQ_NO_TAG;
 	}
 
+	if (data->flags & BLK_MQ_REQ_PM)
+		rq_flags |= RQF_PM;
+	if (blk_queue_io_stat(q))
+		rq_flags |= RQF_IO_STAT;
+	rq->rq_flags = rq_flags;
+
 	if (blk_mq_need_time_stamp(rq))
 		rq->start_time_ns = ktime_get_ns();
 	else
@@ -338,10 +344,6 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->mq_ctx = ctx;
 	rq->mq_hctx = hctx;
 	rq->cmd_flags = data->cmd_flags;
-	if (data->flags & BLK_MQ_REQ_PM)
-		rq->rq_flags |= RQF_PM;
-	if (blk_queue_io_stat(q))
-		rq->rq_flags |= RQF_IO_STAT;
 	rq->rq_disk = NULL;
 	rq->part = NULL;
 #ifdef CONFIG_BLK_RQ_ALLOC_TIME
-- 
2.33.1

Re: [PATCH 0/3] blk_mq_rq_ctx_init() optimisations

[Jens Axboe]

On Mon, 18 Oct 2021 21:37:26 +0100, Pavel Begunkov wrote:
> Restore the original version of the patches. Mostly the same as was
> sent out by Jens, but those were squashed and a couple of details
> is missing.
> 
> Pavel Begunkov (3):
>   block: skip elevator fields init for non-elv queue
>   block: blk_mq_rq_ctx_init cache ctx/q/hctx
>   block: cache rq_flags inside blk_mq_rq_ctx_init()
> 
> [...]

Applied, thanks!

[1/3] block: skip elevator fields init for non-elv queue
      commit: 4f266f2be822eacd70aca2a7a53c4a111be79acb
[2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx
      commit: 605f784e4f5faecf6c78070c6bf446e920104f9f
[3/3] block: cache rq_flags inside blk_mq_rq_ctx_init()
      commit: 128459062bc994355027e190477c432ec5b5638a

Best regards,
-- 
Jens Axboe

Re: [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 09:37:28PM +0100, Pavel Begunkov wrote:
> We should have enough of registers in blk_mq_rq_ctx_init(), store them
> in local vars, so we don't keep reloading them.
> 
> note: keeping q->elevator may look unnecessary, but it's also used
> inside inlined blk_mq_tags_from_data().

Is this really making a difference?  I'd expect todays hyper-optimizing
compilers to not be tricked into specific register allocations just by
adding a local variable.

Re: [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx

[Pavel Begunkov]

On 10/19/21 07:00, Christoph Hellwig wrote:
> On Mon, Oct 18, 2021 at 09:37:28PM +0100, Pavel Begunkov wrote:
>> We should have enough of registers in blk_mq_rq_ctx_init(), store them
>> in local vars, so we don't keep reloading them.
>>
>> note: keeping q->elevator may look unnecessary, but it's also used
>> inside inlined blk_mq_tags_from_data().
> 
> Is this really making a difference?  I'd expect todays hyper-optimizing
> compilers to not be tricked into specific register allocations just by
> adding a local variable.

Looking again, there are only reads before the use site, so indeed
shouldn't matter. Was left from first versions of the patch where
it wasn't the case.

-- 
Pavel Begunkov