Re: [PATCH 4/3] rtmutex: Avoid barrier in rt_mutex_handle_deadlock

[Davidlohr Bueso <dave@stgolabs.net>]

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