linux-kernel - Re: [PATCH RFC 3/5] blk-mq: Facilitate a shared tags per tagset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b85f172-695c-4757-3794-455b8d55e015@acm.org>
Date:   Fri, 15 Nov 2019 09:57:55 -0800
From:   Bart Van Assche <bvanassche@....org>
To:     John Garry <john.garry@...wei.com>, Hannes Reinecke <hare@...e.de>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>
Cc:     "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "ming.lei@...hat.com" <ming.lei@...hat.com>,
        "hare@...e.com" <hare@...e.com>,
        "chenxiang (M)" <chenxiang66@...ilicon.com>
Subject: Re: [PATCH RFC 3/5] blk-mq: Facilitate a shared tags per tagset

On 11/15/19 2:24 AM, John Garry wrote:
> Bart Van Assche wrote:
> > How about sharing tag sets across hardware
> > queues, e.g. like in the (totally untested) patch below?
> 
> So this is similar in principle what Ming Lei came up with here:
> https://lore.kernel.org/linux-block/20190531022801.10003-1-ming.lei@redhat.com/ 
> 
> However your implementation looks neater, which is good.
> 
> My concern with this approach is that we can't differentiate which tags 
> are allocated for which hctx, and sometimes we need to know that.
> 
> An example here was blk_mq_queue_tag_busy_iter(), which iterates the 
> bits for each hctx. This would just be broken by that change, unless we 
> record which bits are associated with each hctx.

I disagree. In bt_iter() I added " && rq->mq_hctx == hctx" such that 
blk_mq_queue_tag_busy_iter() only calls the callback function for 
matching (hctx, rq) pairs.

> Another example was __blk_mq_tag_idle(), which looks problematic.

Please elaborate.

> For debugfs, when we examine 
> /sys/kernel/debug/block/.../hctxX/tags_bitmap, wouldn't that be the tags 
> for all hctx (hctx0)?
> 
> For debugging reasons, I would say we want to know which tags are 
> allocated for a specific hctx, as this is tightly related to the 
> requests for that hctx.

That is an open issue in the patch I posted and something that needs to 
be addressed. One way to address this is to change the 
sbitmap_bitmap_show() calls into calls to a function that only shows 
those bits for which rq->mq_hctx == hctx.

>> @@ -341,8 +341,11 @@ void blk_mq_tagset_busy_iter(struct 
>> blk_mq_tag_set *tagset,
>>       int i;
>>
>>       for (i = 0; i < tagset->nr_hw_queues; i++) {
>> -        if (tagset->tags && tagset->tags[i])
>> +        if (tagset->tags && tagset->tags[i]) {
>>               blk_mq_all_tag_busy_iter(tagset->tags[i], fn, priv);
> 
> As I mentioned earlier, wouldn't this iterate over all tags for all 
> hctx's, when we just want the tags for hctx[i]?
> 
> Thanks,
> John
> 
> [Not trimming reply for future reference]
> 
>> +            if (tagset->share_tags)
>> +                break;
>> +        }
>>       }
>>   }
>>   EXPORT_SYMBOL(blk_mq_tagset_busy_iter);

Since blk_mq_tagset_busy_iter() loops over all hardware queues all what 
is changed is the order in which requests are examined. I am not aware 
of any block driver that calls blk_mq_tagset_busy_iter() and that 
depends on the order of the requests passed to the callback function.

Thanks,

Bart.