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

From: Hannes Reinecke <hare@suse.de>
To: John Garry <john.garry@huawei.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>, Christoph Hellwig <hch@lst.de>,
	James Bottomley <james.bottomley@hansenpartnership.com>,
	Ming Lei <ming.lei@redhat.com>,
	linux-scsi@vger.kernel.org, linux-block@vger.kernel.org
Subject: Re: [PATCH 04/11] blk-mq: Facilitate a shared sbitmap per tagset
Date: Tue, 3 Dec 2019 16:02:19 +0100	[thread overview]
Message-ID: <5beb7d51-500e-5bda-4e46-8414fd8b64ff@suse.de> (raw)
In-Reply-To: <ab7555b2-2e95-6fb1-2e44-fe3a323a24e4@huawei.com>

On 12/3/19 3:54 PM, John Garry wrote:
>>   @@ -483,8 +483,8 @@ static int hctx_tags_bitmap_show(void *data,
>> struct seq_file *m)
>>       res = mutex_lock_interruptible(&q->sysfs_lock);
>>       if (res)
>>           goto out;
>> -    if (hctx->tags)
>> -        sbitmap_bitmap_show(&hctx->tags->bitmap_tags.sb, m);
>> +    if (hctx->tags) /* We should just iterate the relevant bits for
>> this hctx FIXME */
> 
> Bart's solution to this problem seemed ok, if he doesn't mind us
> borrowing his idea:
> 
> https://lore.kernel.org/linux-block/5183ab13-0c81-95f0-95ba-40318569c6c6@huawei.com/T/#m24394fe70b1ea79a154dfd9620f5e553c3e7e7da
> 
> 
> See hctx_tags_bitmap_show().
> 
> It might be also reasonable to put that in another follow on patch, as
> there would be no enablers of the "shared" bitmap until later patches.
> 
Yeah, that was my plan, too.
But then I'd rather wait for feedback on the general approach here;
no point is wasting perfectly good bits if no-one's wanting them ...

[ .. ]
>> @@ -121,10 +121,10 @@ unsigned int blk_mq_get_tag(struct
>> blk_mq_alloc_data *data)
>>               WARN_ON_ONCE(1);
>>               return BLK_MQ_TAG_FAIL;
>>           }
>> -        bt = &tags->breserved_tags;
>> +        bt = tags->breserved_tags;
> 
> We could put all of this in an earlier patch (as you had in v4, modulo
> dynamic memory part), which would be easier to review and get accepted.
> 
Yeah, but I felt it a bit odd, just having pointers to an existing
structure element.
But yes, will be doing for the next round.

>>           tag_offset = 0;
>>       } else {
>> -        bt = &tags->bitmap_tags;
>> +        bt = tags->bitmap_tags;
>>           tag_offset = tags->nr_reserved_tags;
>>       }
> 
> 
> [...]
> 
>>       if (!set)
>> @@ -3160,6 +3179,7 @@ int blk_mq_update_nr_requests(struct
>> request_queue *q, unsigned int nr)
>>               ret = blk_mq_tag_update_depth(hctx, &hctx->tags, nr,
>>                               false);
>>           } else {
>> +            sched_tags = true;
>>               ret = blk_mq_tag_update_depth(hctx, &hctx->sched_tags,
>>                               nr, true);
>>           }
>> @@ -3169,8 +3189,43 @@ int blk_mq_update_nr_requests(struct
>> request_queue *q, unsigned int nr)
>>               q->elevator->type->ops.depth_updated(hctx);
>>       }
>>   -    if (!ret)
>> +    /*
>> +     * if ret is 0, all queues should have been updated to the same
>> depth
>> +     * if not, then maybe some have been updated - yuk, need to
>> handle this for shared sbitmap...
>> +     * if some are updated, we should probably roll back the change
>> altogether. FIXME
>> +     */
> 
> If you don't have a shared sched bitmap - which I didn't think we needed
> - then all we need is a simple sbitmap_queue_resize(&tagset->__bitmap_tags)
> 
> Otherwise it's horrible to resize shared sched bitmaps...
> 
Resizing shared sched bitmaps is done in patch 6/11.
General idea is to move the scheduler bitmap into the request queue
(well, actually the elevator), as this gives us a per-request_queue
bitmap. Which is actually what we want here, as the scheduler will need
to look at all requests, hence needing to access to the same bitmap.
And it also gives us an easy way of resizing the sched tag bitmap, as
then we can resize the bitmap on a per-queue basis, and leave the
underlying tagset bitmap untouched.

[ .. ]
>> diff --git a/block/blk-mq.h b/block/blk-mq.h
>> index 78d38b5f2793..4c1ea206d3f4 100644
>> --- a/block/blk-mq.h
>> +++ b/block/blk-mq.h
>> @@ -166,6 +166,11 @@ struct blk_mq_alloc_data {
>>       struct blk_mq_hw_ctx *hctx;
>>   };
>>   +static inline bool blk_mq_is_sbitmap_shared(struct blk_mq_tag_set
>> *tag_set)
>> +{
>> +    return !!(tag_set->flags & BLK_MQ_F_TAG_HCTX_SHARED);
> 
> Bart already gave some comments on this
> 
Ah. Missed that one. Will be including in the next round.

Thanks for the review!

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