Messages in this thread Patch in this message | | | Date | Mon, 21 Mar 2016 11:16:22 -0700 | From | Davidlohr Bueso <> | Subject | Re: [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
| |