[PATCH v2 0/2] sched/debug: schedstats bug fix and improvement

[PATCH v2 0/2] sched/debug: schedstats bug fix and improvement

[Josh Poimboeuf]

v2:
- fixed !SCHEDSTATS build for patch 1 using new schedstat_val() macro
- dropped patch 3

A schedstats-related bug fix and improvement.

Josh Poimboeuf (2):
  sched/debug: fix /proc/sched_debug regression
  sched/debug: always show nr_migrations

 kernel/sched/debug.c | 17 +++++------------
 kernel/sched/stats.h |  3 +++
 2 files changed, 8 insertions(+), 12 deletions(-)

-- 
2.4.11

[PATCH v2 1/2] sched/debug: fix /proc/sched_debug regression

[Josh Poimboeuf]

Commit cb2517653fcc ("sched/debug: Make schedstats a runtime tunable
that is disabled by default") introduced a bug when CONFIG_SCHEDSTATS is
enabled and the runtime tunable is disabled (which is the default).  The
wait-time, sum-exec, and sum-sleep fields are missing from the
/proc/sched_debug file in the runnable_tasks section.

Fix it with a new schedstat_val() macro which returns the field value
when schedstats is enabled and zero otherwise.  The macro works with
both SCHEDSTATS and !SCHEDSTATS.  I put the macro in stats.h since it
might end up being useful in other places.

Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
Cc: stable@vger.kernel.org
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/sched/debug.c | 15 ++++-----------
 kernel/sched/stats.h |  3 +++
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index cf905f6..0368c39 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -427,19 +427,12 @@ print_task(struct seq_file *m, struct rq *rq, struct task_struct *p)
 		SPLIT_NS(p->se.vruntime),
 		(long long)(p->nvcsw + p->nivcsw),
 		p->prio);
-#ifdef CONFIG_SCHEDSTATS
-	if (schedstat_enabled()) {
-		SEQ_printf(m, "%9Ld.%06ld %9Ld.%06ld %9Ld.%06ld",
-			SPLIT_NS(p->se.statistics.wait_sum),
-			SPLIT_NS(p->se.sum_exec_runtime),
-			SPLIT_NS(p->se.statistics.sum_sleep_runtime));
-	}
-#else
+
 	SEQ_printf(m, "%9Ld.%06ld %9Ld.%06ld %9Ld.%06ld",
-		0LL, 0L,
+		SPLIT_NS(schedstat_val(p, se.statistics.wait_sum)),
 		SPLIT_NS(p->se.sum_exec_runtime),
-		0LL, 0L);
-#endif
+		SPLIT_NS(schedstat_val(p, se.statistics.sum_sleep_runtime)));
+
 #ifdef CONFIG_NUMA_BALANCING
 	SEQ_printf(m, " %d %d", task_node(p), task_numa_group_id(p));
 #endif
diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
index 70b3b6a..78955cb 100644
--- a/kernel/sched/stats.h
+++ b/kernel/sched/stats.h
@@ -33,6 +33,8 @@ rq_sched_info_dequeued(struct rq *rq, unsigned long long delta)
 # define schedstat_inc(rq, field)	do { if (schedstat_enabled()) { (rq)->field++; } } while (0)
 # define schedstat_add(rq, field, amt)	do { if (schedstat_enabled()) { (rq)->field += (amt); } } while (0)
 # define schedstat_set(var, val)	do { if (schedstat_enabled()) { var = (val); } } while (0)
+# define schedstat_val(rq, field)	((schedstat_enabled()) ? (rq)->field : 0)
+
 #else /* !CONFIG_SCHEDSTATS */
 static inline void
 rq_sched_info_arrive(struct rq *rq, unsigned long long delta)
@@ -47,6 +49,7 @@ rq_sched_info_depart(struct rq *rq, unsigned long long delta)
 # define schedstat_inc(rq, field)	do { } while (0)
 # define schedstat_add(rq, field, amt)	do { } while (0)
 # define schedstat_set(var, val)	do { } while (0)
+# define schedstat_val(rq, field)	0
 #endif
 
 #ifdef CONFIG_SCHED_INFO
-- 
2.4.11

[PATCH v2 2/2] sched/debug: always show nr_migrations

[Josh Poimboeuf]

The nr_migrations field is updated independently of CONFIG_SCHEDSTATS,
so it can be displayed regardless.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/sched/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 0368c39..2a0a999 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -879,9 +879,9 @@ void proc_sched_show_task(struct task_struct *p, struct seq_file *m)
 
 	nr_switches = p->nvcsw + p->nivcsw;
 
-#ifdef CONFIG_SCHEDSTATS
 	P(se.nr_migrations);
 
+#ifdef CONFIG_SCHEDSTATS
 	if (schedstat_enabled()) {
 		u64 avg_atom, avg_per_cpu;
 
-- 
2.4.11

Re: [PATCH v2 1/2] sched/debug: fix /proc/sched_debug regression

[Mel Gorman]

On Fri, Jun 03, 2016 at 05:58:40PM -0500, Josh Poimboeuf wrote:
> Commit cb2517653fcc ("sched/debug: Make schedstats a runtime tunable
> that is disabled by default") introduced a bug when CONFIG_SCHEDSTATS is
> enabled and the runtime tunable is disabled (which is the default).  The
> wait-time, sum-exec, and sum-sleep fields are missing from the
> /proc/sched_debug file in the runnable_tasks section.
> 

I take it that this breaks userspace parsing?

> Fix it with a new schedstat_val() macro which returns the field value
> when schedstats is enabled and zero otherwise.  The macro works with
> both SCHEDSTATS and !SCHEDSTATS.  I put the macro in stats.h since it
> might end up being useful in other places.
> 
> Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
> Cc: stable@vger.kernel.org
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH v2 2/2] sched/debug: always show nr_migrations

[Mel Gorman]

On Fri, Jun 03, 2016 at 05:58:41PM -0500, Josh Poimboeuf wrote:
> The nr_migrations field is updated independently of CONFIG_SCHEDSTATS,
> so it can be displayed regardless.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH v2 1/2] sched/debug: fix /proc/sched_debug regression

[Josh Poimboeuf]

On Tue, Jun 07, 2016 at 02:47:57PM +0100, Mel Gorman wrote:
> On Fri, Jun 03, 2016 at 05:58:40PM -0500, Josh Poimboeuf wrote:
> > Commit cb2517653fcc ("sched/debug: Make schedstats a runtime tunable
> > that is disabled by default") introduced a bug when CONFIG_SCHEDSTATS is
> > enabled and the runtime tunable is disabled (which is the default).  The
> > wait-time, sum-exec, and sum-sleep fields are missing from the
> > /proc/sched_debug file in the runnable_tasks section.
> > 
> 
> I take it that this breaks userspace parsing?

In theory, yes, though I don't know if there are any tools out there
which parse it.

> > Fix it with a new schedstat_val() macro which returns the field value
> > when schedstats is enabled and zero otherwise.  The macro works with
> > both SCHEDSTATS and !SCHEDSTATS.  I put the macro in stats.h since it
> > might end up being useful in other places.
> > 
> > Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Acked-by: Mel Gorman <mgorman@techsingularity.net>

Thanks!

-- 
Josh

[tip:sched/core] sched/debug: Fix /proc/sched_debug regression

[tip-bot for Josh Poimboeuf]

Commit-ID:  9c57259117b9c25472a3fa6d5a14d6bb3b647e87
Gitweb:     http://git.kernel.org/tip/9c57259117b9c25472a3fa6d5a14d6bb3b647e87
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Fri, 3 Jun 2016 17:58:40 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 8 Jun 2016 14:31:58 +0200

sched/debug: Fix /proc/sched_debug regression

Commit:

  cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")

... introduced a bug when CONFIG_SCHEDSTATS is enabled and the
runtime tunable is disabled (which is the default).

The wait-time, sum-exec, and sum-sleep fields are missing from the
/proc/sched_debug file in the runnable_tasks section.

Fix it with a new schedstat_val() macro which returns the field value
when schedstats is enabled and zero otherwise.  The macro works with
both SCHEDSTATS and !SCHEDSTATS.  I put the macro in stats.h since it
might end up being useful in other places.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: cb2517653fcc ("sched/debug: Make schedstats a runtime tunable that is disabled by default")
Link: http://lkml.kernel.org/r/bcda7c2790cf2ccbe586a28c02dd7b6fe7749a2b.1464994423.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/debug.c | 15 ++++-----------
 kernel/sched/stats.h |  3 +++
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index cf905f6..0368c39 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -427,19 +427,12 @@ print_task(struct seq_file *m, struct rq *rq, struct task_struct *p)
 		SPLIT_NS(p->se.vruntime),
 		(long long)(p->nvcsw + p->nivcsw),
 		p->prio);
-#ifdef CONFIG_SCHEDSTATS
-	if (schedstat_enabled()) {
-		SEQ_printf(m, "%9Ld.%06ld %9Ld.%06ld %9Ld.%06ld",
-			SPLIT_NS(p->se.statistics.wait_sum),
-			SPLIT_NS(p->se.sum_exec_runtime),
-			SPLIT_NS(p->se.statistics.sum_sleep_runtime));
-	}
-#else
+
 	SEQ_printf(m, "%9Ld.%06ld %9Ld.%06ld %9Ld.%06ld",
-		0LL, 0L,
+		SPLIT_NS(schedstat_val(p, se.statistics.wait_sum)),
 		SPLIT_NS(p->se.sum_exec_runtime),
-		0LL, 0L);
-#endif
+		SPLIT_NS(schedstat_val(p, se.statistics.sum_sleep_runtime)));
+
 #ifdef CONFIG_NUMA_BALANCING
 	SEQ_printf(m, " %d %d", task_node(p), task_numa_group_id(p));
 #endif
diff --git a/kernel/sched/stats.h b/kernel/sched/stats.h
index 70b3b6a..78955cb 100644
--- a/kernel/sched/stats.h
+++ b/kernel/sched/stats.h
@@ -33,6 +33,8 @@ rq_sched_info_dequeued(struct rq *rq, unsigned long long delta)
 # define schedstat_inc(rq, field)	do { if (schedstat_enabled()) { (rq)->field++; } } while (0)
 # define schedstat_add(rq, field, amt)	do { if (schedstat_enabled()) { (rq)->field += (amt); } } while (0)
 # define schedstat_set(var, val)	do { if (schedstat_enabled()) { var = (val); } } while (0)
+# define schedstat_val(rq, field)	((schedstat_enabled()) ? (rq)->field : 0)
+
 #else /* !CONFIG_SCHEDSTATS */
 static inline void
 rq_sched_info_arrive(struct rq *rq, unsigned long long delta)
@@ -47,6 +49,7 @@ rq_sched_info_depart(struct rq *rq, unsigned long long delta)
 # define schedstat_inc(rq, field)	do { } while (0)
 # define schedstat_add(rq, field, amt)	do { } while (0)
 # define schedstat_set(var, val)	do { } while (0)
+# define schedstat_val(rq, field)	0
 #endif
 
 #ifdef CONFIG_SCHED_INFO

[tip:sched/core] sched/debug: Always show 'nr_migrations'

[tip-bot for Josh Poimboeuf]

Commit-ID:  03c041c5bf6ed584dff36b7cd509e0146a124277
Gitweb:     http://git.kernel.org/tip/03c041c5bf6ed584dff36b7cd509e0146a124277
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Fri, 3 Jun 2016 17:58:41 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 8 Jun 2016 14:34:49 +0200

sched/debug: Always show 'nr_migrations'

The nr_migrations field is updated independently of CONFIG_SCHEDSTATS,
so it can be displayed regardless.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/5b1b04057ae2b14d73c2d03f56582c1d38cfe066.1464994423.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 0368c39..2a0a999 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -879,9 +879,9 @@ void proc_sched_show_task(struct task_struct *p, struct seq_file *m)
 
 	nr_switches = p->nvcsw + p->nivcsw;
 
-#ifdef CONFIG_SCHEDSTATS
 	P(se.nr_migrations);
 
+#ifdef CONFIG_SCHEDSTATS
 	if (schedstat_enabled()) {
 		u64 avg_atom, avg_per_cpu;