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

[Hannes Reinecke <hare@suse.de>]

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?

>>  	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.

>>  		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
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer