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) <[email protected]>
> Reported-by: Will Deacon <[email protected]>
> Acked-by: Will Deacon <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> 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 <[email protected]>
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
