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

Linus Torvalds
2008-08-17T16:18:17Z

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