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

Paul E. McKenney
2009-04-28T13:01:06Z

On Tue, Apr 28, 2009 at 11:18:51AM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > But I would strongly suggest at least a note calling this out, preferably a
> > "don't do this" example.
>
> How about I add this to the bottom of the new section:
>
> [!] Note that the memory barriers implied by the sleeper and the waker do _not_
> order multiple stores before the wake-up with respect to loads of those stored
> values after the sleeper has called set_current_state(). For instance, if the
> sleeper does:
>
> set_current_state(TASK_INTERRUPTIBLE);
> if (event_indicated)
> break;
> __set_current_state(TASK_RUNNING);
> do_something(my_data);
>
> and the waker does:
>
> my_data = value;
> event_indicated = 1;
> wake_up(&event_wait_queue);
>
> there's no guarantee that the change to event_indicated will be perceived by
> the sleeper as coming after the change to my_data. In such a circumstance, the
> code on both sides must interpolate its own memory barriers between the
> separate data accesses. Thus the above sleeper ought to do:
>
> set_current_state(TASK_INTERRUPTIBLE);
> if (event_indicated) {
> smp_rmb();
> do_something(my_data);
> }
>
> and the waker should do:
>
> my_data = value;
> smp_wmb();
> event_indicated = 1;
> wake_up(&event_wait_queue);

Looks good to me!

Thanx, Paul