Re: [PATCH] It may not be assumed that wake_up(), finish_wait() and co. imply a memory barrier

Oleg Nesterov
2009-04-24T18:35:51Z

On 04/24, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Suppose that "event_indicated = 1" leaks into try_to_wake_up() after we
> > read p->state.
>
> In that case, it's entirely possible that the smp_wmb() in try_to_wake_up()
> should actually be an smp_mb(), but that on whichever arch patch:
>
> commit 04e2f1741d235ba599037734878d72e57cb302b5
> Author: Linus Torvalds <torvalds@woody.linux-foundation.org>
> Date: Sat Feb 23 18:05:03 2008 -0800
> Subject: Add memory barrier semantics to wake_up() & co
>
> was tested on, it made no difference.

I think that, from the correctness pov, this patch does not depend on arch.
>From the changelog:

However, adding a smp_wmb() to before the spinlock should now order the
writing of CONDITION wrt the lock itself, which in turn is ordered wrt
the accesses within the spinlock (which includes the reading of the old
state).

IOW, spin_lock() itself is STORE, and has the acquire semantics. This means
that "wmb() + spin_lock()" must correctly serialize "STOREs bfore" and
"STOREs/LOADs after". At least this is my understanding.

But does it really "equal" to mb() on all arches ? I don't know, but I guess
it is not possible to derive from documentation. So, at least in theory,
try_to_wake_up() doesn't provide rmb() afaics, and

LOAD_1;
try_to_wake_up();
LOAD_2;

this can be re-ordered.

But I know nothing about how mb/lock _actually_ play with hardware, even on
x86.

Oleg.