Re: [PATCH 4/3] rtmutex: Avoid barrier in rt_mutex_handle_deadlock
From: Davidlohr Bueso
Date: Mon Mar 21 2016 - 14:16:35 EST
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@xxxxxxxxxxxx>
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@xxxxxxx>
---
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