Re: [PATCH v4 11/13] blk-mq: Refactor and rename blk_mq_free_map_and_{requests->rqs}()

[John Garry <john.garry@xxxxxxxxxx>]

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?

Thanks,
John