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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 12 May 2010 12:01:10 +0100
From:	David Howells <dhowells@...hat.com>
To:	Michel Lespinasse <walken@...gle.com>
Cc:	dhowells@...hat.com,
	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.

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.

> +	waiter = list_entry(sem->wait_list.next, struct rwsem_waiter, list);
> +	if (!(waiter->flags & RWSEM_WAITING_FOR_WRITE))
> +		goto readers_only;
> +
>  	if (downgrading)
> -		goto dont_wake_writers;
> +		/* Caller's lock is still active, so we can't possibly
> +		 * succeed waking writers.
> +		 */
> +		goto out;

It's a nice idea to do it this way round - it puts the wake-up-reader path
first and puts the downgrader on the slower path.

> -	/* if we came through an up_xxxx() call, we only only wake someone up
> +	/* There's a writer at the front of the queue - try to grant it the
> +	 * write lock. However, we only only wake someone up
>  	 * if we can transition the active part of the count from 0 -> 1
>  	 */

Two spaces after a full stop, please, and can you please adjust the comment so
that it fills out to 80 chars.  E.g:

	/* There's a writer at the front of the queue - try to grant it the
	 * write lock.  However, we only only wake someone up if we can
	 * transition the active part of the count from 0 -> 1
	 */

instead of:

	/* There's a writer at the front of the queue - try to grant it the
	 * write lock. However, we only only wake someone up
	 * if we can transition the active part of the count from 0 -> 1
	 */

> + 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.

David
--
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/