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

[Ming Lei <ming.lei@redhat.com>]

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