Re: [PATCH RFC v7 04/12] blk-mq: Facilitate a shared sbitmap per tagset

[John Garry <john.garry@xxxxxxxxxx>]

On 11/06/2020 04:37, Ming Lei wrote:

Hi Ming,

Thanks for checking this.

> > bool bt_iter(struct sbitmap *bitmap, unsigned int bitnr, void *data)
> >   	 * We can hit rq == NULL here, because the tagging functions
> >   	 * test and set the bit before assigning ->rqs[].
> >   	 */
> > -	if (rq && rq->q == hctx->queue)
> > +	if (rq && rq->q == hctx->queue && rq->mq_hctx == hctx)
> >   		return iter_data->fn(hctx, rq, iter_data->data, reserved);
> >   	return true;
> >   }
> > @@ -466,6 +466,7 @@ static int blk_mq_init_bitmap_tags(struct blk_mq_tags *tags,
> >   		     round_robin, node))
> >   		goto free_bitmap_tags;
> >   
> > +	/* We later overwrite these in case of per-set shared sbitmap */
> >   	tags->bitmap_tags = &tags->__bitmap_tags;
> >   	tags->breserved_tags = &tags->__breserved_tags;
> You may skip to allocate anything for blk_mq_is_sbitmap_shared(), and
> similar change for blk_mq_free_tags().

I did try that, but it breaks scheduler tags allocation - this is common 
code. Maybe I can pass some flag, to avoid the allocation for case of 
shared sbitmap and !sched tags. Same for free path.

BTW, if you check patch 7/12, I mentioned that we could use this sbitmap 
for iterating to get the per-hctx bitmap, instead of allocating a temp 
sbitmap. Maybe it's better.

> >   
> > @@ -475,7 +476,32 @@ static int blk_mq_init_bitmap_tags(struct blk_mq_tags *tags,
> >   	return -ENOMEM;
> >   }
> >   
> > -struct blk_mq_tags *blk_mq_init_tags(unsigned int total_tags,
> > +bool blk_mq_init_shared_sbitmap(struct blk_mq_tag_set *tag_set)
> > +{
> > +	unsigned int depth = tag_set->queue_depth - tag_set->reserved_tags;
> > +	int alloc_policy = BLK_MQ_FLAG_TO_ALLOC_POLICY(tag_set->flags);
> > +	bool round_robin = alloc_policy == BLK_TAG_ALLOC_RR;
> > +	int node = tag_set->numa_node;
> > +
> > +	if (bt_alloc(&tag_set->__bitmap_tags, depth, round_robin, node))
> > +		return false;
> > +	if (bt_alloc(&tag_set->__breserved_tags, tag_set->reserved_tags,
> > +		     round_robin, node))
> > +		goto free_bitmap_tags;
> > +	return true;
> > +free_bitmap_tags:
> > +	sbitmap_queue_free(&tag_set->__bitmap_tags);
> > +	return false;
> > +}
> > +

[...]

> > index 90b645c3092c..77120dd4e4d5 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2229,7 +2229,7 @@ struct blk_mq_tags *blk_mq_alloc_rq_map(struct blk_mq_tag_set *set,
> >   	if (node == NUMA_NO_NODE)
> >   		node = set->numa_node;
> >   
> > -	tags = blk_mq_init_tags(nr_tags, reserved_tags, node,
> > +	tags = blk_mq_init_tags(set, nr_tags, reserved_tags, node,
> >   				BLK_MQ_FLAG_TO_ALLOC_POLICY(set->flags));
> >   	if (!tags)
> >   		return NULL;
> > @@ -3349,11 +3349,28 @@ int blk_mq_alloc_tag_set(struct blk_mq_tag_set *set)
> >   	if (ret)
> >   		goto out_free_mq_map;
> >   
> > +	if (blk_mq_is_sbitmap_shared(set)) {
> > +		if (!blk_mq_init_shared_sbitmap(set)) {
> > +			ret = -ENOMEM;
> > +			goto out_free_mq_rq_maps;
> > +		}
> > +
> > +		for (i = 0; i < set->nr_hw_queues; i++) {
> > +			struct blk_mq_tags *tags = set->tags[i];
> > +
> > +			tags->bitmap_tags = &set->__bitmap_tags;
> > +			tags->breserved_tags = &set->__breserved_tags;
> > +		}
> I am wondering why you don't put ->[bitmap|breserved]_tags initialization into
> blk_mq_init_shared_sbitmap().

I suppose I could.

Thanks,
John