linux-kernel - Re: [PATCH 02/12] rwsem: use single atomic update for sem count when waking up readers.

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 12 May 2010 17:54:42 -0700
From:	Michel Lespinasse <walken@...gle.com>
To:	David Howells <dhowells@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mike Waychison <mikew@...gle.com>,
	Suleiman Souhlal <suleiman@...gle.com>,
	Ying Han <yinghan@...gle.com>
Subject: Re: [PATCH 02/12] rwsem: use single atomic update for sem count when 
	waking up readers.

On Wed, May 12, 2010 at 4:01 AM, David Howells <dhowells@...hat.com> wrote:
> Michel Lespinasse <walken@...gle.com> wrote:
>
>> - *   - there must be someone on the queue
>> + * - there must be someone on the queue
>
> Why did you change this comment?  This is still a guarantee up_xxxx() must
> make about the state of the rwsem.

What I meant is that we do rely on there being someone on the queue
even if not coming from up_xxxx().

>> + retry_readers:
>> +     oldcount = rwsem_atomic_update(woken, sem) - woken;
>> +     if (!downgrading && (oldcount & RWSEM_ACTIVE_MASK))
>
> The problem with doing this here is that you may just have wasted all the work
> you did working out what woken is going to be.  That may have been quite slow
> as the CPU may have had to get hold of a bunch of cachelines that weren't in
> its cache.  Furthermore, you are doing this under a spinlock, so you may have
> lost your right to wake anyone up, and you'll be blocking the CPU that will be
> allowed to perform the wakeup.
>
> Incrementing the count first nets you a guarantee that you have the right to
> wake things up.
>
> You may point out that if there's no contention, then what your revised code
> does doesn't slow anything down.  That's true, but on modern CPU's, neither
> does the old code as the exclusively held cache line will lurk in the CPU's
> cache until there's contention on it.

My reasoning was more that in all cases except downgrade_write()
(which gets an exemption anyway because we know nobody can grab the
write lock from it) we've already checked the active count and found
it to be zero, so there is not much point checking again at the start
of __rwsem_do_wake(). However you are correct that incrementing the
active count at that point could be important, just to guarantee that
nobody can grab a write lock while we count how many readers we want
to wake.

I'll make sure to address this.

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/