Re: [PATCH v4 11/13] blk-mq: Refactor and rename blk_mq_free_map_and_{requests->rqs}()
From: Ming Lei
Date: Mon Sep 27 2021 - 05:20:21 EST
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@xxxxxxxxxx>
> > > Signed-off-by: John Garry<john.garry@xxxxxxxxxx>
> > > Reviewed-by: Hannes Reinecke<hare@xxxxxxx>
> > > ---
> > > 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