Re: [PATCH v3] locking/rwsem: reduce spinlock contention in wakeup after up_read/up_write

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Apr 24, 2015 at 01:54:29PM -0400, Waiman Long wrote:
> +static inline bool rwsem_has_active_writer(struct rw_semaphore *sem)
> +{
> +	return READ_ONCE(sem->owner) != NULL;
> +}

> +static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
> +{
> +	return osq_is_locked(&sem->osq);
> +}

> +	/*
> +	 * If a spinner is present, it is not necessary to do the wakeup.
> +	 * Try to do wakeup only if the trylock succeeds to minimize
> +	 * spinlock contention which may introduce too much delay in the
> +	 * unlock operation.
> +	 *
> +	 *    spinning writer		up_write/up_read caller
> +	 *    ---------------		-----------------------
> +	 * [S]   osq_unlock()		[L]   osq
> +	 *	 MB			      MB
> +	 * [RmW] rwsem_try_write_lock() [RmW] spin_trylock(wait_lock)
> +	 *
> +	 * Here, it is important to make sure that there won't be a missed
> +	 * wakeup while the rwsem is free and the only spinning writer goes
> +	 * to sleep without taking the rwsem. In case the spinning writer is
> +	 * just going to break out of the waiting loop, it will still do a
> +	 * trylock in rwsem_down_write_failed() before sleeping. IOW, if
> +	 * rwsem_has_spinner() is true, it will  guarantee at least one
> +	 * trylock attempt on the rwsem.
> +	 */
> +	if (!rwsem_has_spinner(sem)) {
> +		raw_spin_lock_irqsave(&sem->wait_lock, flags);
> +	} else {
> +		/*
> +		 * rwsem_has_spinner() is an atomic read while spin_trylock
> +		 * does not guarantee a full memory barrier. Insert a memory
> +		 * barrier here to make sure that wait_lock isn't read until
> +		 * after osq.
> +		 * Note: smp_rmb__after_atomic() should be used if available.
> +		 */
> +		smp_mb__after_atomic();

Sorry, that's wrong. the smp_mb__{before,after}_atomic() are for all
atomic (RmW) ops that do not return a value.

They end up as whatever barrier is required to make real atomic (RmW)
ops (LOCK on x86, LL/SC on risc etc) ordered. And all atomic (RmW) ops
that return a value are already guaranteed to imply full ordering
semantics.

Note, the (RmW) part is important here, atomic_{set,read}() are _NOT_
read-modify-write ops.

> +		if (!raw_spin_trylock_irqsave(&sem->wait_lock, flags))
> +			return sem;
> +	}
>