Message-ID: <20150910021612.GC18494@fixme-laptop.cn.ibm.com>
Date:	Thu, 10 Sep 2015 10:16:12 +0800
From:	Boqun Feng <boqun.feng@...il.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	Jonathan Corbet <corbet@....net>,
	Michal Hocko <mhocko@...nel.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	David Howells <dhowells@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] Documentation: Remove misleading examples of the
 barriers in wake_*()

On Wed, Sep 09, 2015 at 12:28:22PM -0700, Paul E. McKenney wrote:
> On Tue, Sep 08, 2015 at 09:14:01AM +0800, Boqun Feng wrote:
> > Two examples for barriers in wake_up() and co. in memory-barriers.txt
> > are misleading, along with their explanations:
> > 
> > 1.	The example which wanted to explain the write barrier in
> > 	wake_up() and co. [spotted by Oleg Nesterov <oleg@...hat.com>]
> > 
> > 2.	The example which wanted to explain that the write barriers in
> > 	wake_up() and co. only exist iff a wakeup actually occurs.
> > 
> > For example #1, according to Oleg Nesterov:
> > 
> > >
> > >       The barrier occurs before the task state is cleared
> > >
> > > is not actually right. This is misleading. What is really important is that
> > > we have a barrier before we _read_ the task state. And again, again, the
> > > fact that we actually have the write barrier is just the implementation
> > > detail.
> > >
> > 
> > And the example #2 is actually an example which could explain that the
> > barriers in wait_event() and co. only exist iff a sleep actually occurs.
> > 
> > Further more, these barriers are only used for the correctness of
> > sleeping and waking up, i.e. they exist only to guarantee the ordering
> > of memory accesses to the task states and the global variables
> > indicating an event. Users can't rely on them for other things, so
> > memory-barriers.txt had better to call this out and remove the
> > misleading examples.
> > 
> > This patch removes the misleading examples along with their
> > explanations, calls it out that those implied barriers are only for
> > sleep and wakeup related variables and adds a new example to explain the
> > implied barrier in wake_up() and co.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> 
> At this point, I would favor replacing that entire section with a short
> paragraph describing what guarantees are provided, perhaps with an example
> showing what added barriers/locks/whatever are required.  My feeling is
> that we should avoid saying too much about the internals of wait_event()
> and wake_up().

Good idea!

However, I think a little more words help understand. So I keep the
original first paragraph and also add a paragraph for NOTE which, I
think, may be a little redundant although ;-)

How about the following new whole section?


SLEEP AND WAKE-UP FUNCTIONS
---------------------------

Sleeping and waking on an event flagged in global data can be viewed as an
interaction between two pieces of data: the task state of the task waiting for
the event and the global data used to indicate the event.  To make sure that
these appear to happen in the right order, the primitives to begin the process
of going to sleep, and the primitives to initiate a wake up imply certain
barriers.

The memory ordering requirement here can be expressed by two STORE-LOAD
barriers(barriers which can guarantee a STORE perceding it can never be
reordered after a LOAD following it). One STORE-LOAD barrier is needed on the
sleeper/wakee side, before reading a variable used to indicate the event and
after setting the state of the current task. Another STORE-LOAD barrier is
needed on the waker side, before reading the state of the wakee task and after
setting a variable used to indicate the event. These two barriers can pair with
each other to avoid race conditions between sleepers/wakees and wakers:

	sleepr/wakee on CPU 1		waker on CPU 2
	========================	========================
	{ wakee->state = TASK_RUNNING, event_indicated = 0 }
	STORE current->state=TASK_INTERRUPTIBLE
	<STORE-LOAD barrier>
	c = LOAD event_indicated	
					STORE event_indicated=1
					<STORE-LOAD barrier>
					s = LOAD wakee->state	

	assert(!(c==0 && s == TASK_RUNNING));

A STORE-LOAD barrier is implied after setting task state by wait-related functions:

	prepare_to_wait();
	prepare_to_wait_exclusive();
	prepare_to_wait_event();

A STORE-LOAD barrier is implied before reading task state by wake-related functions:

	complete();
	wake_up();
	wake_up_all();
	wake_up_bit();
	wake_up_interruptible();
	wake_up_interruptible_all();
	wake_up_interruptible_nr();
	wake_up_interruptible_poll();
	wake_up_interruptible_sync();
	wake_up_interruptible_sync_poll();
	wake_up_locked();
	wake_up_locked_poll();
	wake_up_nr();
	wake_up_poll();
	wake_up_process();

Make sure an appropriate wake-related function is called after setting a global
data used to indicate a event.

[!] Note that these implied barriers are only for the correctness of sleep and
wake-up. So don't rely on these barriers for things that are neither the task
states nor the global variables indicating the events.


git log --stat for this is:

 1 file changed, 29 insertions(+), 108 deletions(-)

, which I think it's better, thanks to your advice ;-)


I will rewrite the commit message and send a new patch if this looks to
you.

Regards,
Boqun

> 
> Or am I missing something?
> 
> 							Thanx, Paul

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists