On 7/10/19 2:35 PM, Peter Zijlstra wrote:
> On Wed, Jul 10, 2019 at 02:23:23PM -0600, Jens Axboe wrote:
>> On 7/10/19 1:52 PM, Josef Bacik wrote:
>>> rq-qos sits in the io path so we want to take locks as sparingly as
>>> possible. To accomplish this we try not to take the waitqueue head lock
>>> unless we are sure we need to go to sleep, and we have an optimization
>>> to make sure that we don't starve out existing waiters. Since we check
>>> if there are existing waiters locklessly we need to be able to update
>>> our view of the waitqueue list after we've added ourselves to the
>>> waitqueue. Accomplish this by adding this helper to see if there are
>>> more than two waiters on the waitqueue.
>>>
>>> Suggested-by: Jens Axboe <axboe@xxxxxxxxx>
>>> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>>> ---
>>> include/linux/wait.h | 21 +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/include/linux/wait.h b/include/linux/wait.h
>>> index b6f77cf60dd7..89c41a7b3046 100644
>>> --- a/include/linux/wait.h
>>> +++ b/include/linux/wait.h
>>> @@ -126,6 +126,27 @@ static inline int waitqueue_active(struct wait_queue_head *wq_head)
>>> return !list_empty(&wq_head->head);
>>> }
>>>
>>> +/**
>>> + * wq_has_multiple_sleepers - check if there are multiple waiting prcesses
>>> + * @wq_head: wait queue head
>>> + *
>>> + * Returns true of wq_head has multiple waiting processes.
>>> + *
>>> + * Please refer to the comment for waitqueue_active.
>>> + */
>>> +static inline bool wq_has_multiple_sleepers(struct wait_queue_head *wq_head)
>>> +{
>>> + /*
>>> + * We need to be sure we are in sync with the
>>> + * add_wait_queue modifications to the wait queue.
>>> + *
>>> + * This memory barrier should be paired with one on the
>>> + * waiting side.
>>> + */
>>> + smp_mb();
>>> + return !list_is_singular(&wq_head->head);
>>> +}
>>> +
>>> /**
>>> * wq_has_sleeper - check if there are any waiting processes
>>> * @wq_head: wait queue head
>>
>> This (and 2/2) looks good to me, better than v1 for sure. Peter/Ingo,
>> are you OK with adding this new helper? For reference, this (and the
>> next patch) replace the alternative, which is an open-coding of
>> prepare_to_wait():
>>
>> https://lore.kernel.org/linux-block/20190710190514.86911-1-josef@xxxxxxxxxxxxxx/
>
> Yet another approach would be to have prepare_to_wait*() return this
> state, but I think this is ok.
We did discuss that case, but it seems somewhat random to have it
return that specific piece of info. But it'd work for this case.
> The smp_mb() is superfluous -- in your specific case -- since
> preprare_to_wait*() already does one through set_current_state().
>
> So you could do without it, I think.
But that's specific to this use case. Maybe it's the only one we'll
have, and then it's fine, but as a generic helper it seems safer to
include the same ordering protection as wq_has_sleeper().
--
Jens Axboe
