Skip to contentSkip to conversation

sched: Rework task state printing

PATCH In review Peter Zijlstra 4 participants 14 messages Sep 25, 2017 – Oct 10, 2017

Subsystems (4)
Tracing Scheduler Proc filesystem Tracing OS noise / latency tracers
lore
Share & tools
lore

Peter Zijlstra

author maintainer
#0 lore raw

cover sched: Rework task state printing

With wider usage of TASK_IDLE came a few user visible annoyances. Markus reported that TASK_IDLE is reported as TASK_UNINTERRUPTIBLE and some tools like htop mark those highly visible, which is not warranted for TASK_IDLE.

This patch set reworks the task-state printing and introduces explicit handling for TASK_IDLE and TASK_PARKED.

author maintainer
#1 lore raw

PATCH 1/8 sched: Consistent task-state printing

Currently get_task_state() and task_state_to_char() report different states, create a number of common helpers and unify the reported state space.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 fs/proc/array.c       |   15 ++-------------
 include/linux/sched.h |   26 +++++++++++++++++++-------
 2 files changed, 21 insertions(+), 20 deletions(-)
fs/proc/array.c +2 -13
@@ -129,19 +129,8 @@ static const char * const task_state_arr
 
 static inline const char *get_task_state(struct task_struct *tsk)
 {
-	unsigned int state = (tsk->state | tsk->exit_state) & TASK_REPORT;
-
-	/*
-	 * Parked tasks do not run; they sit in __kthread_parkme().
-	 * Without this check, we would report them as running, which is
-	 * clearly wrong, so we report them as sleeping instead.
-	 */
-	if (tsk->state == TASK_PARKED)
-		state = TASK_INTERRUPTIBLE;
-
-	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array)-1);
-
-	return task_state_array[fls(state)];
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array) - 1);
+	return task_state_array[__get_task_state(tsk)];
 }
 
 static inline int get_task_umask(struct task_struct *tsk)
include/linux/sched.h +19 -7
@@ -1244,17 +1244,29 @@ static inline pid_t task_pgrp_nr(struct
 	return task_pgrp_nr_ns(tsk, &init_pid_ns);
 }
 
-static inline char task_state_to_char(struct task_struct *task)
+static inline unsigned int __get_task_state(struct task_struct *tsk)
 {
-	const char stat_nam[] = TASK_STATE_TO_CHAR_STR;
-	unsigned long state = task->state;
+	unsigned int tsk_state = READ_ONCE(tsk->state);
+	unsigned int state = (tsk_state | tsk->exit_state) & TASK_REPORT;
 
-	state = state ? __ffs(state) + 1 : 0;
+	if (tsk_state == TASK_PARKED)
+		state = TASK_INTERRUPTIBLE;
 
-	/* Make sure the string lines up properly with the number of task states: */
-	BUILD_BUG_ON(sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1);
+	return fls(state);
+}
+
+static inline char __task_state_to_char(unsigned int state)
+{
+	static const char state_char[] = "RSDTtXZ";
+
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != sizeof(state_char) - 2);
 
-	return state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?';
+	return state_char[state];
+}
+
+static inline char task_state_to_char(struct task_struct *tsk)
+{
+	return __task_state_to_char(__get_task_state(tsk));
 }
 
 /**
maintainer
#2 lore raw

Re: [PATCH 1/8] sched: Consistent task-state printing

Quoted 5 lines · On Mon, 25 Sep 2017 14:07:48 +0200 Peter Zijlstra <[email protected]> wrote:
include/linux/sched.h +4 -0
+static inline char __task_state_to_char(unsigned int state)
+{
+	static const char state_char[] = "RSDTtXZ";
+

include/linux/sched.h static inline pid_t task_pgrp_nr(struct · line 1270

Quoted 10 lines
include/linux/sched.h +7 -1
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != sizeof(state_char) - 2);
 
-	return state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?';
+	return state_char[state];
+}
+
+static inline char task_state_to_char(struct task_struct *tsk)
+{
+	return __task_state_to_char(__get_task_state(tsk));
 }

So far I'm fine with the patch set, but I hate the non descriptive "__" prefix of __task_state_to_char(). Can we make this a bit more descriptive, because every time I see it in other patches, I go back to this patch to see if we are using the right function.

What about something like:

task_state_to_state_char(unsigned int state);

task_to_state_char(struct task_struct *tsk);

?

This way, the "__" wont keep making me think we used the wrong function.

-- Steve

author maintainer
#3 lore raw

Re: [PATCH 1/8] sched: Consistent task-state printing

Quoted 24 lines · Steven Rostedt
Quoted 5 lines · On Mon, 25 Sep 2017 14:07:48 +0200 Peter Zijlstra <[email protected]> wrote:
(quoted diff) +4 -0
+static inline char __task_state_to_char(unsigned int state)
+{
+	static const char state_char[] = "RSDTtXZ";
+

Review

Quoted 10 lines
(quoted diff) +7 -1
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != sizeof(state_char) - 2);
 
-	return state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?';
+	return state_char[state];
+}
+
+static inline char task_state_to_char(struct task_struct *tsk)
+{
+	return __task_state_to_char(__get_task_state(tsk));
 }

So far I'm fine with the patch set, but I hate the non descriptive "__" prefix of __task_state_to_char(). Can we make this a bit more descriptive, because every time I see it in other patches, I go back to this patch to see if we are using the right function.

Quoted 7 lines

What about something like:

task_state_to_state_char(unsigned int state);

task_to_state_char(struct task_struct *tsk);

?

Hurmph.. naming.

How about something like so?

fs/proc/array.c +1 -1
@@ -137,7 +137,7 @@ static const char * const task_state_arr
 static inline const char *get_task_state(struct task_struct *tsk)
 {
 	BUILD_BUG_ON(1 + ilog2(TASK_REPORT_MAX) != ARRAY_SIZE(task_state_array));
-	return task_state_array[__get_task_state(tsk)];
+	return task_state_array[task_state_index(tsk)];
 }
 
 static inline int get_task_umask(struct task_struct *tsk)
include/linux/sched.h +3 -3
@@ -1245,7 +1245,7 @@ static inline pid_t task_pgrp_nr(struct
 #define TASK_REPORT_IDLE	(TASK_REPORT + 1)
 #define TASK_REPORT_MAX		(TASK_REPORT_IDLE << 1)
 
-static inline unsigned int __get_task_state(struct task_struct *tsk)
+static inline unsigned int task_state_index(struct task_struct *tsk)
 {
 	unsigned int tsk_state = READ_ONCE(tsk->state);
 	unsigned int state = (tsk_state | tsk->exit_state) & TASK_REPORT;

@@ -1258,7 +1258,7 @@ static inline unsigned int __get_task_st
 	return fls(state);
 }
 
-static inline char __task_state_to_char(unsigned int state)
+static inline char task_index_to_char(unsigned int state)
 {
 	static const char state_char[] = "RSDTtXZPI";
 

@@ -1269,7 +1269,7 @@ static inline char __task_state_to_char(
 
 static inline char task_state_to_char(struct task_struct *tsk)
 {
-	return __task_state_to_char(__get_task_state(tsk));
+	return task_index_to_char(task_state_index(tsk));
 }
 
 /**
include/trace/events/sched.h +1 -1
@@ -117,7 +117,7 @@ static inline long __trace_sched_switch_
 	if (preempt)
 		return TASK_STATE_MAX;
 
-	return __get_task_state(p);
+	return task_state_index(p);
 }
 #endif /* CREATE_TRACE_POINTS */
kernel/trace/trace_output.c +6 -6
@@ -921,8 +921,8 @@ static enum print_line_t trace_ctxwake_p
 
 	trace_assign_type(field, iter->ent);
 
-	T = __task_state_to_char(field->next_state);
-	S = __task_state_to_char(field->prev_state);
+	T = task_index_to_char(field->next_state);
+	S = task_index_to_char(field->prev_state);
 	trace_find_cmdline(field->next_pid, comm);
 	trace_seq_printf(&iter->seq,
 			 " %5d:%3d:%c %s [%03d] %5d:%3d:%c %s\n",

@@ -957,8 +957,8 @@ static int trace_ctxwake_raw(struct trac
 	trace_assign_type(field, iter->ent);
 
 	if (!S)
-		S = __task_state_to_char(field->prev_state);
-	T = __task_state_to_char(field->next_state);
+		S = task_index_to_char(field->prev_state);
+	T = task_index_to_char(field->next_state);
 	trace_seq_printf(&iter->seq, "%d %d %c %d %d %d %c\n",
 			 field->prev_pid,
 			 field->prev_prio,

@@ -993,8 +993,8 @@ static int trace_ctxwake_hex(struct trac
 	trace_assign_type(field, iter->ent);
 
 	if (!S)
-		S = __task_state_to_char(field->prev_state);
-	T = __task_state_to_char(field->next_state);
+		S = task_index_to_char(field->prev_state);
+	T = task_index_to_char(field->next_state);
 
 	SEQ_PUT_HEX_FIELD(s, field->prev_pid);
 	SEQ_PUT_HEX_FIELD(s, field->prev_prio);
kernel/trace/trace_sched_wakeup.c +4 -4
@@ -397,10 +397,10 @@ tracing_sched_switch_trace(struct trace_
 	entry	= ring_buffer_event_data(event);
 	entry->prev_pid			= prev->pid;
 	entry->prev_prio		= prev->prio;
-	entry->prev_state		= __get_task_state(prev);
+	entry->prev_state		= task_state_index(prev);
 	entry->next_pid			= next->pid;
 	entry->next_prio		= next->prio;
-	entry->next_state		= __get_task_state(next);
+	entry->next_state		= task_state_index(next);
 	entry->next_cpu	= task_cpu(next);
 
 	if (!call_filter_check_discard(call, entry, buffer, event))

@@ -425,10 +425,10 @@ tracing_sched_wakeup_trace(struct trace_
 	entry	= ring_buffer_event_data(event);
 	entry->prev_pid			= curr->pid;
 	entry->prev_prio		= curr->prio;
-	entry->prev_state		= __get_task_state(curr);
+	entry->prev_state		= task_state_index(curr);
 	entry->next_pid			= wakee->pid;
 	entry->next_prio		= wakee->prio;
-	entry->next_state		= __get_task_state(wakee);
+	entry->next_state		= task_state_index(wakee);
 	entry->next_cpu			= task_cpu(wakee);
 
 	if (!call_filter_check_discard(call, entry, buffer, event))
maintainer Acked-by
#4 lore raw

Re: [PATCH 1/8] sched: Consistent task-state printing

Quoted 29 lines · On Fri, 29 Sep 2017 13:50:16 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 28 lines · Steven Rostedt
Quoted 5 lines · On Mon, 25 Sep 2017 14:07:48 +0200 Peter Zijlstra <[email protected]> wrote:
(quoted diff) +4 -0
+static inline char __task_state_to_char(unsigned int state)
+{
+	static const char state_char[] = "RSDTtXZ";
+

Review

Quoted 10 lines
(quoted diff) +7 -1
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != sizeof(state_char) - 2);
 
-	return state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?';
+	return state_char[state];
+}
+
+static inline char task_state_to_char(struct task_struct *tsk)
+{
+	return __task_state_to_char(__get_task_state(tsk));
 }

So far I'm fine with the patch set, but I hate the non descriptive "__" prefix of __task_state_to_char(). Can we make this a bit more descriptive, because every time I see it in other patches, I go back to this patch to see if we are using the right function.

What about something like:

task_state_to_state_char(unsigned int state);

Quoted 8 lines
Quoted 3 lines

task_to_state_char(struct task_struct *tsk);

?

Hurmph.. naming.

How about something like so?

I'm not picky, I just hate beginning underscores, and wish we can start removing all functions that use them.

Acked-by Steven Rostedt (VMware) <[email protected]>

-- Steve

merge bot
#5 lore raw

[tip:sched/core] sched/debug: Rename task-state printing helpers

In reply to Peter Zijlstra
sched/debug: Rename task-state printing helpers
Commit-ID:  1d48b080bcce0a5e7d7aa2dbcdb35deefc188c3f
Gitweb:     https://git.kernel.org/tip/1d48b080bcce0a5e7d7aa2dbcdb35deefc188c3f
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 29 Sep 2017 13:50:16 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 10 Oct 2017 11:43:29 +0200

sched/debug: Rename task-state printing helpers

Steve requested better names for the new task-state helper functions.

So introduce the concept of task-state index for the printing and rename __get_task_state() to task_state_index() and __task_state_to_char() to task_index_to_char().

Requested-by Steven Rostedt <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Acked-by Steven Rostedt <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 fs/proc/array.c                   |  2 +-
 include/linux/sched.h             |  6 +++---
 include/trace/events/sched.h      |  2 +-
 kernel/trace/trace_output.c       | 12 ++++++------
 kernel/trace/trace_sched_wakeup.c |  8 ++++----
 5 files changed, 15 insertions(+), 15 deletions(-)
fs/proc/array.c +1 -1
index 77a8eac..1b5406c 100644
@@ -137,7 +137,7 @@ static const char * const task_state_array[] = {
 static inline const char *get_task_state(struct task_struct *tsk)
 {
 	BUILD_BUG_ON(1 + ilog2(TASK_REPORT_MAX) != ARRAY_SIZE(task_state_array));
-	return task_state_array[__get_task_state(tsk)];
+	return task_state_array[task_state_index(tsk)];
 }
 
 static inline int get_task_umask(struct task_struct *tsk)
include/linux/sched.h +3 -3
index bdd6ad6..33a01f4 100644
@@ -1248,7 +1248,7 @@ static inline pid_t task_pgrp_nr(struct task_struct *tsk)
 #define TASK_REPORT_IDLE	(TASK_REPORT + 1)
 #define TASK_REPORT_MAX		(TASK_REPORT_IDLE << 1)
 
-static inline unsigned int __get_task_state(struct task_struct *tsk)
+static inline unsigned int task_state_index(struct task_struct *tsk)
 {
 	unsigned int tsk_state = READ_ONCE(tsk->state);
 	unsigned int state = (tsk_state | tsk->exit_state) & TASK_REPORT;

@@ -1261,7 +1261,7 @@ static inline unsigned int __get_task_state(struct task_struct *tsk)
 	return fls(state);
 }
 
-static inline char __task_state_to_char(unsigned int state)
+static inline char task_index_to_char(unsigned int state)
 {
 	static const char state_char[] = "RSDTtXZPI";
 

@@ -1272,7 +1272,7 @@ static inline char __task_state_to_char(unsigned int state)
 
 static inline char task_state_to_char(struct task_struct *tsk)
 {
-	return __task_state_to_char(__get_task_state(tsk));
+	return task_index_to_char(task_state_index(tsk));
 }
 
 /**
include/trace/events/sched.h +1 -1
index 3c8b7f6..fab74a1 100644
@@ -117,7 +117,7 @@ static inline long __trace_sched_switch_state(bool preempt, struct task_struct *
 	if (preempt)
 		return TASK_STATE_MAX;
 
-	return __get_task_state(p);
+	return task_state_index(p);
 }
 #endif /* CREATE_TRACE_POINTS */
kernel/trace/trace_output.c +6 -6
index c738e76..90db994 100644
@@ -921,8 +921,8 @@ static enum print_line_t trace_ctxwake_print(struct trace_iterator *iter,
 
 	trace_assign_type(field, iter->ent);
 
-	T = __task_state_to_char(field->next_state);
-	S = __task_state_to_char(field->prev_state);
+	T = task_index_to_char(field->next_state);
+	S = task_index_to_char(field->prev_state);
 	trace_find_cmdline(field->next_pid, comm);
 	trace_seq_printf(&iter->seq,
 			 " %5d:%3d:%c %s [%03d] %5d:%3d:%c %s\n",

@@ -957,8 +957,8 @@ static int trace_ctxwake_raw(struct trace_iterator *iter, char S)
 	trace_assign_type(field, iter->ent);
 
 	if (!S)
-		S = __task_state_to_char(field->prev_state);
-	T = __task_state_to_char(field->next_state);
+		S = task_index_to_char(field->prev_state);
+	T = task_index_to_char(field->next_state);
 	trace_seq_printf(&iter->seq, "%d %d %c %d %d %d %c\n",
 			 field->prev_pid,
 			 field->prev_prio,

@@ -993,8 +993,8 @@ static int trace_ctxwake_hex(struct trace_iterator *iter, char S)
 	trace_assign_type(field, iter->ent);
 
 	if (!S)
-		S = __task_state_to_char(field->prev_state);
-	T = __task_state_to_char(field->next_state);
+		S = task_index_to_char(field->prev_state);
+	T = task_index_to_char(field->next_state);
 
 	SEQ_PUT_HEX_FIELD(s, field->prev_pid);
 	SEQ_PUT_HEX_FIELD(s, field->prev_prio);
kernel/trace/trace_sched_wakeup.c +4 -4
index 0c33197..500f370 100644
@@ -397,10 +397,10 @@ tracing_sched_switch_trace(struct trace_array *tr,
 	entry	= ring_buffer_event_data(event);
 	entry->prev_pid			= prev->pid;
 	entry->prev_prio		= prev->prio;
-	entry->prev_state		= __get_task_state(prev);
+	entry->prev_state		= task_state_index(prev);
 	entry->next_pid			= next->pid;
 	entry->next_prio		= next->prio;
-	entry->next_state		= __get_task_state(next);
+	entry->next_state		= task_state_index(next);
 	entry->next_cpu	= task_cpu(next);
 
 	if (!call_filter_check_discard(call, entry, buffer, event))

@@ -425,10 +425,10 @@ tracing_sched_wakeup_trace(struct trace_array *tr,
 	entry	= ring_buffer_event_data(event);
 	entry->prev_pid			= curr->pid;
 	entry->prev_prio		= curr->prio;
-	entry->prev_state		= __get_task_state(curr);
+	entry->prev_state		= task_state_index(curr);
 	entry->next_pid			= wakee->pid;
 	entry->next_prio		= wakee->prio;
-	entry->next_state		= __get_task_state(wakee);
+	entry->next_state		= task_state_index(wakee);
 	entry->next_cpu			= task_cpu(wakee);
 
 	if (!call_filter_check_discard(call, entry, buffer, event))
author maintainer
#6 lore raw

PATCH 2/8 sched: Convert TASK_state to hex

In reply to Peter Zijlstra

Bit patterns are easier in hex.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/sched.h |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
include/linux/sched.h +14 -14
@@ -65,23 +65,23 @@ struct task_group;
  */
 
 /* Used in tsk->state: */
-#define TASK_RUNNING			0
-#define TASK_INTERRUPTIBLE		1
-#define TASK_UNINTERRUPTIBLE		2
-#define __TASK_STOPPED			4
-#define __TASK_TRACED			8
+#define TASK_RUNNING			0x0000
+#define TASK_INTERRUPTIBLE		0x0001
+#define TASK_UNINTERRUPTIBLE		0x0002
+#define __TASK_STOPPED			0x0004
+#define __TASK_TRACED			0x0008
 /* Used in tsk->exit_state: */
-#define EXIT_DEAD			16
-#define EXIT_ZOMBIE			32
+#define EXIT_DEAD			0x0010
+#define EXIT_ZOMBIE			0x0020
 #define EXIT_TRACE			(EXIT_ZOMBIE | EXIT_DEAD)
 /* Used in tsk->state again: */
-#define TASK_DEAD			64
-#define TASK_WAKEKILL			128
-#define TASK_WAKING			256
-#define TASK_PARKED			512
-#define TASK_NOLOAD			1024
-#define TASK_NEW			2048
-#define TASK_STATE_MAX			4096
+#define TASK_DEAD			0x0040
+#define TASK_WAKEKILL			0x0080
+#define TASK_WAKING			0x0100
+#define TASK_PARKED			0x0200
+#define TASK_NOLOAD			0x0400
+#define TASK_NEW			0x0800
+#define TASK_STATE_MAX			0x1000
 
 #define TASK_STATE_TO_CHAR_STR		"RSDTtXZxKWPNn"
author maintainer
#7 lore raw

PATCH 3/8 sched/debug: Remove unused variable

In reply to Peter Zijlstra
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/sched/debug.c |    2 --
 1 file changed, 2 deletions(-)
kernel/sched/debug.c +0 -2
@@ -461,8 +461,6 @@ static char *task_group_path(struct task
 }
 #endif
 
-static const char stat_nam[] = TASK_STATE_TO_CHAR_STR;
-
 static void
 print_task(struct seq_file *m, struct rq *rq, struct task_struct *p)
 {
author maintainer
#8 lore raw

PATCH 4/8 sched/trace: Fix trace_sched_switch task-state printing

In reply to Peter Zijlstra

Convert trace_sched_switch to use the common task-state helpers and fix the "X" and "Z" order, possibly they ended up in the wrong order because TASK_REPORT has them in the wrong order too.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/sched.h        |    2 +-
 include/trace/events/sched.h |   18 +++++++++++-------
 2 files changed, 12 insertions(+), 8 deletions(-)
include/linux/sched.h +1 -1
@@ -99,7 +99,7 @@ struct task_group;
 /* get_task_state(): */
 #define TASK_REPORT			(TASK_RUNNING | TASK_INTERRUPTIBLE | \
 					 TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-					 __TASK_TRACED | EXIT_ZOMBIE | EXIT_DEAD)
+					 __TASK_TRACED | EXIT_DEAD | EXIT_ZOMBIE)
 
 #define task_is_traced(task)		((task->state & __TASK_TRACED) != 0)
include/trace/events/sched.h +11 -7
@@ -114,7 +114,10 @@ static inline long __trace_sched_switch_
 	 * Preemption ignores task state, therefore preempted tasks are always
 	 * RUNNING (we will not have dequeued if state != RUNNING).
 	 */
-	return preempt ? TASK_RUNNING | TASK_STATE_MAX : p->state;
+	if (preempt)
+		return TASK_STATE_MAX;
+
+	return __get_task_state(p);
 }
 #endif /* CREATE_TRACE_POINTS */
 

@@ -152,12 +155,13 @@ TRACE_EVENT(sched_switch,
 
 	TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d",
 		__entry->prev_comm, __entry->prev_pid, __entry->prev_prio,
-		__entry->prev_state & (TASK_STATE_MAX-1) ?
-		  __print_flags(__entry->prev_state & (TASK_STATE_MAX-1), "|",
-				{ 1, "S"} , { 2, "D" }, { 4, "T" }, { 8, "t" },
-				{ 16, "Z" }, { 32, "X" }, { 64, "x" },
-				{ 128, "K" }, { 256, "W" }, { 512, "P" },
-				{ 1024, "N" }) : "R",
+
+		(__entry->prev_state & TASK_REPORT) ?
+		  __print_flags(__entry->prev_state & TASK_REPORT, "|",
+				{ 0x01, "S" }, { 0x02, "D" }, { 0x04, "T" },
+				{ 0x08, "t" }, { 0x10, "X" }, { 0x20, "Z" }) :
+		  "R",
+
 		__entry->prev_state & TASK_STATE_MAX ? "+" : "",
 		__entry->next_comm, __entry->next_pid, __entry->next_prio)
 );
author maintainer
#9 lore raw

PATCH 5/8 sched/trace: Use common task-state helpers

In reply to Peter Zijlstra

Remove yet another task-state char instance.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 include/linux/sched.h             |    2 --
 kernel/trace/trace_output.c       |   21 ++++++---------------
 kernel/trace/trace_sched_wakeup.c |   12 ++++++------
 3 files changed, 12 insertions(+), 23 deletions(-)
include/linux/sched.h +0 -2
@@ -86,8 +86,6 @@ struct task_group;
 #define TASK_NEW			0x0800
 #define TASK_STATE_MAX			0x1000
 
-#define TASK_STATE_TO_CHAR_STR		"RSDTtXZxKWPNn"
-
 /* Convenience macros for the sake of set_current_state: */
 #define TASK_KILLABLE			(TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
 #define TASK_STOPPED			(TASK_WAKEKILL | __TASK_STOPPED)
kernel/trace/trace_output.c +6 -15
@@ -656,15 +656,6 @@ int trace_print_lat_context(struct trace
 	return !trace_seq_has_overflowed(s);
 }
 
-static const char state_to_char[] = TASK_STATE_TO_CHAR_STR;
-
-static int task_state_char(unsigned long state)
-{
-	int bit = state ? __ffs(state) + 1 : 0;
-
-	return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
-}
-
 /**
  * ftrace_find_event - find a registered event
  * @type: the type of event to look for

@@ -930,8 +921,8 @@ static enum print_line_t trace_ctxwake_p
 
 	trace_assign_type(field, iter->ent);
 
-	T = task_state_char(field->next_state);
-	S = task_state_char(field->prev_state);
+	T = __task_state_to_char(field->next_state);
+	S = __task_state_to_char(field->prev_state);
 	trace_find_cmdline(field->next_pid, comm);
 	trace_seq_printf(&iter->seq,
 			 " %5d:%3d:%c %s [%03d] %5d:%3d:%c %s\n",

@@ -966,8 +957,8 @@ static int trace_ctxwake_raw(struct trac
 	trace_assign_type(field, iter->ent);
 
 	if (!S)
-		S = task_state_char(field->prev_state);
-	T = task_state_char(field->next_state);
+		S = __task_state_to_char(field->prev_state);
+	T = __task_state_to_char(field->next_state);
 	trace_seq_printf(&iter->seq, "%d %d %c %d %d %d %c\n",
 			 field->prev_pid,
 			 field->prev_prio,

@@ -1002,8 +993,8 @@ static int trace_ctxwake_hex(struct trac
 	trace_assign_type(field, iter->ent);
 
 	if (!S)
-		S = task_state_char(field->prev_state);
-	T = task_state_char(field->next_state);
+		S = __task_state_to_char(field->prev_state);
+	T = __task_state_to_char(field->next_state);
 
 	SEQ_PUT_HEX_FIELD(s, field->prev_pid);
 	SEQ_PUT_HEX_FIELD(s, field->prev_prio);
kernel/trace/trace_sched_wakeup.c +6 -6
@@ -380,7 +380,7 @@ probe_wakeup_migrate_task(void *ignore,
 }
 
 static void
-tracing_sched_switch_trace(struct trace_array *tr,
+tracing_sched_switch_trace(bool preempt, struct trace_array *tr,
 			   struct task_struct *prev,
 			   struct task_struct *next,
 			   unsigned long flags, int pc)

@@ -397,10 +397,10 @@ tracing_sched_switch_trace(struct trace_
 	entry	= ring_buffer_event_data(event);
 	entry->prev_pid			= prev->pid;
 	entry->prev_prio		= prev->prio;
-	entry->prev_state		= prev->state;
+	entry->prev_state		= __trace_sched_switch_state(preempt, prev);
 	entry->next_pid			= next->pid;
 	entry->next_prio		= next->prio;
-	entry->next_state		= next->state;
+	entry->next_state		= __get_task_state(next);
 	entry->next_cpu	= task_cpu(next);
 
 	if (!call_filter_check_discard(call, entry, buffer, event))

@@ -425,10 +425,10 @@ tracing_sched_wakeup_trace(struct trace_
 	entry	= ring_buffer_event_data(event);
 	entry->prev_pid			= curr->pid;
 	entry->prev_prio		= curr->prio;
-	entry->prev_state		= curr->state;
+	entry->prev_state		= __get_task_state(curr);
 	entry->next_pid			= wakee->pid;
 	entry->next_prio		= wakee->prio;
-	entry->next_state		= wakee->state;
+	entry->next_state		= __get_task_state(wakee);
 	entry->next_cpu			= task_cpu(wakee);
 
 	if (!call_filter_check_discard(call, entry, buffer, event))

@@ -482,7 +482,7 @@ probe_wakeup_sched_switch(void *ignore,
 	data = per_cpu_ptr(wakeup_trace->trace_buffer.data, wakeup_cpu);
 
 	__trace_function(wakeup_trace, CALLER_ADDR0, CALLER_ADDR1, flags, pc);
-	tracing_sched_switch_trace(wakeup_trace, prev, next, flags, pc);
+	tracing_sched_switch_trace(preempt, wakeup_trace, prev, next, flags, pc);
 
 	T0 = data->preempt_timestamp;
 	T1 = ftrace_now(cpu);
author maintainer
#10 lore raw

PATCH 6/8 sched: Add explicit TASK_IDLE printing

In reply to Peter Zijlstra

Markus reported that kthreads that idle using TASK_IDLE instead of TASK_INTERRUPTIBLE are reported in as TASK_UNINTERRUPTIBLE and things like htop mark those red.

This is undesirable, so add an explicit state for TASK_IDLE.

Reported-by Markus Trippelsdorf <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 fs/proc/array.c              |   21 +++++++++++++--------
 include/linux/sched.h        |   12 ++++++++++--
 include/trace/events/sched.h |    7 ++++---
 3 files changed, 27 insertions(+), 13 deletions(-)
fs/proc/array.c +13 -8
@@ -118,18 +118,23 @@ static inline void task_name(struct seq_
  * simple bit tests.
  */
 static const char * const task_state_array[] = {
-	"R (running)",		/*   0 */
-	"S (sleeping)",		/*   1 */
-	"D (disk sleep)",	/*   2 */
-	"T (stopped)",		/*   4 */
-	"t (tracing stop)",	/*   8 */
-	"X (dead)",		/*  16 */
-	"Z (zombie)",		/*  32 */
+
+	/* states in TASK_REPORT: */
+	"R (running)",		/* 0x00 */
+	"S (sleeping)",		/* 0x01 */
+	"D (disk sleep)",	/* 0x02 */
+	"T (stopped)",		/* 0x04 */
+	"t (tracing stop)",	/* 0x08 */
+	"X (dead)",		/* 0x10 */
+	"Z (zombie)",		/* 0x20 */
+
+	/* states beyond TASK_REPORT: */
+	"I (idle)",		/* 0x40 */
 };
 
 static inline const char *get_task_state(struct task_struct *tsk)
 {
-	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != ARRAY_SIZE(task_state_array) - 1);
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT_MAX) != ARRAY_SIZE(task_state_array));
 	return task_state_array[__get_task_state(tsk)];
 }
include/linux/sched.h +10 -2
@@ -1242,22 +1242,30 @@ static inline pid_t task_pgrp_nr(struct
 	return task_pgrp_nr_ns(tsk, &init_pid_ns);
 }
 
+#define TASK_REPORT_IDLE	(TASK_REPORT + 1)
+#define TASK_REPORT_MAX		(TASK_REPORT_IDLE << 1)
+
 static inline unsigned int __get_task_state(struct task_struct *tsk)
 {
 	unsigned int tsk_state = READ_ONCE(tsk->state);
 	unsigned int state = (tsk_state | tsk->exit_state) & TASK_REPORT;
 
+	BUILD_BUG_ON_NOT_POWER_OF_2(TASK_REPORT_MAX);
+
 	if (tsk_state == TASK_PARKED)
 		state = TASK_INTERRUPTIBLE;
 
+	if (tsk_state == TASK_IDLE)
+		state = TASK_REPORT_IDLE;
+
 	return fls(state);
 }
 
 static inline char __task_state_to_char(unsigned int state)
 {
-	static const char state_char[] = "RSDTtXZ";
+	static const char state_char[] = "RSDTtXZI";
 
-	BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != sizeof(state_char) - 2);
+	BUILD_BUG_ON(1 + ilog2(TASK_REPORT_MAX) != sizeof(state_char) - 1);
 
 	return state_char[state];
 }
include/trace/events/sched.h +4 -3
@@ -156,10 +156,11 @@ TRACE_EVENT(sched_switch,
 	TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d",
 		__entry->prev_comm, __entry->prev_pid, __entry->prev_prio,
 
-		(__entry->prev_state & TASK_REPORT) ?
-		  __print_flags(__entry->prev_state & TASK_REPORT, "|",
+		(__entry->prev_state & (TASK_REPORT_MAX - 1)) ?
+		  __print_flags(__entry->prev_state & (TASK_REPORT_MAX - 1), "|",
 				{ 0x01, "S" }, { 0x02, "D" }, { 0x04, "T" },
-				{ 0x08, "t" }, { 0x10, "X" }, { 0x20, "Z" }) :
+				{ 0x08, "t" }, { 0x10, "X" }, { 0x20, "Z" },
+				{ 0x40, "I" }) :
 		  "R",
 
 		__entry->prev_state & TASK_STATE_MAX ? "+" : "",
author maintainer
#11 lore raw

PATCH 7/8 sched: Ignore TASK_IDLE for SysRq-W

In reply to Peter Zijlstra

Markus reported that tasks in TASK_IDLE state are reported by SysRq-W, which results in undesirable clutter.

Reported-by Markus Trippelsdorf <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 kernel/sched/core.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)
kernel/sched/core.c +23 -1
@@ -5164,6 +5164,28 @@ void sched_show_task(struct task_struct
 	put_task_stack(p);
 }
 
+static inline bool
+state_filter_match(unsigned long state_filter, struct task_struct *p)
+{
+	/* no filter, everything matches */
+	if (!state_filter)
+		return true;
+
+	/* filter, but doesn't match */
+	if (!(p->state & state_filter))
+		return false;
+
+	/*
+	 * When looking for TASK_UNINTERRUPTIBLE skip TASK_IDLE (allows
+	 * TASK_KILLABLE).
+	 */
+	if (state_filter == TASK_UNINTERRUPTIBLE && p->state == TASK_IDLE)
+		return false;
+
+	return true;
+}
+
+
 void show_state_filter(unsigned long state_filter)
 {
 	struct task_struct *g, *p;

@@ -5186,7 +5208,7 @@ void show_state_filter(unsigned long sta
 		 */
 		touch_nmi_watchdog();
 		touch_all_softlockup_watchdogs();
-		if (!state_filter || (p->state & state_filter))
+		if (state_filter_match(state_filter, p))
 			sched_show_task(p);
 	}
author maintainer
#12 lore raw

PATCH 8/8 sched: Add explicit TASK_PARKED printing

In reply to Peter Zijlstra

Currently TASK_PARKED is masqueraded as TASK_INTERRUPTIBLE, give it its own print state because it will not in fact get woken by regular wakeups and is a long-term state.

This requires moving TASK_PARKED into the TASK_REPORT mask, and since that latter needs to be a contiguous bitmask, we need to shuffle the bits around a bit.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>
---
 fs/proc/array.c              |    3 ++-
 include/linux/sched.h        |   16 +++++++---------
 include/trace/events/sched.h |    2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)
fs/proc/array.c +2 -1
@@ -127,9 +127,10 @@ static const char * const task_state_arr
 	"t (tracing stop)",	/* 0x08 */
 	"X (dead)",		/* 0x10 */
 	"Z (zombie)",		/* 0x20 */
+	"P (parked)",		/* 0x40 */
 
 	/* states beyond TASK_REPORT: */
-	"I (idle)",		/* 0x40 */
+	"I (idle)",		/* 0x80 */
 };
 
 static inline const char *get_task_state(struct task_struct *tsk)
include/linux/sched.h +7 -9
@@ -75,10 +75,10 @@ struct task_group;
 #define EXIT_ZOMBIE			0x0020
 #define EXIT_TRACE			(EXIT_ZOMBIE | EXIT_DEAD)
 /* Used in tsk->state again: */
-#define TASK_DEAD			0x0040
-#define TASK_WAKEKILL			0x0080
-#define TASK_WAKING			0x0100
-#define TASK_PARKED			0x0200
+#define TASK_PARKED			0x0040
+#define TASK_DEAD			0x0080
+#define TASK_WAKEKILL			0x0100
+#define TASK_WAKING			0x0200
 #define TASK_NOLOAD			0x0400
 #define TASK_NEW			0x0800
 #define TASK_STATE_MAX			0x1000

@@ -97,7 +97,8 @@ struct task_group;
 /* get_task_state(): */
 #define TASK_REPORT			(TASK_RUNNING | TASK_INTERRUPTIBLE | \
 					 TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
-					 __TASK_TRACED | EXIT_DEAD | EXIT_ZOMBIE)
+					 __TASK_TRACED | EXIT_DEAD | EXIT_ZOMBIE | \
+					 TASK_PARKED)
 
 #define task_is_traced(task)		((task->state & __TASK_TRACED) != 0)
 

@@ -1252,9 +1253,6 @@ static inline unsigned int __get_task_st
 
 	BUILD_BUG_ON_NOT_POWER_OF_2(TASK_REPORT_MAX);
 
-	if (tsk_state == TASK_PARKED)
-		state = TASK_INTERRUPTIBLE;
-
 	if (tsk_state == TASK_IDLE)
 		state = TASK_REPORT_IDLE;
 

@@ -1263,7 +1261,7 @@ static inline unsigned int __get_task_st
 
 static inline char __task_state_to_char(unsigned int state)
 {
-	static const char state_char[] = "RSDTtXZI";
+	static const char state_char[] = "RSDTtXZPI";
 
 	BUILD_BUG_ON(1 + ilog2(TASK_REPORT_MAX) != sizeof(state_char) - 1);
include/trace/events/sched.h +1 -1
@@ -160,7 +160,7 @@ TRACE_EVENT(sched_switch,
 		  __print_flags(__entry->prev_state & (TASK_REPORT_MAX - 1), "|",
 				{ 0x01, "S" }, { 0x02, "D" }, { 0x04, "T" },
 				{ 0x08, "t" }, { 0x10, "X" }, { 0x20, "Z" },
-				{ 0x40, "I" }) :
+				{ 0x40, "P" }, { 0x80, "I" }) :
 		  "R",
 
 		__entry->prev_state & TASK_STATE_MAX ? "+" : "",
Reviewed-by on the v1 series
#13 lore raw
In reply to Peter Zijlstra
Quoted 6 lines · Peter Zijlstra

With wider usage of TASK_IDLE came a few user visible annoyances. Markus reported that TASK_IDLE is reported as TASK_UNINTERRUPTIBLE and some tools like htop mark those highly visible, which is not warranted for TASK_IDLE.

This patch set reworks the task-state printing and introduces explicit handling for TASK_IDLE and TASK_PARKED.

Thanks for cleaning that up. I went through the lot and did not find anything to bitch about.

Reviewed-by Thomas Gleixner <[email protected]>