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

[John Garry <john.garry@huawei.com>]

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
>