Re: wake_up_process implied memory barrier clarification

[Michal Hocko <mhocko@kernel.org>]

On Thu 27-08-15 14:43:34, Peter Zijlstra wrote:
> On Thu, Aug 27, 2015 at 02:27:27PM +0200, Michal Hocko wrote:
> > Hi,
> > I have just stumbled over the comment above wake_up_process which
> > claims:
> > "
> >  * It may be assumed that this function implies a write memory barrier before
> >  * changing the task state if and only if any tasks are woken up.
> > "
> > 
> > but try_to_wake_up does smp_mb__before_spinlock and did smp_wmb
> > since 04e2f1741d235 unconditionally. The comment was added when the
> > smp_wmb was in place already so I am wondering whether the comment is
> > wrong/misleading.
> > 
> > Could somebody clarify please?
> 
> Its true for wake_up(), since that bails early if the waitqueue list is
> empty.
> 
> I suspect there was no exception made for wake_up_process() to simplify
> the rules.

Thanks for the confirmation. Shouldn't we rather change the
documentation because this is clearly misleading and confusing.
--- 
>From b70d9a384cfd018e686c0aca06e830f564a34dd9 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 27 Aug 2015 15:10:55 +0200
Subject: [PATCH] sched: Clarigy wake_up_process memory barrier semantic

wake_up_process unlike other wake up primitives based on __wake_up
implies the write memory barrier unconditionally because it relies
on try_to_wake_up directly.

Clarify this in the function comment and memory-barriers.txt because the
current doc is quite misleading.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 Documentation/memory-barriers.txt | 3 +++
 kernel/sched/core.c               | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 13feb697271f..c4f180caf0ff 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -2031,6 +2031,9 @@ something up.  The barrier occurs before the task state is cleared, and so sits
 	    <general barrier>		  STORE current->state
 	LOAD event_indicated
 
+Please note that wake_up_process is an exception here because it implies
+the write memory barrier unconditionally.
+
 To repeat, this write memory barrier is present if and only if something
 is actually awakened.  To see this, consider the following sequence of
 events, where X and Y are both initially zero:
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 78b4bad10081..39583b76ad2c 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1967,8 +1967,7 @@ static void try_to_wake_up_local(struct task_struct *p)
  *
  * Return: 1 if the process was woken up, 0 if it was already running.
  *
- * It may be assumed that this function implies a write memory barrier before
- * changing the task state if and only if any tasks are woken up.
+ * It may be assumed that this function implies a write memory barrier.
  */
 int wake_up_process(struct task_struct *p)
 {
-- 
2.5.0


-- 
Michal Hocko
SUSE Labs