Re: [PATCH] rwsem: fix missed wakeup due to reordering of load

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Jul 27, 2017 at 01:47:52AM +0530, Prateek Sood wrote:
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index 34e727f..21c111a 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -585,6 +585,40 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
>  	unsigned long flags;
>  	DEFINE_WAKE_Q(wake_q);
>  
> +        /*
> +         * If a spinner is present, there is a chance that the load of
> +         * rwsem_has_spinner() in rwsem_wake() can be reordered with
> +         * respect to decrement of rwsem count in __up_write() leading
> +         * to wakeup being missed.
> +         *
> +         * spinning writer                  up_write caller
> +         * ---------------                  -----------------------
> +         * [S] osq_unlock()                 [L] osq
> +         *  spin_lock(wait_lock)
> +         *  sem->count=0xFFFFFFFF00000001
> +         *            +0xFFFFFFFF00000000
> +         *  count=sem->count
> +         *  MB
> +         *                                   sem->count=0xFFFFFFFE00000001
> +         *                                             -0xFFFFFFFF00000001
> +         *                                   spin_trylock(wait_lock)
> +         *                                   return
> +         * rwsem_try_write_lock(count)
> +         * spin_unlock(wait_lock)
> +         * schedule()
> +         *
> +         * Reordering of atomic_long_sub_return_release() in __up_write()
> +         * and rwsem_has_spinner() in rwsem_wake() can cause missing of
> +         * wakeup in up_write() context. In spinning writer, sem->count
> +         * and local variable count is 0XFFFFFFFE00000001. It would result
> +         * in rwsem_try_write_lock() failing to acquire rwsem and spinning
> +         * writer going to sleep in rwsem_down_write_failed().
> +         *
> +         * The smp_rmb() here is to make sure that the spinner state is
> +         * consulted after sem->count is updated in up_write context.

I feel that comment can use help.. for example the RMB you add below is
not present at all.

> +         */
> +        smp_rmb();
> +
>  	/*
>  	 * If a spinner is present, it is not necessary to do the wakeup.
>  	 * Try to do wakeup only if the trylock succeeds to minimize

Your patch is whitespace damaged, all the indentation on the + lines is
with spaces. Please resend with \t.