Re: [PATCH] Document that wake_up(), complete() and co. imply a full memory barrier

[Ingo Molnar <mingo@elte.hu>]

* David Howells <dhowells@redhat.com> wrote:

> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > > That's an interesting question.  Should wake_up() imply a barrier of any
> > > sort, I wonder.  Well, __wake_up() does impose a barrier as it uses a
> > > spinlock, but I wonder if that's sufficient.
> > 
> > wake_up() does imply the barrier. Note the smp_wmb() in try_to_wake_up().
> > And in fact this wmb() implies mb(), because spin_lock() itself is STORE,
> > and the futher LOADs can't leak up before spin_lock().
> > 
> > But afaics, this doesn't matter? prepare_to_wait() sets 
> > task->state under wait_queue_head_t->lock and wake_up() takes 
> > this look too, so we can't miss the event.
> > 
> > Or I completely misunderstood the issue...
> 
> The problem is not what wake_up() and co. do, it's what you are 
> allowed to assume that they do.
> 
> However, I think you're right, and that we can assume they imply a 
> full memory barrier.  To this end, I've attached a patch to 
> document this.
> 
> David
> ---
> From: David Howells <dhowells@redhat.com>
> Subject: [PATCH] Document that wake_up(), complete() and co. imply a full memory barrier
> 
> Add to the memory barriers document to note that wake_up(), complete() and
> co. all imply a full memory barrier.

No. They dont generally imply a full memory barrier versus any 
arbitrary prior (or following) memory access.

try_to_wake_up() has an smp_wmb() so it is a write memory barrier 
(but not necessarily a read memory barrier). Otherwise there are 
spinlocks there but spinlocks are not explicit 'full memory 
barriers'.

Also, there's a sub-detail wrt. the wake_up() variants in that they 
have a fastpath for the !q case - then they dont have any atomics at 
all - they just return straight away.

Another sub-detail: wakeups using a special wakeup handler might not 
even call try_to_wake_up() - so in their case not even a write 
barrier can be assumed.

So your patch is misleading in a number of areas here.

	Ingo