Re: [PATCH 4/8] blk-mq: Facilitate a shared sbitmap per tagset

John Garry
2019-11-26T11:50:22Z

On 26/11/2019 11:27, Hannes Reinecke wrote:
> On 11/26/19 12:05 PM, Ming Lei wrote:
>> On Tue, Nov 26, 2019 at 10:14:12AM +0100, Hannes Reinecke wrote:
> [ .. ]
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index ca22afd47b3d..f3589f42b96d 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -452,7 +452,7 @@ static void blk_mq_sched_free_tags(struct blk_mq_tag_set *set,
>>> {
>>> if (hctx->sched_tags) {
>>> blk_mq_free_rqs(set, hctx->sched_tags, hctx_idx);
>>> - blk_mq_free_rq_map(hctx->sched_tags);
>>> + blk_mq_free_rq_map(hctx->sched_tags, false);
>>> hctx->sched_tags = NULL;
>>> }
>>> }
>>> @@ -462,10 +462,14 @@ static int blk_mq_sched_alloc_tags(struct request_queue *q,
>>> unsigned int hctx_idx)
>>> {
>>> struct blk_mq_tag_set *set = q->tag_set;
>>> + int flags = set->flags;
>>> int ret;
>>>
>>> + /* Scheduler tags are never shared */
>>> + set->flags &= ~BLK_MQ_F_TAG_HCTX_SHARED;
>>> hctx->sched_tags = blk_mq_alloc_rq_map(set, hctx_idx, q->nr_requests,
>>> set->reserved_tags);
>>> + set->flags = flags;
>>
>> This way is very fragile, race is made against other uses of
>> blk_mq_is_sbitmap_shared().
>>
> We are allocating tags, I don't think we're even able to modify it at
> this point.
>
>> From performance viewpoint, all hctx belonging to this request queue should
>> share one scheduler tagset in case of BLK_MQ_F_TAG_HCTX_SHARED, cause
>> driver tag queue depth isn't changed.
>>
> Hmm. Now you get me confused.
> In an earlier mail you said:
>
>> This kind of sharing is wrong, sched tags should be request
>> queue wide instead of tagset wide, and each request queue has
>> its own & independent scheduler queue.
>
> as in v2 we _had_ shared scheduler tags, too.
> Did I misread your comment above?

As I understand, we just don't require shared scheduler tags. It's only
blk_mq_tag_set.tags which need to use the shared sbitmap.

>
>>> if (!hctx->sched_tags)
>>> return -ENOMEM;
>>>
> [ .. ]
>>> @@ -2456,7 +2459,8 @@ static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set,
>>> {
>>> 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]);
>>> + blk_mq_free_rq_map(set->tags[hctx_idx],
>>> + blk_mq_is_sbitmap_shared(set));
>>> set->tags[hctx_idx] = NULL;
>>> }
>>
>> Who will free the shared tags finally in case of blk_mq_is_sbitmap_shared()?
>>
> Hmm. Indeed, that bit is missing; will be adding it.
>
>>> }
> [ .. ]
>>> @@ -3168,8 +3186,17 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr)
>>> q->elevator->type->ops.depth_updated(hctx);
>>> }
>>>
>>> - if (!ret)
>>> + if (!ret) {
>>> + if (blk_mq_is_sbitmap_shared(set)) {
>>> + sbitmap_queue_resize(&set->shared_bitmap_tags, nr);
>>> + sbitmap_queue_resize(&set->shared_breserved_tags, nr);
>>> + }
>>
>> The above change is wrong in case of hctx->sched_tags.
>>
> Yes, we need to add a marker here if these are 'normal' or 'scheduler'
> tags. This will also help when allocating as then we won't need this
> flag twiddling you've complained about.

nit: To be proper, since this is tag management, we should move it to
blk-mq-tags.c

>
>>> q->nr_requests = nr;
>>> + }
>>> + /*
>>> + * if ret != 0, q->nr_requests would not be updated, yet the depth
>>> + * for some hctx may have changed - is that right?
>>> + */
>>>
>>> blk_mq_unquiesce_queue(q);
>>> blk_mq_unfreeze_queue(q);
> [ .. ]
>>> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
>>> index 147185394a25..670e9a949d32 100644
>>> --- a/include/linux/blk-mq.h
>>> +++ b/include/linux/blk-mq.h
>>> @@ -109,6 +109,12 @@ struct blk_mq_tag_set {
>>> unsigned int flags; /* BLK_MQ_F_* */
>>> void *driver_data;
>>>
>>> + struct sbitmap_queue shared_bitmap_tags;
>>> + struct sbitmap_queue shared_breserved_tags;
>>> +
>>> + struct sbitmap_queue sched_shared_bitmap_tags;
>>> + struct sbitmap_queue sched_shared_breserved_tags;
>>> +
>>
>> The above two fields aren't used in this patch.
>>
> Left-overs from merging. Will be removed.
>
> Cheers,
>
> Hannes
>