Re: [PATCH] blk-wbt: get back the missed wakeup from __wbt_done

[van der Linden, Frank]

On 8/23/18 10:56 PM, jianchao.wang wrote:
>
> 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.
> "
>
Hmm, I am not sure about this one. Sure, it will reduce lock contention
for the waitq lock, but it also introduces more complexity.

It's expected that there will be more contention if the waitq lock is
held longer. That's the tradeoff for waking up more throttled tasks and
making progress faster. Is this added complexity worth the gains? My
first inclination would be to say no.

If lock contention on a wait queue is an issue, then either the wait
queue mechanism itself should be improved, or the code that uses the
wait queue should be fixed. Also, the contention is still a lot lower
than it used to be.

- Frank