lkml.org 
[lkml]   [2016]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 4/3] rtmutex: Avoid barrier in rt_mutex_handle_deadlock
On Mon, 14 Mar 2016, Peter Zijlstra wrote:

>So you're right that it doesn't matter here, however for that very
>reason I would suggest not using __set_current_state() before schedule()
>unless there is a _really_ good reason, and then with an extensive
>comment to go with.

No problem.

>
>Otherwise people will manage to pick this as an example to copy and who
>all knows what kind of borkage will result from that.

Although I would expect 'people' to at least read the comments around the
code... and not blindly use rt-deadlock-related things :)

But yeah, lets drop this, I have no objection. While going through this,
I did find that we could do a little better documenting the actual helpers.

What do you think of the following?

Thanks,
Davidlohr


----------8<----------------------------------------------------------
From: Davidlohr Bueso <dave@stgolabs.net>
Subject: [PATCH -tip] sched: Cleanup comments for tsk->state helpers

While there is nothing wrong about the current comments, we could
easily improve them by the changes proposed in this patch:

- Remove duplicate text for CONFIG_DEBUG_ATOMIC_SLEEP.

- Update blocking example to consider spurious wakeups (for-loop).

- Point the reader to the infamous memory-barriers.txt doc, which
goes into plenty of detail in the 'SLEEP AND WAKE-UP FUNCTIONS'
section (the above also taken from there).

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
include/linux/sched.h | 51 ++++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c617ea12c6b7..3a3ec2503897 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -249,6 +249,31 @@ extern char ___assert_task_state[1 - 2*!!(
(task->flags & PF_FROZEN) == 0 && \
(task->state & TASK_NOLOAD) == 0)

+/*
+ * Helpers for modifying the state of either the current task, or a foreign
+ * task. Each of these calls come in both full barrier and weak flavors:
+ *
+ * Weak
+ * set_task_state() __set_task_state()
+ * set_current_state() __set_current_state()
+ *
+ * Where set_current_state() and set_task_state() includes a full smp barrier
+ * -after- the write of ->state is correctly serialized with the later test
+ * of whether to actually sleep:
+ *
+ * for (;;) {
+ * set_current_state(TASK_UNINTERRUPTIBLE);
+ * if (event_indicated)
+ * break;
+ * schedule();
+ * }
+ *
+ * This is commonly necessary for processes sleeping and waking through flag
+ * based events. If the caller does not need such serialization, then use
+ * weaker counterparts, which simply writes the state.
+ *
+ * Refer to Documentation/memory-barriers.txt
+ */
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP

#define __set_task_state(tsk, state_value) \
@@ -261,18 +286,6 @@ extern char ___assert_task_state[1 - 2*!!(
(tsk)->task_state_change = _THIS_IP_; \
smp_store_mb((tsk)->state, (state_value)); \
} while (0)
-
-/*
- * set_current_state() includes a barrier so that the write of current->state
- * is correctly serialised wrt the caller's subsequent test of whether to
- * actually sleep:
- *
- * set_current_state(TASK_UNINTERRUPTIBLE);
- * if (do_i_need_to_sleep())
- * schedule();
- *
- * If the caller does not need such serialisation then use __set_current_state()
- */
#define __set_current_state(state_value) \
do { \
current->task_state_change = _THIS_IP_; \
@@ -290,24 +303,12 @@ extern char ___assert_task_state[1 - 2*!!(
do { (tsk)->state = (state_value); } while (0)
#define set_task_state(tsk, state_value) \
smp_store_mb((tsk)->state, (state_value))
-
-/*
- * set_current_state() includes a barrier so that the write of current->state
- * is correctly serialised wrt the caller's subsequent test of whether to
- * actually sleep:
- *
- * set_current_state(TASK_UNINTERRUPTIBLE);
- * if (do_i_need_to_sleep())
- * schedule();
- *
- * If the caller does not need such serialisation then use __set_current_state()
- */
#define __set_current_state(state_value) \
do { current->state = (state_value); } while (0)
#define set_current_state(state_value) \
smp_store_mb(current->state, (state_value))

-#endif
+#endif /* CONFIG_DEBUG_ATOMIC_SLEEP */

/* Task command name length */
#define TASK_COMM_LEN 16
--
2.1.4

\
 
 \ /
  Last update: 2016-03-21 20:01    [W:0.152 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site