From: John Garry <hidden> Date: 2021-07-14 15:11:17
Currently a full set of static requests are allocated per hw queue per
tagset when shared sbitmap is used.
However, only tagset->queue_depth number of requests may be active at
any given time. As such, only tagset->queue_depth number of static
requests are required.
The same goes for using an IO scheduler, which allocates a full set of
static requests per hw queue per request queue.
This series very significantly reduces memory usage in both scenarios by
allocating static rqs per tagset and per request queue, respectively,
rather than per hw queue per tagset and per request queue.
For megaraid sas driver on my 128-CPU arm64 system with 1x SATA disk, we
save approx. 300MB(!) [370MB -> 60MB]
A couple of patches are marked as RFC, as maybe there is a better
implementation approach.
Any more testing would be appreciated also.
John Garry (9):
blk-mq: Change rqs check in blk_mq_free_rqs()
block: Rename BLKDEV_MAX_RQ -> BLKDEV_DEFAULT_RQ
blk-mq: Relocate shared sbitmap resize in blk_mq_update_nr_requests()
blk-mq: Add blk_mq_tag_resize_sched_shared_sbitmap()
blk-mq: Invert check in blk_mq_update_nr_requests()
blk-mq: Refactor blk_mq_{alloc,free}_rqs
blk-mq: Allocate per tag set static rqs for shared sbitmap
blk-mq: Allocate per request queue static rqs for shared sbitmap
blk-mq: Clear mappings for shared sbitmap sched static rqs
block/blk-core.c | 2 +-
block/blk-mq-sched.c | 57 ++++++++++++--
block/blk-mq-sched.h | 2 +-
block/blk-mq-tag.c | 22 ++++--
block/blk-mq-tag.h | 1 +
block/blk-mq.c | 165 +++++++++++++++++++++++++++++++----------
block/blk-mq.h | 9 +++
drivers/block/rbd.c | 2 +-
include/linux/blk-mq.h | 4 +
include/linux/blkdev.h | 6 +-
10 files changed, 215 insertions(+), 55 deletions(-)
--
2.26.2
From: John Garry <hidden> Date: 2021-07-14 15:11:22
The original code in commit 24d2f90309b23 ("blk-mq: split out tag
initialization, support shared tags") would check tags->rqs is non-NULL and
then dereference tags->rqs[].
Then in commit 2af8cbe30531 ("blk-mq: split tag ->rqs[] into two"), we
started to dereference tags->static_rqs[], but continued to check non-NULL
tags->rqs.
Check tags->static_rqs as non-NULL instead, which is more logical.
Signed-off-by: John Garry <redacted>
---
block/blk-mq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Garry <hidden> Date: 2021-07-14 15:11:26
It is a bit confusing that there is BLKDEV_MAX_RQ and MAX_SCHED_RQ, as
the name BLKDEV_MAX_RQ would imply the max requests always, which it is
not.
Rename to BLKDEV_MAX_RQ to BLKDEV_DEFAULT_RQ, matching its usage - that being
the default number of requests assigned when allocating a request queue.
Signed-off-by: John Garry <redacted>
---
block/blk-core.c | 2 +-
block/blk-mq-sched.c | 2 +-
block/blk-mq-sched.h | 2 +-
drivers/block/rbd.c | 2 +-
include/linux/blkdev.h | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
From: John Garry <hidden> Date: 2021-07-14 15:11:30
For shared sbitmap, if the call to blk_mq_tag_update_depth() was
successful for any hctx when hctx->sched_tags is not set, then it would be
successful for all (due to nature in which blk_mq_tag_update_depth()
fails).
As such, there is no need to call blk_mq_tag_resize_shared_sbitmap() for
each hctx. So relocate the call until after the hctx iteration under the
!q->elevator check, which is equivalent (to !hctx->sched_tags).
Signed-off-by: John Garry <redacted>
---
block/blk-mq.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
From: John Garry <hidden> Date: 2021-07-14 15:11:35
Put the functionality to resize the sched shared sbitmap in a common
function.
Since the same formula is always used to resize, and it can be got from
the request queue argument, so just pass the request queue pointer.
Signed-off-by: John Garry <redacted>
---
block/blk-mq-sched.c | 3 +--
block/blk-mq-tag.c | 10 ++++++++++
block/blk-mq-tag.h | 1 +
block/blk-mq.c | 3 +--
4 files changed, 13 insertions(+), 4 deletions(-)
From: John Garry <hidden> Date: 2021-07-14 15:11:40
It will be required to be able to allocate and free static requests for a
tag set or a request queue (in addition to a struct blk_mq_tags), so break
out the common functionality of blk_mq_{alloc,free}_rqs, passing the
required static rqs ** and page list pointer to the core code.
Signed-off-by: John Garry <redacted>
---
block/blk-mq.c | 61 +++++++++++++++++++++++++++++++++-----------------
block/blk-mq.h | 7 ++++++
2 files changed, 47 insertions(+), 21 deletions(-)
@@ -2310,14 +2310,15 @@ static size_t order_to_size(unsigned int order)}/* called before freeing request pool in @tags */-staticvoidblk_mq_clear_rq_mapping(structblk_mq_tag_set*set,-structblk_mq_tags*tags,unsignedinthctx_idx)+voidblk_mq_clear_rq_mapping(structblk_mq_tag_set*set,+unsignedinthctx_idx,+structlist_head*page_list){structblk_mq_tags*drv_tags=set->tags[hctx_idx];structpage*page;unsignedlongflags;-list_for_each_entry(page,&tags->page_list,lru){+list_for_each_entry(page,page_list,lru){unsignedlongstart=(unsignedlong)page_address(page);unsignedlongend=start+order_to_size(page->private);inti;
From: John Garry <hidden> Date: 2021-07-14 15:11:42
It is wasteful for memory that a full set of static rqs is allocated per
hw queue for shared sbitmap, considering that the total number of requests
usable at any given time across all hctx is limited by the shared sbitmap
depth.
As such, it is considerably more memory efficient in the case of shared
sbitmap to allocate a set of static rqs per tag set, and not per hw queue.
Make this change, and ensure that the hctx_idx argument to
blk_mq_init_request() and __blk_mq_free_rqs() is == 0 for case of
shared sbitmap. If a driver needs to init the static rq with a hctx idx,
then it cannot now use shared sbitmap - only SCSI drivers and null block
use shared sbitmap today, and neither use hctx_idx for static request init
or free.
Signed-off-by: John Garry <redacted>
---
block/blk-mq.c | 66 +++++++++++++++++++++++++++++++++++++-----
include/linux/blk-mq.h | 4 +++
2 files changed, 62 insertions(+), 8 deletions(-)
From: John Garry <hidden> Date: 2021-07-14 15:11:43
Similar to allocating a full set of static rqs per hw queue per tag set for
when using a shared sbitmap, it is also inefficient in memory terms to
allocate a full set of static rqs per hw queue per request queue.
Reduce memory usage by allocating a set of static rqs per request queue
for when using a shared sbitmap, and make the per-hctx
sched_tags->static_rqs[] point at them.
Error handling for updating the number of requests in
blk_mq_update_nr_requests() -> blk_mq_tag_update_depth() can get quite
complicated, so allocate the full max depth of rqs at init time to try to
simplify things. This will be somewhat inefficient for when the request
queue depth is not close to max, but generally still more efficient than
the current situation.
For failures in blk_mq_update_nr_requests() -> blk_mq_tag_update_depth()
when shrinking request queue depth, q->nr_requests still needs to be
updated. This is because some of the hctx->sched_tags may be successfully
updated, and now they are now smaller than q->nr_requests, which will lead
to problems since a scheduler tag could be greater than hctx->sched_tags
size.
For failures in blk_mq_update_nr_requests() -> blk_mq_tag_update_depth()
when growing a request queue depth, q->nr_requests does not need to be
updated.
Signed-off-by: John Garry <redacted>
---
block/blk-mq-sched.c | 51 ++++++++++++++++++++++++++++++++++++++----
block/blk-mq-tag.c | 12 +++++-----
block/blk-mq.c | 20 ++++++++++++++++-
include/linux/blkdev.h | 4 ++++
4 files changed, 76 insertions(+), 11 deletions(-)
From: John Garry <hidden> Date: 2021-07-14 15:11:45
To avoid use-after-free in accessing stale requests in the driver tags
rqs[], clear the mappings for the request queue static rqs.
Signed-off-by: John Garry <redacted>
---
block/blk-mq-sched.c | 1 +
block/blk-mq.h | 2 ++
2 files changed, 3 insertions(+)
On Wed, Jul 14, 2021 at 11:06:27PM +0800, John Garry wrote:
quoted hunk
The original code in commit 24d2f90309b23 ("blk-mq: split out tag
initialization, support shared tags") would check tags->rqs is non-NULL and
then dereference tags->rqs[].
Then in commit 2af8cbe30531 ("blk-mq: split tag ->rqs[] into two"), we
started to dereference tags->static_rqs[], but continued to check non-NULL
tags->rqs.
Check tags->static_rqs as non-NULL instead, which is more logical.
Signed-off-by: John Garry <redacted>
---
block/blk-mq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Yeah, it is reasonable to check ->static_rqs since both ->init_request()
and ->exit_request() operate on request from ->static_rqs[]:
Reviewed-by: Ming Lei <redacted>
--
Ming
On Wed, Jul 14, 2021 at 11:06:28PM +0800, John Garry wrote:
quoted hunk
It is a bit confusing that there is BLKDEV_MAX_RQ and MAX_SCHED_RQ, as
the name BLKDEV_MAX_RQ would imply the max requests always, which it is
not.
Rename to BLKDEV_MAX_RQ to BLKDEV_DEFAULT_RQ, matching its usage - that being
the default number of requests assigned when allocating a request queue.
Signed-off-by: John Garry <redacted>
---
block/blk-core.c | 2 +-
block/blk-mq-sched.c | 2 +-
block/blk-mq-sched.h | 2 +-
drivers/block/rbd.c | 2 +-
include/linux/blkdev.h | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
The above assignment isn't necessary since bio based queue doesn't use
->nr_requests. For request based queue, ->nr_requests will be re-set
in either blk_mq_init_sched() or blk_mq_init_allocated_queue(), but
that may not be related with this patch itself.
On Wed, Jul 14, 2021 at 11:06:29PM +0800, John Garry wrote:
quoted hunk
For shared sbitmap, if the call to blk_mq_tag_update_depth() was
successful for any hctx when hctx->sched_tags is not set, then it would be
successful for all (due to nature in which blk_mq_tag_update_depth()
fails).
As such, there is no need to call blk_mq_tag_resize_shared_sbitmap() for
each hctx. So relocate the call until after the hctx iteration under the
!q->elevator check, which is equivalent (to !hctx->sched_tags).
Signed-off-by: John Garry <redacted>
---
block/blk-mq.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
On Wed, Jul 14, 2021 at 11:06:30PM +0800, John Garry wrote:
quoted hunk
Put the functionality to resize the sched shared sbitmap in a common
function.
Since the same formula is always used to resize, and it can be got from
the request queue argument, so just pass the request queue pointer.
Signed-off-by: John Garry <redacted>
---
block/blk-mq-sched.c | 3 +--
block/blk-mq-tag.c | 10 ++++++++++
block/blk-mq-tag.h | 1 +
block/blk-mq.c | 3 +--
4 files changed, 13 insertions(+), 4 deletions(-)
@@ -3624,8 +3624,6 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)if(!hctx->sched_tags){ret=blk_mq_tag_update_depth(hctx,&hctx->tags,nr,false);-if(!ret&&blk_mq_is_sbitmap_shared(set->flags))-blk_mq_tag_resize_shared_sbitmap(set,nr);}else{ret=blk_mq_tag_update_depth(hctx,&hctx->sched_tags,nr,true);
@@ -3643,9 +3641,14 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)}if(!ret){q->nr_requests=nr;-if(q->elevator&&blk_mq_is_sbitmap_shared(set->flags))-sbitmap_queue_resize(&q->sched_bitmap_tags,-nr-set->reserved_tags);+if(blk_mq_is_sbitmap_shared(set->flags)){
Hi Ming,
quoted
+ if (q->elevator) {+ sbitmap_queue_resize(&q->sched_bitmap_tags,+ nr - set->reserved_tags);
I have learned that some people prefer {} for multi-line single statements, like this.
Anyway, more code is added here later in the series, so better to add {} now, rather than re-arrange code later.
On Wed, Jul 14, 2021 at 11:06:26PM +0800, John Garry wrote:
Currently a full set of static requests are allocated per hw queue per
tagset when shared sbitmap is used.
However, only tagset->queue_depth number of requests may be active at
any given time. As such, only tagset->queue_depth number of static
requests are required.
The same goes for using an IO scheduler, which allocates a full set of
static requests per hw queue per request queue.
This series very significantly reduces memory usage in both scenarios by
allocating static rqs per tagset and per request queue, respectively,
rather than per hw queue per tagset and per request queue.
For megaraid sas driver on my 128-CPU arm64 system with 1x SATA disk, we
save approx. 300MB(!) [370MB -> 60MB]
A couple of patches are marked as RFC, as maybe there is a better
implementation approach.
There is another candidate for addressing this issue, and looks simpler:
block/blk-mq-sched.c | 4 ++++
block/blk-mq-tag.c | 4 ++++
block/blk-mq-tag.h | 3 +++
block/blk-mq.c | 18 ++++++++++++++++++
block/blk-mq.h | 11 +++++++++++
5 files changed, 40 insertions(+)
@@ -21,6 +21,9 @@ struct blk_mq_tags {structrequest**static_rqs;structlist_headpage_list;+/* only used for blk_mq_is_sbitmap_shared() */+structblk_mq_tags*master;+/**usedtoclearrequestreferenceinrqs[]beforefreeingone*requestpool
From: John Garry <hidden> Date: 2021-07-20 17:07:57
On 20/07/2021 16:22, Ming Lei wrote:
On Wed, Jul 14, 2021 at 11:06:26PM +0800, John Garry wrote:
quoted
Currently a full set of static requests are allocated per hw queue per
tagset when shared sbitmap is used.
However, only tagset->queue_depth number of requests may be active at
any given time. As such, only tagset->queue_depth number of static
requests are required.
The same goes for using an IO scheduler, which allocates a full set of
static requests per hw queue per request queue.
This series very significantly reduces memory usage in both scenarios by
allocating static rqs per tagset and per request queue, respectively,
rather than per hw queue per tagset and per request queue.
For megaraid sas driver on my 128-CPU arm64 system with 1x SATA disk, we
save approx. 300MB(!) [370MB -> 60MB]
A couple of patches are marked as RFC, as maybe there is a better
implementation approach.
Hi Ming,
To be clear, my concerns with my implementation were:
a. Interface of __blk_mq_{alloc,free}_rqs was a bit strange in passing a struct list_head * and struct request ** . But maybe it's ok.
b. We need to ensure that the host should not expect a set of static rqs per hw queue, i.e. should not use blk_mq_ops.init_request hctx_idx argument. The guard I have added is effectively useless against that. I am thinking that we should have a variant of blk_mq_ops.init_request without a hctx_idx argument, which needs to be used for shared sbitmap.
There is another candidate for addressing this issue, and looks simpler:
Thanks, I think that this solution does not deal with b., above, either.
@@ -21,6 +21,9 @@ struct blk_mq_tags {structrequest**static_rqs;structlist_headpage_list;+/* only used for blk_mq_is_sbitmap_shared() */+structblk_mq_tags*master;
It is a bit odd to have a pointer to struct blk_mq_tags in struct blk_mq_tags like this
quoted hunk
+ /* * used to clear request reference in rqs[] before freeing one * request pool