Re: [PATCH 2/2] locking/rwsem: Wake readers in a reader-owned rwsem if first waiter is a reader

[Waiman Long <longman@redhat.com>]

On 3/21/22 08:52, Peter Zijlstra wrote:
> On Fri, Mar 18, 2022 at 12:16:09PM -0400, Waiman Long wrote:
>> In an analysis of a recent vmcore, a reader-owned rwsem was found with
>> 385 readers but no writer in the wait queue. That is kind of unusual
>> but it may be caused by some race conditions that we have not fully
>> understood yet. In such a case, all the readers in the wait queue should
>> join the other reader-owners and acquire the read lock.
>>
>> In rwsem_down_write_slowpath(), an incoming writer will try to wake
>> up the front readers under such circumstance. That is not the case for
>> rwsem_down_read_slowpath(), modify the code to do this. This includes the
>> original supported case where the wait queue is empty and the incoming
>> reader is going to wake up itself.
>>
>> With CONFIG_LOCK_EVENT_COUNTS enabled, the newly added rwsem_rlock_rwake
>> event counter had 13 hits right after the bootup of a 2-socket system. So
>> the condition that a reader-owned rwsem has readers at the front of
>> the wait queue does happen pretty frequently. This patch will help to
>> speed thing up in such cases.
> Urgh.. this so reads like a band-aid.

Yes, you may consider this a band-aid. On the other hand, the down_write 
slowpath is doing that and this patch modifies the down_read slowpath to 
do the same thing. I do agree that we need to do further investigation 
on what race conditions can cause this condition.


>
> Anyway; it appears to me the out_nolock case of down_read doesn't
> feature a wakeup, can we create a scenario with that?

I have consider adding extra wakeup in down_read out_nolock case. Unlike 
a writer that can block other readers later in the wait queue from 
getting the read lock, a reader earlier in the queue won't other readers 
later in the queue. So it is less useful from my point of view. For 
consistency, however, you are right that maybe we should do that too.


> Anyway, I think I much prefer you sitting down writing the rules for
> queueing and wakeup and then varifying them against the code rather than
> adding extra wakeups just cause.

Will rework the patch to apply the same rules for both readers and 
writers for consistency.

Cheers,
Longman