blk_mq_rq_ctx_init() optimisations

PATCH Applied Pavel Begunkov 3 participants 7 messages Oct 18, 2021 – Oct 19, 2021 Block layer

Share & tools

Pavel Begunkov

author

2021-10-18 20:37 UTC #0 lore raw

cover blk_mq_rq_ctx_init() optimisations

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

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-18 20:37 UTC #1 lore raw

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

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 <[email protected]>

---
 block/blk-mq.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

block/blk-mq.c +14 ~~-14~~

index 28eb1f3c6f76..1d2e2fd4043e 100644
@@ -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);

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-18 20:37 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 block/blk-mq.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

block/blk-mq.c +9 -5

index 1d2e2fd4043e..fa4de25c3bcb 100644
@@ -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;

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-18 20:37 UTC #5 lore raw

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

In reply to Pavel Begunkov

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

Signed-off-by Pavel Begunkov <[email protected]>

---
 block/blk-mq.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

block/blk-mq.c +8 -6

index fa4de25c3bcb..633d73580712 100644
@@ -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

Signature · 1 line

2.33.1

Jens Axboe

maintainer Applied

2021-10-18 21:42 UTC #6 lore raw

In reply to Pavel Begunkov

Quoted 10 lines · 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()
[...]

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,

Signature · 1 line

Jens Axboe

Christoph Hellwig

2021-10-19 06:00 UTC #3 lore raw

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

In reply to Pavel Begunkov

Quoted 5 lines · 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().

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.

Pavel Begunkov

author

2021-10-19 10:38 UTC #4 lore raw

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

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · 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().
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.

Signature · 1 line

Pavel Begunkov