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 <d...@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 <dbu...@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