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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <dda57284-ea2e-f2ab-73c2-dacc0a796ebf@redhat.com>
Date:   Mon, 21 Mar 2022 10:57:13 -0400
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] locking/rwsem: Wake readers in a reader-owned rwsem
 if first waiter is a reader

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