[PATCH memory-model 10/14] sched: Use smp_mb() in wake_woken_function()
From: Andrea Parri
wake_woken_function() synchronizes with wait_woken() as follows:
[wait_woken] [wake_woken_function]
entry->flags &= ~wq_flag_woken;condition = true;
smp_mb(); smp_wmb();
if (condition) wq_entry->flags |= wq_flag_woken;
break;
This commit replaces the above smp_wmb() with an smp_mb() in order to
guarantee that either wait_woken() sees the wait condition being true
or the store to wq_entry->flags in woken_wake_function() follows the
store in wait_woken() in the coherence order (so that the former can
eventually be observed by wait_woken()).
The commit also fixes a comment associated to set_current_state() in
wait_woken(): the comment pairs the barrier in set_current_state() to
the above smp_wmb(), while the actual pairing involves the barrier in
set_current_state() and the barrier executed by the try_to_wake_up()
in wake_woken_function().
Signed-off-by: Andrea Parri
Cc: Ingo Molnar
Acked-by: Peter Zijlstra (Intel)
Signed-off-by: Paul E. McKenney
---
kernel/sched/wait.c | 47 -
1 file changed, 21 insertions(+), 26 deletions(-)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 928be527477e..a7a2aaa3026a 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -392,35 +392,36 @@ static inline bool is_kthread_should_stop(void)
* if (condition)
* break;
*
- * p->state = mode;condition = true;
- * smp_mb(); // A smp_wmb(); // C
- * if (!wq_entry->flags & WQ_FLAG_WOKEN) wq_entry->flags |=
WQ_FLAG_WOKEN;
- * schedule() try_to_wake_up();
- * p->state = TASK_RUNNING;~~
- * wq_entry->flags &= ~WQ_FLAG_WOKEN; condition = true;
- * smp_mb() // B smp_wmb(); // C
- * wq_entry->flags |=
WQ_FLAG_WOKEN;
- * }
- * remove_wait_queue(_head, );
+ * // in wait_woken() // in woken_wake_function()
*
+ * p->state = mode;wq_entry->flags |=
WQ_FLAG_WOKEN;
+ * smp_mb(); // A try_to_wake_up():
+ * if (!(wq_entry->flags & WQ_FLAG_WOKEN))
+ * schedule() if (p->state & mode)
+ * p->state = TASK_RUNNING; p->state =
TASK_RUNNING;
+ * wq_entry->flags &= ~WQ_FLAG_WOKEN; ~~
+ * smp_mb(); // B condition = true;
+ * } smp_mb(); // C
+ * remove_wait_queue(_head, ); wq_entry->flags |=
WQ_FLAG_WOKEN;
*/
long wait_woken(struct wait_queue_entry *wq_entry, unsigned mode, long timeout)
{
- set_current_state(mode); /* A */
/*
-* The above implies an smp_mb(), which matches with the smp_wmb() from
-* woken_wake_function() such that if we observe WQ_FLAG_WOKEN we must
-* also observe all state before the wakeup.
+* The below executes an smp_mb(), which matches with the full barrier
+* executed by the try_to_wake_up() in woken_wake_function() such that
+* either we see the store to wq_entry->flags in woken_wake_function()
+* or woken_wake_function() sees our store to current->state.
*/
+ set_current_state(mode); /* A */
if (!(wq_entry->flags & WQ_FLAG_WOKEN) && !is_kthread_should_stop())
timeout = schedule_timeout(timeout);
__set_current_state(TASK_RUNNING);
/*
-* The below implies an smp_mb(), it too pairs with the smp_wmb() from
-* woken_wake_function() such that we must either observe the wait
-* condition being true _OR_ WQ_FLAG_WOKEN such that we will not miss
-* an event.
+* The below executes an smp_mb(), which matches with the smp_mb() (C)
+* in woken_wake_function() such that either we see the wait condition
+* being true or the store to wq_entry->flags in woken_wake_function()
+* follows ours in the coherence order.
*/
smp_store_mb(wq_entry->flags, wq_entry->flags & ~WQ_FLAG_WOKEN); /* B */
@@ -430,14 +431,8 @@ EXPORT_SYMBOL(wait_woken);
int woken_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int
sync, void *key)
{
- /*
-* Although this function is called under waitqueue lock, LOCK
-* doesn't imply write barrier and the users expects write
-* barrier semantics on wakeup functions. The following
-* smp_wmb() is equivalent to smp_wmb() in try_to_wake_up()
-* and is paired with smp_store_mb() in wait_woken().
-*/
- smp_wmb(); /* C */
+ /* Pairs with the smp_store_mb() in wait_woken(). */
+ smp_mb(); /* C */
wq_entry->flags |= WQ_FLAG_WOKEN;
[PATCH memory-model 10/14] sched: Use smp_mb() in wake_woken_function()
From: Andrea Parri
wake_woken_function() synchronizes with wait_woken() as follows:
[wait_woken] [wake_woken_function]
entry->flags &= ~wq_flag_woken;condition = true;
smp_mb(); smp_wmb();
if (condition) wq_entry->flags |= wq_flag_woken;
break;
This commit replaces the above smp_wmb() with an smp_mb() in order to
guarantee that either wait_woken() sees the wait condition being true
or the store to wq_entry->flags in woken_wake_function() follows the
store in wait_woken() in the coherence order (so that the former can
eventually be observed by wait_woken()).
The commit also fixes a comment associated to set_current_state() in
wait_woken(): the comment pairs the barrier in set_current_state() to
the above smp_wmb(), while the actual pairing involves the barrier in
set_current_state() and the barrier executed by the try_to_wake_up()
in wake_woken_function().
Signed-off-by: Andrea Parri
Cc: Ingo Molnar
Acked-by: Peter Zijlstra (Intel)
Signed-off-by: Paul E. McKenney
---
kernel/sched/wait.c | 47 -
1 file changed, 21 insertions(+), 26 deletions(-)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 928be527477e..a7a2aaa3026a 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -392,35 +392,36 @@ static inline bool is_kthread_should_stop(void)
* if (condition)
* break;
*
- * p->state = mode;condition = true;
- * smp_mb(); // A smp_wmb(); // C
- * if (!wq_entry->flags & WQ_FLAG_WOKEN) wq_entry->flags |=
WQ_FLAG_WOKEN;
- * schedule() try_to_wake_up();
- * p->state = TASK_RUNNING;~~
- * wq_entry->flags &= ~WQ_FLAG_WOKEN; condition = true;
- * smp_mb() // B smp_wmb(); // C
- * wq_entry->flags |=
WQ_FLAG_WOKEN;
- * }
- * remove_wait_queue(_head, );
+ * // in wait_woken() // in woken_wake_function()
*
+ * p->state = mode;wq_entry->flags |=
WQ_FLAG_WOKEN;
+ * smp_mb(); // A try_to_wake_up():
+ * if (!(wq_entry->flags & WQ_FLAG_WOKEN))
+ * schedule() if (p->state & mode)
+ * p->state = TASK_RUNNING; p->state =
TASK_RUNNING;
+ * wq_entry->flags &= ~WQ_FLAG_WOKEN; ~~
+ * smp_mb(); // B condition = true;
+ * } smp_mb(); // C
+ * remove_wait_queue(_head, ); wq_entry->flags |=
WQ_FLAG_WOKEN;
*/
long wait_woken(struct wait_queue_entry *wq_entry, unsigned mode, long timeout)
{
- set_current_state(mode); /* A */
/*
-* The above implies an smp_mb(), which matches with the smp_wmb() from
-* woken_wake_function() such that if we observe WQ_FLAG_WOKEN we must
-* also observe all state before the wakeup.
+* The below executes an smp_mb(), which matches with the full barrier
+* executed by the try_to_wake_up() in woken_wake_function() such that
+* either we see the store to wq_entry->flags in woken_wake_function()
+* or woken_wake_function() sees our store to current->state.
*/
+ set_current_state(mode); /* A */
if (!(wq_entry->flags & WQ_FLAG_WOKEN) && !is_kthread_should_stop())
timeout = schedule_timeout(timeout);
__set_current_state(TASK_RUNNING);
/*
-* The below implies an smp_mb(), it too pairs with the smp_wmb() from
-* woken_wake_function() such that we must either observe the wait
-* condition being true _OR_ WQ_FLAG_WOKEN such that we will not miss
-* an event.
+* The below executes an smp_mb(), which matches with the smp_mb() (C)
+* in woken_wake_function() such that either we see the wait condition
+* being true or the store to wq_entry->flags in woken_wake_function()
+* follows ours in the coherence order.
*/
smp_store_mb(wq_entry->flags, wq_entry->flags & ~WQ_FLAG_WOKEN); /* B */
@@ -430,14 +431,8 @@ EXPORT_SYMBOL(wait_woken);
int woken_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int
sync, void *key)
{
- /*
-* Although this function is called under waitqueue lock, LOCK
-* doesn't imply write barrier and the users expects write
-* barrier semantics on wakeup functions. The following
-* smp_wmb() is equivalent to smp_wmb() in try_to_wake_up()
-* and is paired with smp_store_mb() in wait_woken().
-*/
- smp_wmb(); /* C */
+ /* Pairs with the smp_store_mb() in wait_woken(). */
+ smp_mb(); /* C */
wq_entry->flags |= WQ_FLAG_WOKEN;
