Re: [PATCH v2] blk-mq: Use request queue-wide tags for tagset-wide sbitmap

[John Garry]

> >   
> > +static int blk_mq_init_sched_shared_sbitmap(struct request_queue *queue)
> > +{
> > +	struct blk_mq_tag_set *set = queue->tag_set;
> > +	int ret;
> > +
> > +	/*
> > +	 * Set initial depth at max so that we don't need to reallocate for
> > +	 * updating nr_requests.
> > +	 */
> > +	ret = blk_mq_init_bitmaps(&queue->sched_bitmap_tags,
> > +				  &queue->sched_breserved_tags,
> > +				  set, MAX_SCHED_RQ, set->reserved_tags);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sbitmap_queue_resize(&queue->sched_bitmap_tags,
> > +			     queue->nr_requests - set->reserved_tags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void blk_mq_exit_sched_shared_sbitmap(struct request_queue *queue)
> > +{
> > +	sbitmap_queue_free(&queue->sched_bitmap_tags);
> > +	sbitmap_queue_free(&queue->sched_breserved_tags);
> > +}
> > +
> >   int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
> >   {
> >   	struct blk_mq_hw_ctx *hctx;
> > @@ -578,12 +598,25 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
> >   	queue_for_each_hw_ctx(q, hctx, i) {
> >   		ret = blk_mq_sched_alloc_tags(q, hctx, i);
> >   		if (ret)
> > -			goto err;
> > +			goto err_free_tags;
> > +	}
> > +
> > +	if (blk_mq_is_sbitmap_shared(q->tag_set->flags)) {
> > +		ret = blk_mq_init_sched_shared_sbitmap(q);
> > +		if (ret)
> > +			goto err_free_tags;
> > +
> > +		queue_for_each_hw_ctx(q, hctx, i) {
> > +			hctx->sched_tags->bitmap_tags =
> > +						&q->sched_bitmap_tags;
> > +			hctx->sched_tags->breserved_tags =
> > +						&q->sched_breserved_tags;
> > +		}
> The above assignment can be folded into blk_mq_init_sched_shared_sbitmap().

ok

> >   	}
> >   
> >   	ret = e->ops.init_sched(q, e);
> >   	if (ret)
> > -		goto err;
> > +		goto err_free_sbitmap;
> >   
> >   	blk_mq_debugfs_register_sched(q);
> >   
> > @@ -603,7 +636,10 @@ int blk_mq_init_sched(struct request_queue *q, struct elevator_type *e)
> >   
> >   	return 0;
> >   
> > -err:
> > +err_free_sbitmap:
> > +	if (blk_mq_is_sbitmap_shared(q->tag_set->flags))
> > +		blk_mq_exit_sched_shared_sbitmap(q);
> > +err_free_tags:
> >   	blk_mq_sched_free_requests(q);
> >   	blk_mq_sched_tags_teardown(q);
> >   	q->elevator = NULL;
> > @@ -641,5 +677,7 @@ void blk_mq_exit_sched(struct request_queue *q, struct elevator_queue *e)
> >   	if (e->type->ops.exit_sched)
> >   		e->type->ops.exit_sched(e);
> >   	blk_mq_sched_tags_teardown(q);
> > +	if (blk_mq_is_sbitmap_shared(q->tag_set->flags))
> > +		blk_mq_exit_sched_shared_sbitmap(q);
> The above two lines can be moved to blk_mq_sched_tags_teardown().

blk_mq_sched_tags_teardown() is also used in blk_mq_init_sched() to undo 
the blk_mq_sched_alloc_tags() calls; however, in that same function we 
call blk_mq_sched_alloc_tags() and blk_mq_init_sched_shared_sbitmap() 
separately, so can't combine into a single teardown function.

> >   	q->elevator = NULL;
> >   }
> > diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> > index 5b18ab915c65..aff037cfd8e7 100644
> > --- a/block/blk-mq-sched.h
> > +++ b/block/blk-mq-sched.h
> > @@ -5,6 +5,8 @@
> >   #include "blk-mq.h"
> >   #include "blk-mq-tag.h"
> >   
> > +#define MAX_SCHED_RQ (16 * BLKDEV_MAX_RQ)
> > +
> >   void blk_mq_sched_assign_ioc(struct request *rq);
> >   
> >   bool blk_mq_sched_try_merge(struct request_queue *q, struct bio *bio,
> > diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> > index 2a37731e8244..e3ab8631be22 100644
> > --- a/block/blk-mq-tag.c
> > +++ b/block/blk-mq-tag.c
> > @@ -13,6 +13,7 @@
> >   #include <linux/delay.h>
> >   #include "blk.h"
> >   #include "blk-mq.h"
> > +#include "blk-mq-sched.h"
> >   #include "blk-mq-tag.h"
> >   
> >   /*
> > @@ -466,19 +467,39 @@ static int blk_mq_init_bitmap_tags(struct blk_mq_tags *tags,
> >   	return -ENOMEM;
> >   }
> >   
> > -int blk_mq_init_shared_sbitmap(struct blk_mq_tag_set *set, unsigned int flags)
> > +int blk_mq_init_bitmaps(struct sbitmap_queue *bitmap_tags,
> > +			struct sbitmap_queue *breserved_tags,
> > +			struct blk_mq_tag_set *set,
> The 'set' parameter can be killed, meantime pass 'node' & 'alloc_policy',
> just like blk_mq_init_bitmap_tags()'s type, then blk_mq_init_bitmaps()
> can be re-used by blk_mq_init_bitmap_tags() for avoiding to duplicate
> bitmap allocation code.

I was thinking that we could consolidate here, so let me check this.

> > +			unsigned int queue_depth, unsigned int reserved)
> >   {
> > -	unsigned int depth = set->queue_depth - set->reserved_tags;
> > +	unsigned int depth = queue_depth - reserved;
> >   	int alloc_policy = BLK_MQ_FLAG_TO_ALLOC_POLICY(set->flags);
> >   	bool round_robin = alloc_policy == BLK_TAG_ALLOC_RR;
> > -	int i, node = set->numa_node;
> >   
> > -	if (bt_alloc(&set->__bitmap_tags, depth, round_robin, node))
> > +	if (bt_alloc(bitmap_tags, depth, round_robin, set->numa_node))
> >   		return -ENOMEM;
> > -	if (bt_alloc(&set->__breserved_tags, set->reserved_tags,
> > -		     round_robin, node))
> > +	if (bt_alloc(breserved_tags, set->reserved_tags,
> s/set->reserved_tags/reserved/

ok

Thanks!