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

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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