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

John Garry
2020-06-11T10:11:05Z

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