linux-kernel - Re: [RFC PATCH] Fair low-latency rwlock v3

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:	Sun, 17 Aug 2008 09:17:02 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
cc:	"H. Peter Anvin" <hpa@...or.com>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>, Joe Perches <joe@...ches.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] Fair low-latency rwlock v3

On Sun, 17 Aug 2008, Mathieu Desnoyers wrote:
> +/*
> + * Uncontended fastpath.
> + */
> +static int fair_write_lock_irq_fast(struct fair_rwlock *rwlock)

So first off, you should call this "trylock", since it doesn't necessarily 
get the lock at all. It has nothing to do with fast.

Secondly:

> +	value = atomic_long_read(&rwlock->value);
> +	if (likely(!value)) {
> +		/* no other reader nor writer present, try to take the lock */
> +		local_bh_disable();
> +		local_irq_disable();
> +		if (likely(atomic_long_cmpxchg(&rwlock->value, value,

This is actually potentially very slow. 

Why? If the lock is uncontended, but is not in the current CPU's caches, 
the read -> rmw operation generates multiple cache coherency protocol 
events. First it gets the line in shared mode (for the read), and then 
later it turns it into exclusive mode.

So if it's likely that the value is zero (or even if it's just the only 
case we really care about), then you really should do the

	atomic_long_cmpxchg(&rwlock->value, 0, newvalue);

thing as the _first_ access to the lock.

Yeah, yeah, that means that you need to do the local_bh_disable etc first 
too, and undo it if it fails, but the failure path should be the unusual 
one. 

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