* [PATCH 0/3] blk_mq_rq_ctx_init() optimisations
@ 2021-10-18 20:37 Pavel Begunkov
2021-10-18 20:37 ` [PATCH 1/3] block: skip elevator fields init for non-elv queue Pavel Begunkov
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Pavel Begunkov @ 2021-10-18 20:37 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe, Christoph Hellwig, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] block: skip elevator fields init for non-elv queue
2021-10-18 20:37 [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Pavel Begunkov
@ 2021-10-18 20:37 ` Pavel Begunkov
2021-10-18 20:37 ` [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx Pavel Begunkov
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Pavel Begunkov @ 2021-10-18 20:37 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe, Christoph Hellwig, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx
2021-10-18 20:37 [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Pavel Begunkov
2021-10-18 20:37 ` [PATCH 1/3] block: skip elevator fields init for non-elv queue Pavel Begunkov
@ 2021-10-18 20:37 ` Pavel Begunkov
2021-10-19 6:00 ` Christoph Hellwig
2021-10-18 20:37 ` [PATCH 3/3] block: cache rq_flags inside blk_mq_rq_ctx_init() Pavel Begunkov
2021-10-18 21:42 ` [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Jens Axboe
3 siblings, 1 reply; 7+ messages in thread
From: Pavel Begunkov @ 2021-10-18 20:37 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe, Christoph Hellwig, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] block: cache rq_flags inside blk_mq_rq_ctx_init()
2021-10-18 20:37 [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Pavel Begunkov
2021-10-18 20:37 ` [PATCH 1/3] block: skip elevator fields init for non-elv queue Pavel Begunkov
2021-10-18 20:37 ` [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx Pavel Begunkov
@ 2021-10-18 20:37 ` Pavel Begunkov
2021-10-18 21:42 ` [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Jens Axboe
3 siblings, 0 replies; 7+ messages in thread
From: Pavel Begunkov @ 2021-10-18 20:37 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe, Christoph Hellwig, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/3] blk_mq_rq_ctx_init() optimisations
2021-10-18 20:37 [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Pavel Begunkov
` (2 preceding siblings ...)
2021-10-18 20:37 ` [PATCH 3/3] block: cache rq_flags inside blk_mq_rq_ctx_init() Pavel Begunkov
@ 2021-10-18 21:42 ` Jens Axboe
3 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2021-10-18 21:42 UTC (permalink / raw)
To: Pavel Begunkov, linux-block; +Cc: Jens Axboe, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx
2021-10-18 20:37 ` [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx Pavel Begunkov
@ 2021-10-19 6:00 ` Christoph Hellwig
2021-10-19 10:38 ` Pavel Begunkov
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2021-10-19 6:00 UTC (permalink / raw)
To: Pavel Begunkov; +Cc: linux-block, Jens Axboe, 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx
2021-10-19 6:00 ` Christoph Hellwig
@ 2021-10-19 10:38 ` Pavel Begunkov
0 siblings, 0 replies; 7+ messages in thread
From: Pavel Begunkov @ 2021-10-19 10:38 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, Jens Axboe
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-19 10:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18 20:37 [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Pavel Begunkov
2021-10-18 20:37 ` [PATCH 1/3] block: skip elevator fields init for non-elv queue Pavel Begunkov
2021-10-18 20:37 ` [PATCH 2/3] block: blk_mq_rq_ctx_init cache ctx/q/hctx Pavel Begunkov
2021-10-19 6:00 ` Christoph Hellwig
2021-10-19 10:38 ` Pavel Begunkov
2021-10-18 20:37 ` [PATCH 3/3] block: cache rq_flags inside blk_mq_rq_ctx_init() Pavel Begunkov
2021-10-18 21:42 ` [PATCH 0/3] blk_mq_rq_ctx_init() optimisations Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).