blk-mq: optimise plugging
cover blk-mq: optimise plugging
Clean and optimise blk_mq_flush_plug_list().
Pavel Begunkov (3):
blk-mq: optimise rq sort function
list: introduce list_for_each_continue()
blk-mq: optimise blk_mq_flush_plug_list()
block/blk-mq.c | 69 +++++++++++++++----------------------------- include/linux/list.h | 10 +++++++ 2 files changed, 33 insertions(+), 46 deletions(-)
Signature · 1 line
2.24.0
PATCH 1/3 blk-mq: optimise rq sort function
Check "!=" in multi-layer comparisons. The same memory usage, fewer instructions, and 2 from 4 jumps are replaced with SETcc.
Note, that list_sort() doesn't differ 0 and <0.
--- block/blk-mq.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
block/blk-mq.c +4 -8
@@ -1668,14 +1668,10 @@ static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b) struct request *rqa = container_of(a, struct request, queuelist); struct request *rqb = container_of(b, struct request, queuelist); -if (rqa->mq_ctx < rqb->mq_ctx) -return -1 ; -else if (rqa->mq_ctx > rqb->mq_ctx) -return 1 ; -else if (rqa->mq_hctx < rqb->mq_hctx) -return -1 ; -else if (rqa->mq_hctx > rqb->mq_hctx) -return 1 ; +if (rqa->mq_ctx != rqb->mq_ctx) +return rqa->mq_ctx > rqb->mq_ctx; +if (rqa->mq_hctx != rqb->mq_hctx) +return rqa->mq_hctx > rqb->mq_hctx;return blk_rq_pos (rqa) >blk_rq_pos (rqb); }
Signature · 1 line
2.24.0
Re: [PATCH 1/3] blk-mq: optimise rq sort function
Quoted 6 lines · Pavel Begunkov
Check "!=" in multi-layer comparisons. The same memory usage, fewer instructions, and 2 from 4 jumps are replaced with SETcc.
Note, that
list_sort()doesn't differ 0 and <0.Signed-off-by Pavel Begunkov <[email protected]>
My first reaction was this is wrong since you no longer return negative values. But then I looked into list_sort/merge and this branch 'if (cmp(priv, a, b) <= 0) {' clearly shows this is correct.
So :
Quoted 28 lines
--- block/blk-mq.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)block/blk-mq.c +4
-8@@ -1668,14 +1668,10 @@ static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b) struct request *rqa = container_of(a, struct request, queuelist); struct request *rqb = container_of(b, struct request, queuelist); -if (rqa->mq_ctx < rqb->mq_ctx) -return -1 ; -else if (rqa->mq_ctx > rqb->mq_ctx) -return 1 ; -else if (rqa->mq_hctx < rqb->mq_hctx) -return -1 ; -else if (rqa->mq_hctx > rqb->mq_hctx) -return 1 ; +if (rqa->mq_ctx != rqb->mq_ctx) +return rqa->mq_ctx > rqb->mq_ctx; +if (rqa->mq_hctx != rqb->mq_hctx) +return rqa->mq_hctx > rqb->mq_hctx;return blk_rq_pos (rqa) >blk_rq_pos (rqb); }
Re: [PATCH 1/3] blk-mq: optimise rq sort function
Quoted 11 lines · Nikolay Borisov
Quoted 6 lines · Pavel Begunkov
Check "!=" in multi-layer comparisons. The same memory usage, fewer instructions, and 2 from 4 jumps are replaced with SETcc.
Note, that
list_sort()doesn't differ 0 and <0.Signed-off-by Pavel Begunkov <[email protected]>My first reaction was this is wrong since you no longer return negative values. But then I looked into list_sort/merge and this branch 'if (
cmp(priv, a, b)<= 0) {' clearly shows this is correct.
Yes, that's why there is a note in the patch description. The same is told by list_sort() description.
Quoted 4 lines
So :
Reviewed-by Nikolay Borisov <[email protected]>
Thanks for taking a look
Quoted 29 lines
Quoted 29 lines
--- block/blk-mq.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)block/blk-mq.c +4
-8@@ -1668,14 +1668,10 @@ static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b) struct request *rqa = container_of(a, struct request, queuelist); struct request *rqb = container_of(b, struct request, queuelist); -if (rqa->mq_ctx < rqb->mq_ctx) -return -1 ; -else if (rqa->mq_ctx > rqb->mq_ctx) -return 1 ; -else if (rqa->mq_hctx < rqb->mq_hctx) -return -1 ; -else if (rqa->mq_hctx > rqb->mq_hctx) -return 1 ; +if (rqa->mq_ctx != rqb->mq_ctx) +return rqa->mq_ctx > rqb->mq_ctx; +if (rqa->mq_hctx != rqb->mq_hctx) +return rqa->mq_hctx > rqb->mq_hctx;return blk_rq_pos (rqa) >blk_rq_pos (rqb); }
Signature · 1 line
Pavel Begunkov
Re: [PATCH 1/3] blk-mq: optimise rq sort function
Quoted 7 lines · Pavel Begunkov
Quoted 6 lines · Nikolay Borisov
Quoted 5 lines · Pavel Begunkov
Check "!=" in multi-layer comparisons. The same memory usage, fewer instructions, and 2 from 4 jumps are replaced with SETcc.
Note, that
list_sort()doesn't differ 0 and <0.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Pavel Begunkov <[email protected]>My first reaction was this is wrong since you no longer return negative values. But then I looked into list_sort/merge and this branch 'if (
cmp(priv, a, b)<= 0) {' clearly shows this is correct.Yes, that's why there is a note in the patch description. The same is told by
list_sort()description.
And of course I have missed your remark in the changelog :)
Quoted 38 lines
Quoted 5 lines
So :
Reviewed-by Nikolay Borisov <[email protected]>Thanks for taking a look
Quoted 29 lines
Quoted 29 lines
--- block/blk-mq.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)block/blk-mq.c +4
-8@@ -1668,14 +1668,10 @@ static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b) struct request *rqa = container_of(a, struct request, queuelist); struct request *rqb = container_of(b, struct request, queuelist); -if (rqa->mq_ctx < rqb->mq_ctx) -return -1 ; -else if (rqa->mq_ctx > rqb->mq_ctx) -return 1 ; -else if (rqa->mq_hctx < rqb->mq_hctx) -return -1 ; -else if (rqa->mq_hctx > rqb->mq_hctx) -return 1 ; +if (rqa->mq_ctx != rqb->mq_ctx) +return rqa->mq_ctx > rqb->mq_ctx; +if (rqa->mq_hctx != rqb->mq_hctx) +return rqa->mq_hctx > rqb->mq_hctx;return blk_rq_pos (rqa) >blk_rq_pos (rqb); }
PATCH 2/3 list: introduce list_for_each_continue()
In reply to Pavel BegunkovAs other *continue() helpers, this continues iteration from a given position.
--- include/linux/list.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
include/linux/list.h +10 -0
@@ -538,6 +538,16 @@ static inline void list_splice_tail_init(struct list_head *list,#define list_for_each (pos, head) \ for (pos = (head)->next; pos != (head); pos = pos->next) +/** +* list_for_each_continue - continue iteration over a list +* @pos: the &struct list_head to use as a loop cursor. +* @head: the head for your list. +* +* Continue to iterate over a list, continuing after the current position. +*/ +#define list_for_each_continue (pos, head) \ + for (pos = pos->next; pos != (head); pos = pos->next) +/** * list_for_each_prev - iterate over a list backwards * @pos: the &struct list_head to use as a loop cursor.
Signature · 1 line
2.24.0
PATCH 3/3 blk-mq: optimise blk_mq_flush_plug_list()
In reply to Pavel BegunkovInstead of using list_del_init() in a loop, that generates a lot of unnecessary memory read/writes, iterate from the first request of a batch and cut out a sublist with list_cut_before().
Apart from removing the list node initialisation part, this is more register-friendly, and the assembly uses the stack less intensively.
list_empty() at the beginning is done with hope, that the compiler can optimise out the same check in the following list_splice_init().
--- block/blk-mq.c | 57 +++++++++++++++++--------------------------------- 1 file changed, 19 insertions(+), 38 deletions(-)
block/blk-mq.c +19 -38
@@ -1678,14 +1678,10 @@ static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b)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_queue *this_q; -struct request *rq;LIST_HEAD (list); -LIST_HEAD (rq_list); -unsigned int depth; +if (list_empty (&plug->mq_list)) +return ;list_splice_init (&plug->mq_list, &list);if (plug->rq_count >2 && plug->multiple_queues)
@@ -1693,42 +1689,27 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule) plug->rq_count =0 ; - this_q =NULL ; - this_hctx =NULL ; - this_ctx =NULL ; - depth =0 ; - -while (!list_empty (&list)) { - rq =list_entry_rq (list.next); -list_del_init (&rq->queuelist); -BUG_ON (!rq->q); -if (rq->mq_hctx != this_hctx || rq->mq_ctx != this_ctx) { -if (this_hctx) { -trace_block_unplug (this_q, depth, !from_schedule); -blk_mq_sched_insert_requests (this_hctx, this_ctx, - &rq_list, - from_schedule); - } - - this_q = rq->q; - this_ctx = rq->mq_ctx; - this_hctx = rq->mq_hctx; - depth =0 ; +do { +struct list_head rq_list; +struct request *rq, *head_rq =list_entry_rq (list.next); +struct list_head *pos = &head_rq->queuelist;/* skip first */ +struct blk_mq_hw_ctx *this_hctx = head_rq->mq_hctx; +struct blk_mq_ctx *this_ctx = head_rq->mq_ctx; +unsigned int depth =1 ; + +list_for_each_continue (pos, &list) { + rq =list_entry_rq (pos); +BUG_ON (!rq->q); +if (rq->mq_hctx != this_hctx || rq->mq_ctx != this_ctx) +break ; + depth++; } - depth++; -list_add_tail (&rq->queuelist, &rq_list); - } - -/* -* If 'this_hctx' is set, we know we have entries to complete -* on 'rq_list'. Do those. -*/ -if (this_hctx) { -trace_block_unplug (this_q, depth, !from_schedule); +list_cut_before (&rq_list, &list, pos); +trace_block_unplug (head_rq->q, depth, !from_schedule);blk_mq_sched_insert_requests (this_hctx, this_ctx, &rq_list, from_schedule); - } + }while (!list_empty (&list)); } static void blk_mq_bio_to_request(struct request *rq, struct bio *bio,
Signature · 1 line
2.24.0
Quoted 2 lines · Pavel Begunkov
Clean and optimise
blk_mq_flush_plug_list().
ping
Quoted 9 lines
Pavel Begunkov (3):
blk-mq: optimise rq sort function
list: introducelist_for_each_continue()
blk-mq: optimiseblk_mq_flush_plug_list()block/blk-mq.c | 69 +++++++++++++++----------------------------- include/linux/list.h | 10 +++++++ 2 files changed, 33 insertions(+), 46 deletions(-)
Signature · 1 line
Pavel Begunkov
Jens Axboe
maintainerQuoted 4 lines · Pavel Begunkov
Quoted 2 lines · Pavel Begunkov
Clean and optimise
blk_mq_flush_plug_list().ping
Looks good to me, I've been waiting a bit on this as I'll queue it up for 5.6.
Signature · 1 line
Jens Axboe