Re: [PATCH 2/4] rwsem: Add missing ACQUIRE to read_slowpath sleep loop

[Waiman Long]

On 7/18/19 9:49 AM, Peter Zijlstra wrote:
> While reviewing another read_slowpath patch, both Will and I noticed
> another missing ACQUIRE, namely:
>
>   X = 0;
>
>   CPU0			CPU1
>
>   rwsem_down_read()
>     for (;;) {
>       set_current_state(TASK_UNINTERRUPTIBLE);
>
>                         X = 1;
>                         rwsem_up_write();
>                           rwsem_mark_wake()
>                             atomic_long_add(adjustment, &sem->count);
>                             smp_store_release(&waiter->task, NULL);
>
>       if (!waiter.task)
>         break;
>
>       ...
>     }
>
>   r = X;
>
> Allows 'r == 0'.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Reported-by: Will Deacon <will@xxxxxxxxxx>
> Acked-by: Will Deacon <will@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  kernel/locking/rwsem.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -1069,8 +1069,10 @@ rwsem_down_read_slowpath(struct rw_semap
>  	/* wait to be given the lock */
>  	while (true) {
>  		set_current_state(state);
> -		if (!waiter.task)
> +		if (!smp_load_acquire(&waiter.task)) {
> +			/* Orders against rwsem_mark_wake()'s smp_store_release() */
>  			break;
> +		}
>  		if (signal_pending_state(state, current)) {
>  			raw_spin_lock_irq(&sem->wait_lock);
>  			if (waiter.task)
>
>
Acked-by: Waiman Long <longman@xxxxxxxxxx>

Thanks for fixing this old problem.

This problem does not apply to x86 and we do our testing mostly on x86.
That is why we seldom notice this kind of issue. Maybe we should be
doing more testing on ARM64 and PPC.

Cheers,
Longman