Message ID | 1632472110-244938-12-git-send-email-john.garry@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | blk-mq: Reduce static requests memory footprint for shared sbitmap | expand |
On Fri, Sep 24, 2021 at 04:28:28PM +0800, John Garry wrote: > Refactor blk_mq_free_map_and_requests() such that it can be used at many > sites at which the tag map and rqs are freed. > > Also rename to blk_mq_free_map_and_rqs(), which is shorter and matches the > alloc equivalent. > > Suggested-by: Ming Lei <ming.lei@redhat.com> > Signed-off-by: John Garry <john.garry@huawei.com> > Reviewed-by: Hannes Reinecke <hare@suse.de> > --- > block/blk-mq-tag.c | 3 +-- > block/blk-mq.c | 40 ++++++++++++++++++++++++---------------- > block/blk-mq.h | 4 +++- > 3 files changed, 28 insertions(+), 19 deletions(-) > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c > index db99f1246795..a0ecc6d88f84 100644 > --- a/block/blk-mq-tag.c > +++ b/block/blk-mq-tag.c > @@ -607,8 +607,7 @@ int blk_mq_tag_update_depth(struct blk_mq_hw_ctx *hctx, > if (!new) > return -ENOMEM; > > - blk_mq_free_rqs(set, *tagsptr, hctx->queue_num); > - blk_mq_free_rq_map(*tagsptr, set->flags); > + blk_mq_free_map_and_rqs(set, *tagsptr, hctx->queue_num); > *tagsptr = new; > } else { > /* > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 46772773b9c4..464ea20b9bcb 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2878,15 +2878,15 @@ static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, > return set->tags[hctx_idx]; > } > > -static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set, > - unsigned int hctx_idx) > +void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, > + struct blk_mq_tags *tags, > + unsigned int hctx_idx) > { > unsigned int flags = set->flags; > > - if (set->tags && set->tags[hctx_idx]) { > - blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx); > - blk_mq_free_rq_map(set->tags[hctx_idx], flags); > - set->tags[hctx_idx] = NULL; > + if (tags) { > + blk_mq_free_rqs(set, tags, hctx_idx); > + blk_mq_free_rq_map(tags, flags); > } > } > > @@ -2967,8 +2967,10 @@ static void blk_mq_map_swqueue(struct request_queue *q) > * fallback in case of a new remap fails > * allocation > */ > - if (i && set->tags[i]) > - blk_mq_free_map_and_requests(set, i); > + if (i && set->tags[i]) { > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > + set->tags[i] = NULL; > + } > > hctx->tags = NULL; > continue; > @@ -3264,8 +3266,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set, > struct blk_mq_hw_ctx *hctx = hctxs[j]; > > if (hctx) { > - if (hctx->tags) > - blk_mq_free_map_and_requests(set, j); > + blk_mq_free_map_and_rqs(set, set->tags[j], j); > + set->tags[j] = NULL; > blk_mq_exit_hctx(q, set, hctx, j); > hctxs[j] = NULL; > } > @@ -3361,8 +3363,10 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) > return 0; > > out_unwind: > - while (--i >= 0) > - blk_mq_free_map_and_requests(set, i); > + while (--i >= 0) { > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > + set->tags[i] = NULL; > + } > > return -ENOMEM; > } > @@ -3557,8 +3561,10 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set) > return 0; > > out_free_mq_rq_maps: > - for (i = 0; i < set->nr_hw_queues; i++) > - blk_mq_free_map_and_requests(set, i); > + for (i = 0; i < set->nr_hw_queues; i++) { > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > + set->tags[i] = NULL; > + } > out_free_mq_map: > for (i = 0; i < set->nr_maps; i++) { > kfree(set->map[i].mq_map); > @@ -3590,8 +3596,10 @@ void blk_mq_free_tag_set(struct blk_mq_tag_set *set) > { > int i, j; > > - for (i = 0; i < set->nr_hw_queues; i++) > - blk_mq_free_map_and_requests(set, i); > + for (i = 0; i < set->nr_hw_queues; i++) { > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > + set->tags[i] = NULL; > + } There are 5 callers in which 'set->tags[i]' is cleared, so just wondering why you don't clear set->tags[i] at default in blk_mq_free_map_and_rqs(). And just call __blk_mq_free_map_and_rqs() for the only other user? Thanks, Ming
On 26/09/2021 03:05, Ming Lei wrote: > On Fri, Sep 24, 2021 at 04:28:28PM +0800, John Garry wrote: >> Refactor blk_mq_free_map_and_requests() such that it can be used at many >> sites at which the tag map and rqs are freed. >> >> Also rename to blk_mq_free_map_and_rqs(), which is shorter and matches the >> alloc equivalent. >> >> Suggested-by: Ming Lei<ming.lei@redhat.com> >> Signed-off-by: John Garry<john.garry@huawei.com> >> Reviewed-by: Hannes Reinecke<hare@suse.de> >> --- >> block/blk-mq-tag.c | 3 +-- >> block/blk-mq.c | 40 ++++++++++++++++++++++++---------------- >> block/blk-mq.h | 4 +++- >> 3 files changed, 28 insertions(+), 19 deletions(-) >> >> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c >> index db99f1246795..a0ecc6d88f84 100644 >> --- a/block/blk-mq-tag.c >> +++ b/block/blk-mq-tag.c >> @@ -607,8 +607,7 @@ int blk_mq_tag_update_depth(struct blk_mq_hw_ctx *hctx, >> if (!new) >> return -ENOMEM; >> >> - blk_mq_free_rqs(set, *tagsptr, hctx->queue_num); >> - blk_mq_free_rq_map(*tagsptr, set->flags); >> + blk_mq_free_map_and_rqs(set, *tagsptr, hctx->queue_num); >> *tagsptr = new; >> } else { >> /* >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 46772773b9c4..464ea20b9bcb 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -2878,15 +2878,15 @@ static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, >> return set->tags[hctx_idx]; >> } >> >> -static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set, >> - unsigned int hctx_idx) >> +void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, >> + struct blk_mq_tags *tags, >> + unsigned int hctx_idx) >> { >> unsigned int flags = set->flags; >> >> - if (set->tags && set->tags[hctx_idx]) { >> - blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx); >> - blk_mq_free_rq_map(set->tags[hctx_idx], flags); >> - set->tags[hctx_idx] = NULL; >> + if (tags) { >> + blk_mq_free_rqs(set, tags, hctx_idx); >> + blk_mq_free_rq_map(tags, flags); >> } >> } >> >> @@ -2967,8 +2967,10 @@ static void blk_mq_map_swqueue(struct request_queue *q) >> * fallback in case of a new remap fails >> * allocation >> */ >> - if (i && set->tags[i]) >> - blk_mq_free_map_and_requests(set, i); >> + if (i && set->tags[i]) { >> + blk_mq_free_map_and_rqs(set, set->tags[i], i); >> + set->tags[i] = NULL; >> + } >> >> hctx->tags = NULL; >> continue; >> @@ -3264,8 +3266,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set, >> struct blk_mq_hw_ctx *hctx = hctxs[j]; >> >> if (hctx) { >> - if (hctx->tags) >> - blk_mq_free_map_and_requests(set, j); >> + blk_mq_free_map_and_rqs(set, set->tags[j], j); >> + set->tags[j] = NULL; >> blk_mq_exit_hctx(q, set, hctx, j); >> hctxs[j] = NULL; >> } >> @@ -3361,8 +3363,10 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) >> return 0; >> >> out_unwind: >> - while (--i >= 0) >> - blk_mq_free_map_and_requests(set, i); >> + while (--i >= 0) { >> + blk_mq_free_map_and_rqs(set, set->tags[i], i); >> + set->tags[i] = NULL; >> + } >> >> return -ENOMEM; >> } >> @@ -3557,8 +3561,10 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set) >> return 0; >> >> out_free_mq_rq_maps: >> - for (i = 0; i < set->nr_hw_queues; i++) >> - blk_mq_free_map_and_requests(set, i); >> + for (i = 0; i < set->nr_hw_queues; i++) { >> + blk_mq_free_map_and_rqs(set, set->tags[i], i); >> + set->tags[i] = NULL; >> + } >> out_free_mq_map: >> for (i = 0; i < set->nr_maps; i++) { >> kfree(set->map[i].mq_map); >> @@ -3590,8 +3596,10 @@ void blk_mq_free_tag_set(struct blk_mq_tag_set *set) >> { >> int i, j; >> >> - for (i = 0; i < set->nr_hw_queues; i++) >> - blk_mq_free_map_and_requests(set, i); >> + for (i = 0; i < set->nr_hw_queues; i++) { >> + blk_mq_free_map_and_rqs(set, set->tags[i], i); >> + set->tags[i] = NULL; >> + } > There are 5 callers in which 'set->tags[i]' is cleared, so just > wondering why you don't clear set->tags[i] at default in > blk_mq_free_map_and_rqs(). And just call __blk_mq_free_map_and_rqs() > for the only other user? blk_mq_free_map_and_rqs() is not always passed set->tags[i]: - blk_mq_tag_update_depth() calls blk_mq_free_map_and_rqs() for sched tags - __blk_mq_alloc_rq_maps() calls blk_mq_free_map_and_rqs() for shared_sbitmap_tags Function __blk_mq_free_map_and_rqs() is not public and really only intended for set->tags[i] So I did consider passing the address of the tags pointer to blk_mq_free_map_and_rqs(), like: void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, struct blk_mq_tag **tags, unsigned int hctx_idx) { ... *tags = NULL; } But then the API becomes a bit asymmetric, as we deal with tags pointer normally: struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, unsigned int hctx_idx, unsigned int depth); However, apart from this, I can change __blk_mq_free_map_and_rqs() to NULLify set->tags[i], as it is always passed set->tags[i]. Do you have a preference? Thanks, John
On Mon, Sep 27, 2021 at 10:02:40AM +0100, John Garry wrote: > On 26/09/2021 03:05, Ming Lei wrote: > > On Fri, Sep 24, 2021 at 04:28:28PM +0800, John Garry wrote: > > > Refactor blk_mq_free_map_and_requests() such that it can be used at many > > > sites at which the tag map and rqs are freed. > > > > > > Also rename to blk_mq_free_map_and_rqs(), which is shorter and matches the > > > alloc equivalent. > > > > > > Suggested-by: Ming Lei<ming.lei@redhat.com> > > > Signed-off-by: John Garry<john.garry@huawei.com> > > > Reviewed-by: Hannes Reinecke<hare@suse.de> > > > --- > > > block/blk-mq-tag.c | 3 +-- > > > block/blk-mq.c | 40 ++++++++++++++++++++++++---------------- > > > block/blk-mq.h | 4 +++- > > > 3 files changed, 28 insertions(+), 19 deletions(-) > > > > > > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c > > > index db99f1246795..a0ecc6d88f84 100644 > > > --- a/block/blk-mq-tag.c > > > +++ b/block/blk-mq-tag.c > > > @@ -607,8 +607,7 @@ int blk_mq_tag_update_depth(struct blk_mq_hw_ctx *hctx, > > > if (!new) > > > return -ENOMEM; > > > - blk_mq_free_rqs(set, *tagsptr, hctx->queue_num); > > > - blk_mq_free_rq_map(*tagsptr, set->flags); > > > + blk_mq_free_map_and_rqs(set, *tagsptr, hctx->queue_num); > > > *tagsptr = new; > > > } else { > > > /* > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > > index 46772773b9c4..464ea20b9bcb 100644 > > > --- a/block/blk-mq.c > > > +++ b/block/blk-mq.c > > > @@ -2878,15 +2878,15 @@ static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, > > > return set->tags[hctx_idx]; > > > } > > > -static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set, > > > - unsigned int hctx_idx) > > > +void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, > > > + struct blk_mq_tags *tags, > > > + unsigned int hctx_idx) > > > { > > > unsigned int flags = set->flags; > > > - if (set->tags && set->tags[hctx_idx]) { > > > - blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx); > > > - blk_mq_free_rq_map(set->tags[hctx_idx], flags); > > > - set->tags[hctx_idx] = NULL; > > > + if (tags) { > > > + blk_mq_free_rqs(set, tags, hctx_idx); > > > + blk_mq_free_rq_map(tags, flags); > > > } > > > } > > > @@ -2967,8 +2967,10 @@ static void blk_mq_map_swqueue(struct request_queue *q) > > > * fallback in case of a new remap fails > > > * allocation > > > */ > > > - if (i && set->tags[i]) > > > - blk_mq_free_map_and_requests(set, i); > > > + if (i && set->tags[i]) { > > > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > > > + set->tags[i] = NULL; > > > + } > > > hctx->tags = NULL; > > > continue; > > > @@ -3264,8 +3266,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set, > > > struct blk_mq_hw_ctx *hctx = hctxs[j]; > > > if (hctx) { > > > - if (hctx->tags) > > > - blk_mq_free_map_and_requests(set, j); > > > + blk_mq_free_map_and_rqs(set, set->tags[j], j); > > > + set->tags[j] = NULL; > > > blk_mq_exit_hctx(q, set, hctx, j); > > > hctxs[j] = NULL; > > > } > > > @@ -3361,8 +3363,10 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) > > > return 0; > > > out_unwind: > > > - while (--i >= 0) > > > - blk_mq_free_map_and_requests(set, i); > > > + while (--i >= 0) { > > > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > > > + set->tags[i] = NULL; > > > + } > > > return -ENOMEM; > > > } > > > @@ -3557,8 +3561,10 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set) > > > return 0; > > > out_free_mq_rq_maps: > > > - for (i = 0; i < set->nr_hw_queues; i++) > > > - blk_mq_free_map_and_requests(set, i); > > > + for (i = 0; i < set->nr_hw_queues; i++) { > > > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > > > + set->tags[i] = NULL; > > > + } > > > out_free_mq_map: > > > for (i = 0; i < set->nr_maps; i++) { > > > kfree(set->map[i].mq_map); > > > @@ -3590,8 +3596,10 @@ void blk_mq_free_tag_set(struct blk_mq_tag_set *set) > > > { > > > int i, j; > > > - for (i = 0; i < set->nr_hw_queues; i++) > > > - blk_mq_free_map_and_requests(set, i); > > > + for (i = 0; i < set->nr_hw_queues; i++) { > > > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > > > + set->tags[i] = NULL; > > > + } > > There are 5 callers in which 'set->tags[i]' is cleared, so just > > wondering why you don't clear set->tags[i] at default in > > blk_mq_free_map_and_rqs(). And just call __blk_mq_free_map_and_rqs() > > for the only other user? > > blk_mq_free_map_and_rqs() is not always passed set->tags[i]: > > - blk_mq_tag_update_depth() calls blk_mq_free_map_and_rqs() for sched tags > > - __blk_mq_alloc_rq_maps() calls blk_mq_free_map_and_rqs() for > shared_sbitmap_tags > > Function __blk_mq_free_map_and_rqs() is not public and really only intended > for set->tags[i] > > So I did consider passing the address of the tags pointer to > blk_mq_free_map_and_rqs(), like: > > void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, > struct blk_mq_tag **tags, > unsigned int hctx_idx) > > { > ... > *tags = NULL; > } > > But then the API becomes a bit asymmetric, as we deal with tags pointer > normally: > > struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, > unsigned int hctx_idx, > unsigned int depth); > > > However, apart from this, I can change __blk_mq_free_map_and_rqs() to > NULLify set->tags[i], as it is always passed set->tags[i]. > > Do you have a preference? I meant there are 5 following uses in your patch: + blk_mq_free_map_and_rqs(set, set->tags[i], i); + set->tags[i] = NULL; and one new helper(blk_mq_free_set_map_and_rqs(set, i)?) can be added for just doing that, Thanks, Ming
On 27/09/2021 10:19, Ming Lei wrote: >> However, apart from this, I can change __blk_mq_free_map_and_rqs() to >> NULLify set->tags[i], as it is always passed set->tags[i]. >> >> Do you have a preference? > I meant there are 5 following uses in your patch: > > + blk_mq_free_map_and_rqs(set, set->tags[i], i); > + set->tags[i] = NULL; > > and one new helper(blk_mq_free_set_map_and_rqs(set, i)?) can be added for just > doing that, Ah, ok, but in the next patch we replace these blk_mq_free_map_and_rqs() calls with __blk_mq_free_map_and_rqs(), and __blk_mq_free_map_and_rqs() is always passed set->tags[i], so we do as you request there, i.e. NULLify set->tags[i] in __blk_mq_free_map_and_rqs(). Thanks, John
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c index db99f1246795..a0ecc6d88f84 100644 --- a/block/blk-mq-tag.c +++ b/block/blk-mq-tag.c @@ -607,8 +607,7 @@ int blk_mq_tag_update_depth(struct blk_mq_hw_ctx *hctx, if (!new) return -ENOMEM; - blk_mq_free_rqs(set, *tagsptr, hctx->queue_num); - blk_mq_free_rq_map(*tagsptr, set->flags); + blk_mq_free_map_and_rqs(set, *tagsptr, hctx->queue_num); *tagsptr = new; } else { /* diff --git a/block/blk-mq.c b/block/blk-mq.c index 46772773b9c4..464ea20b9bcb 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2878,15 +2878,15 @@ static bool __blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, return set->tags[hctx_idx]; } -static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set, - unsigned int hctx_idx) +void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, + struct blk_mq_tags *tags, + unsigned int hctx_idx) { unsigned int flags = set->flags; - if (set->tags && set->tags[hctx_idx]) { - blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx); - blk_mq_free_rq_map(set->tags[hctx_idx], flags); - set->tags[hctx_idx] = NULL; + if (tags) { + blk_mq_free_rqs(set, tags, hctx_idx); + blk_mq_free_rq_map(tags, flags); } } @@ -2967,8 +2967,10 @@ static void blk_mq_map_swqueue(struct request_queue *q) * fallback in case of a new remap fails * allocation */ - if (i && set->tags[i]) - blk_mq_free_map_and_requests(set, i); + if (i && set->tags[i]) { + blk_mq_free_map_and_rqs(set, set->tags[i], i); + set->tags[i] = NULL; + } hctx->tags = NULL; continue; @@ -3264,8 +3266,8 @@ static void blk_mq_realloc_hw_ctxs(struct blk_mq_tag_set *set, struct blk_mq_hw_ctx *hctx = hctxs[j]; if (hctx) { - if (hctx->tags) - blk_mq_free_map_and_requests(set, j); + blk_mq_free_map_and_rqs(set, set->tags[j], j); + set->tags[j] = NULL; blk_mq_exit_hctx(q, set, hctx, j); hctxs[j] = NULL; } @@ -3361,8 +3363,10 @@ static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) return 0; out_unwind: - while (--i >= 0) - blk_mq_free_map_and_requests(set, i); + while (--i >= 0) { + blk_mq_free_map_and_rqs(set, set->tags[i], i); + set->tags[i] = NULL; + } return -ENOMEM; } @@ -3557,8 +3561,10 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set) return 0; out_free_mq_rq_maps: - for (i = 0; i < set->nr_hw_queues; i++) - blk_mq_free_map_and_requests(set, i); + for (i = 0; i < set->nr_hw_queues; i++) { + blk_mq_free_map_and_rqs(set, set->tags[i], i); + set->tags[i] = NULL; + } out_free_mq_map: for (i = 0; i < set->nr_maps; i++) { kfree(set->map[i].mq_map); @@ -3590,8 +3596,10 @@ void blk_mq_free_tag_set(struct blk_mq_tag_set *set) { int i, j; - for (i = 0; i < set->nr_hw_queues; i++) - blk_mq_free_map_and_requests(set, i); + for (i = 0; i < set->nr_hw_queues; i++) { + blk_mq_free_map_and_rqs(set, set->tags[i], i); + set->tags[i] = NULL; + } if (blk_mq_is_sbitmap_shared(set->flags)) blk_mq_exit_shared_sbitmap(set); diff --git a/block/blk-mq.h b/block/blk-mq.h index 83585a344568..bcb0ca89d37a 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -57,7 +57,9 @@ void blk_mq_free_rqs(struct blk_mq_tag_set *set, struct blk_mq_tags *tags, void blk_mq_free_rq_map(struct blk_mq_tags *tags, unsigned int flags); struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set, unsigned int hctx_idx, unsigned int depth); - +void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set, + struct blk_mq_tags *tags, + unsigned int hctx_idx); /* * Internal helpers for request insertion into sw queues */