linux-kernel - Re: [PATCH] blk-wbt: get back the missed wakeup from __wbt

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]

Date:   Fri, 24 Aug 2018 13:55:58 +0800
From:   "jianchao.wang" <jianchao.w.wang@...cle.com>
To:     Jens Axboe <axboe@...nel.dk>, Anchal Agarwal <anchalag@...n.com>,
        "van der Linden, Frank" <fllinden@...zon.com>
Cc:     "mlinux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] blk-wbt: get back the missed wakeup from __wbt_done



On 08/24/2018 07:14 AM, Jens Axboe wrote:
> On 8/23/18 5:03 PM, Jens Axboe wrote:
>>> Hi Jens, This patch looks much cleaner for sure as Frank pointed out
>>> too. Basically this looks similar to wake_up_nr only making sure that
>>> those woken up requests won't get reordered. This does solves the
>>> thundering herd issue. However, I tested the patch against my
>>> application and lock contention numbers rose to around 10 times from
>>> what I had from your last 3 patches.  Again this did add to drop in
>>> of total files read by 0.12% and rate at which they were read by
>>> 0.02% but this is not a very significant drop. Is lock contention
>>> worth the tradeoff?  I also added missing
>>> __set_current_state(TASK_RUNNING) to the patch for testing.
>>
>> Can you try this variant? I don't think we need a
>> __set_current_state() after io_schedule(), should be fine as-is.
>>
>> I'm not surprised this will raise contention a bit, since we're now
>> waking N tasks potentially, if N can queue. With the initial change,
>> we'd always just wake one.  That is arguably incorrect. You say it's
>> 10 times higher contention, how does that compare to before your
>> patch?
>>
>> Is it possible to run something that looks like your workload?
> 
> Additionally, is the contention you are seeing the wait queue, or the
> atomic counter? When you say lock contention, I'm inclined to think it's
> the rqw->wait.lock.
> 

I guess the increased lock contend is due to:
when the wake up is ongoing with wait head lock is held, there is still waiter
on wait queue, and __wbt_wait will go to wait and try to require the wait head lock.
This is necessary to keep the order on the rqw->wait queue.

The attachment does following thing to try to avoid the scenario above.
"
Introduce wait queue rqw->delayed. Try to lock rqw->wait.lock firstly, if fails, add
the waiter on rqw->delayed. __wbt_done will pick the waiters on rqw->delayed up and
queue them on the tail of rqw->wait before it do wake up operation.
"

Thanks
Jianchao

View attachment "0001-blk-wbt-get-back-the-missed-wakeup-from-__wbt_done.patch" of type "text/x-patch" (7147 bytes)