fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this get_cpu_idle_time_us and get_cpu_iowait_time_us should check for offline cpus.

From ccd68723637d7003124704a3329179a5947c54d2 Mon Sep 17 00:00:00 2001

Goot catch. But I think that the following fix should be better because it doesn't change the semantic of the function. What do you think?

kernel/time/tick-sched.c +1 -1

index 7656642..dec767f 100644
@@ -221,7 +221,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 		update_ts_time_stats(cpu, ts, now, last_update_time);
 		idle = ts->idle_sleeptime;
 	} else {
-		if (ts->idle_active && !nr_iowait_cpu(cpu)) {
+		if (cpu_online(cpu) && ts->idle_active && !nr_iowait_cpu(cpu)) {
 			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
 
 			idle = ktime_add(ts->idle_sleeptime, delta);

Quoted 2 lines · On Mon, 12 Mar 2012 13:17:26 +0100 Michal Hocko <[email protected]> wrote:

Goot catch. But I think that the following fix should be better because it doesn't change the semantic of the function. What do you think?

..

Quoted 21 lines

kernel/time/tick-sched.c +2 -2

index 7656642..dec767f 100644
@@ -221,7 +221,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 		update_ts_time_stats(cpu, ts, now, last_update_time);
 		idle = ts->idle_sleeptime;
 	} else {
-		if (ts->idle_active && !nr_iowait_cpu(cpu)) {
+		if (cpu_online(cpu) && ts->idle_active && !nr_iowait_cpu(cpu)) {
 			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
 
 			idle = ktime_add(ts->idle_sleeptime, delta);

@@ -262,7 +262,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 		update_ts_time_stats(cpu, ts, now, last_update_time);
 		iowait = ts->iowait_sleeptime;
 	} else {
-		if (ts->idle_active && nr_iowait_cpu(cpu) > 0) {
+		if (cpu_online(cpu) && ts->idle_active && nr_iowait_cpu(cpu) > 0) {
 			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
 

kernel/time/tick-sched.c u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time) · line 268

Quoted 1 line

kernel/time/tick-sched.c +0 -0

 			iowait = ktime_add(ts->iowait_sleeptime, delta);

I would prefer an early exit from the functions. The target cpu is offline, who guarantees that the "struct tick_sched" for the cpu contains anything useful?

Quoted 27 lines · Martin Schwidefsky

Quoted 2 lines · On Mon, 12 Mar 2012 13:17:26 +0100 Michal Hocko <[email protected]> wrote:

Goot catch. But I think that the following fix should be better because it doesn't change the semantic of the function. What do you think?

..

Quoted 21 lines

kernel/time/tick-sched.c +2 -2

index 7656642..dec767f 100644
@@ -221,7 +221,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *last_update_time)
 		update_ts_time_stats(cpu, ts, now, last_update_time);
 		idle = ts->idle_sleeptime;
 	} else {
-		if (ts->idle_active && !nr_iowait_cpu(cpu)) {
+		if (cpu_online(cpu) && ts->idle_active && !nr_iowait_cpu(cpu)) {
 			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
 
 			idle = ktime_add(ts->idle_sleeptime, delta);

@@ -262,7 +262,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time)
 		update_ts_time_stats(cpu, ts, now, last_update_time);
 		iowait = ts->iowait_sleeptime;
 	} else {
-		if (ts->idle_active && nr_iowait_cpu(cpu) > 0) {
+		if (cpu_online(cpu) && ts->idle_active && nr_iowait_cpu(cpu) > 0) {
 			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
 

Quoted 5 lines

kernel/time/tick-sched.c u64 get_cpu_iowait_time_us(int cpu, u64 *last_update_time) · line 268

Quoted 1 line

kernel/time/tick-sched.c +0 -0

 			iowait = ktime_add(ts->iowait_sleeptime, delta);

I would prefer an early exit from the functions. The target cpu is offline, who guarantees that the "struct tick_sched" for the cpu contains anything useful?

Also, what about the case where last_update_time is non-NULL? With Martin's patch update_ts_time_stats() won't be called for offline cpus,

whereas with Michal's patch it will be called and hence the counters will get updated.. We don't want to update counters for offline cpus right?

Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hmm, the semantic is that the function either returns the sleeptime or -1 if nohz is disabled. Bringing also online/offline into it seems rather confusing.
Maybe we shouldn't do the test layer up when we call the function instead. This should be much cleaner IMO (it also reduced cpu_online call from the governors call paths which might be a problem as well):

fs/proc/stat.c +4 -1

index 121f77c..d437258 100644
@@ -24,7 +24,10 @@
 
 static u64 get_idle_time(int cpu)
 {
-	u64 idle, idle_time = get_cpu_idle_time_us(cpu, NULL);
+	u64 idle, idle_time = -1ULL;
+	
+	if (cpu_online(cpu))
+		idle_time = get_cpu_idle_time_us(cpu, NULL);
 
 	if (idle_time == -1ULL) {
 		/* !NO_HZ so we can rely on cpustat.idle */

@@ -38,7 +41,10 @@ static u64 get_idle_time(int cpu)
 

From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---
From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/proc/stat.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

fs/proc/stat.c +6 -3

index 121f77c..62bda24 100644
@@ -24,10 +24,13 @@
 
 static u64 get_idle_time(int cpu)
 {
-	u64 idle, idle_time = get_cpu_idle_time_us(cpu, NULL);
+	u64 idle, idle_time = -1ULL;
+
+	if (cpu_online(cpu))
+		idle_time = get_cpu_idle_time_us(cpu, NULL);
 
 	if (idle_time == -1ULL) {
-		/* !NO_HZ so we can rely on cpustat.idle */
+		/* !NO_HZ or cpu offline so we can rely on cpustat.idle */
 		idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
 		idle += arch_idle_time(cpu);
 	} else

@@ -38,10 +41,13 @@ static u64 get_idle_time(int cpu)
 
 static u64 get_iowait_time(int cpu)
 {
-	u64 iowait, iowait_time = get_cpu_iowait_time_us(cpu, NULL);
+	u64 iowait, iowait_time = -1ULL;

Quoted 58 lines · Michal Hocko

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---
From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/proc/stat.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

fs/proc/stat.c +6 -3

index 121f77c..62bda24 100644
@@ -24,10 +24,13 @@
 
 static u64 get_idle_time(int cpu)
 {
-	u64 idle, idle_time = get_cpu_idle_time_us(cpu, NULL);
+	u64 idle, idle_time = -1ULL;
+
+	if (cpu_online(cpu))
+		idle_time = get_cpu_idle_time_us(cpu, NULL);
 
 	if (idle_time == -1ULL) {
-		/* !NO_HZ so we can rely on cpustat.idle */
+		/* !NO_HZ or cpu offline so we can rely on cpustat.idle */
 		idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
 		idle += arch_idle_time(cpu);
 	} else

@@ -38,10 +41,13 @@ static u64 get_idle_time(int cpu)
 
 static u64 get_iowait_time(int cpu)
 {
-	u64 iowait, iowait_time = get_cpu_iowait_time_us(cpu, NULL);
+	u64 iowait, iowait_time = -1ULL;

fs/proc/stat.c static u64 get_idle_time(int cpu) · line 53

Quoted 10 lines

fs/proc/stat.c +4 -1

+
+	if (cpu_online(cpu))
+		iowait_time = get_cpu_iowait_time_us(cpu, NULL);
 
 	if (iowait_time == -1ULL)
-		/* !NO_HZ so we can rely on cpustat.iowait */
+		/* !NO_HZ or cpu offline so we can rely on cpustat.iowait */
 		iowait = kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT];
 	else
 		iowait = usecs_to_cputime64(iowait_time);

Yeah, this looks much better..

Reviewed-by Srivatsa S. Bhat <[email protected]>

Quoted 75 lines · On Tue, 13 Mar 2012 14:02:35 +0530 "Srivatsa S. Bhat"…

Quoted 58 lines · Michal Hocko

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---
From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/proc/stat.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

fs/proc/stat.c +6 -3

index 121f77c..62bda24 100644
@@ -24,10 +24,13 @@
 
 static u64 get_idle_time(int cpu)
 {
-	u64 idle, idle_time = get_cpu_idle_time_us(cpu, NULL);
+	u64 idle, idle_time = -1ULL;
+
+	if (cpu_online(cpu))
+		idle_time = get_cpu_idle_time_us(cpu, NULL);
 
 	if (idle_time == -1ULL) {
-		/* !NO_HZ so we can rely on cpustat.idle */
+		/* !NO_HZ or cpu offline so we can rely on cpustat.idle */
 		idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
 		idle += arch_idle_time(cpu);
 	} else

@@ -38,10 +41,13 @@ static u64 get_idle_time(int cpu)
 
 static u64 get_iowait_time(int cpu)
 {
-	u64 iowait, iowait_time = get_cpu_iowait_time_us(cpu, NULL);
+	u64 iowait, iowait_time = -1ULL;

fs/proc/stat.c static u64 get_idle_time(int cpu) · line 53

Quoted 10 lines

fs/proc/stat.c +4 -1

+
+	if (cpu_online(cpu))
+		iowait_time = get_cpu_iowait_time_us(cpu, NULL);
 
 	if (iowait_time == -1ULL)
-		/* !NO_HZ so we can rely on cpustat.iowait */
+		/* !NO_HZ or cpu offline so we can rely on cpustat.iowait */
 		iowait = kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT];
 	else
 		iowait = usecs_to_cputime64(iowait_time);

Yeah, this looks much better..

Quoted 1 line

Reviewed-by Srivatsa S. Bhat <[email protected]>

What happened to this patch? The fix for s390 (git commit cb85a6ed67e979c59 "proc: stats: Use arch_idle_time for idle and iowait times if available" is upstream but the fix for non-s390 systems is missing, no?

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---

Quoted 1 line

From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Quoted 24 lines · Michal Hocko

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---

Quoted 1 line

From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Quoted 3 lines

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Martin, does that solve the problem for you ?

Hi Thomas,

Quoted 26 lines · On Thu, 29 Mar 2012 12:42:22 +0200 (CEST) Thomas Gleixner <[email protected]>…

Quoted 24 lines · Michal Hocko

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---

Quoted 1 line

From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Quoted 5 lines

Quoted 3 lines

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Martin, does that solve the problem for you ?

Yes, the patch does solve the immediate problem. I just tested if again on x86 and s390, works fine. I would like to have another change though. The idle_sleep calculation in the scheduler is fine for x86 but for s390 the arch_idle_time delivers a much better precision. A patch would look like this:
--
Subject: [PATCH] use arch_idle_time for idle and iowait times if available

From: Martin Schwidefsky <[email protected]>

Git commit a25cac5198d4ff28 "proc: Consider NO_HZ when printing idle and iowait times" changes the code for /proc/stat to use get_cpu_idle_time_us and get_cpu_iowait_time_us if the system is running with nohz enabled. For architectures which define arch_idle_time (currently s390 only) this is a change for the worse. The result of arch_idle_time is supposed to be the exact sleep time of the target cpu and should be used instead of the value kept by the scheduler.

Signed-off-by Martin Schwidefsky <[email protected]>

---
 fs/proc/stat.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

fs/proc/stat.c +28 -6

@@ -18,9 +18,30 @@
 #ifndef arch_irq_stat
 #define arch_irq_stat() 0
 #endif
-#ifndef arch_idle_time
-#define arch_idle_time(cpu) 0
-#endif
+
+#ifdef arch_idle_time
+
+static cputime64_t get_idle_time(int cpu)
+{
+	cputime64_t idle;
+
+	idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
+	if (cpu_online(cpu) && !nr_iowait_cpu(cpu))
+		idle += arch_idle_time(cpu);
+	return idle;
+}
+
+static cputime64_t get_iowait_time(int cpu)
+{
+	cputime64_t iowait;
+
+	iowait = kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT];
+	if (cpu_online(cpu) && nr_iowait_cpu(cpu))
+		iowait += arch_idle_time(cpu);
+	return iowait;
+}
+
+#else
 
 static u64 get_idle_time(int cpu)
 {

@@ -29,11 +50,10 @@ static u64 get_idle_time(int cpu)
 	if (cpu_online(cpu))
 		idle_time = get_cpu_idle_time_us(cpu, NULL);
 
-	if (idle_time == -1ULL) {
+	if (idle_time == -1ULL)
 		/* !NO_HZ or cpu offline so we can rely on cpustat.idle */
 		idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
-		idle += arch_idle_time(cpu);
-	} else
+	else
 		idle = usecs_to_cputime64(idle_time);
 
 	return idle;

@@ -55,6 +75,8 @@ static u64 get_iowait_time(int cpu)
 	return iowait;
 }
 
+#endif
+
 static int show_stat(struct seq_file *p, void *v)
 {
 	int i, j;

Signature · 11 lines

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi Thomas,

Quoted 26 lines · On Thu, 29 Mar 2012 12:42:22 +0200 (CEST) Thomas Gleixner <[email protected]>…

Quoted 24 lines · Michal Hocko

OK, so the updated version of the patch looks like this. I am sorry but I had time to only compile test this...
---

Quoted 1 line

From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Mon, 12 Mar 2012 13:11:38 +0100
Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat

Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update conditional" introduced a bug in regard to cpu hotplug. The effect is that the number of idle ticks in the cpu summary line in /proc/stat is still counting ticks for offline cpus.

Reproduction is easy, just start a workload that keeps all cpus busy, switch off one or more cpus and then watch the idle field in top. On a dual-core with one cpu 100% busy and one offline cpu you will get something like this:

%Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st

The problem is that an offline cpu still has ts->idle_active == 1. To fix this we should make sure that the cpu is online when calling get_cpu_idle_time_us and get_cpu_iowait_time_us.

Quoted 5 lines

Quoted 3 lines

Cc Thomas Gleixner <[email protected]>

Cc "Srivatsa S. Bhat" <[email protected]>

Reported-by Martin Schwidefsky <[email protected]>

Martin, does that solve the problem for you ?

Yes, the patch does solve the immediate problem. I just tested if again on x86 and s390, works fine. I would like to have another change though. The idle_sleep calculation in the scheduler is fine for x86 but for s390 the arch_idle_time delivers a much better precision. A patch would look like this:
--
Subject: [PATCH] use arch_idle_time for idle and iowait times if available

From: Martin Schwidefsky <[email protected]>

Git commit a25cac5198d4ff28 "proc: Consider NO_HZ when printing idle and iowait times" changes the code for /proc/stat to use get_cpu_idle_time_us and get_cpu_iowait_time_us if the system is running with nohz enabled. For architectures which define arch_idle_time (currently s390 only) this is a change for the worse. The result of arch_idle_time is supposed to be the exact sleep time of the target cpu and should be used instead of the value kept by the scheduler.

Signed-off-by Martin Schwidefsky <[email protected]>

---
 fs/proc/stat.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

fs/proc/stat.c +28 -6

@@ -18,9 +18,30 @@
 #ifndef arch_irq_stat
 #define arch_irq_stat() 0
 #endif
-#ifndef arch_idle_time
-#define arch_idle_time(cpu) 0
-#endif
+
+#ifdef arch_idle_time
+
+static cputime64_t get_idle_time(int cpu)
+{
+	cputime64_t idle;
+
+	idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
+	if (cpu_online(cpu) && !nr_iowait_cpu(cpu))
+		idle += arch_idle_time(cpu);
+	return idle;
+}
+
+static cputime64_t get_iowait_time(int cpu)
+{
+	cputime64_t iowait;
+
+	iowait = kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT];
+	if (cpu_online(cpu) && nr_iowait_cpu(cpu))
+		iowait += arch_idle_time(cpu);
+	return iowait;
+}
+
+#else
 
 static u64 get_idle_time(int cpu)
 {

@@ -29,11 +50,10 @@ static u64 get_idle_time(int cpu)
 	if (cpu_online(cpu))
 		idle_time = get_cpu_idle_time_us(cpu, NULL);
 
-	if (idle_time == -1ULL) {
+	if (idle_time == -1ULL)
 		/* !NO_HZ or cpu offline so we can rely on cpustat.idle */
 		idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
-		idle += arch_idle_time(cpu);
-	} else
+	else
 		idle = usecs_to_cputime64(idle_time);
 
 	return idle;

@@ -55,6 +75,8 @@ static u64 get_iowait_time(int cpu)
 	return iowait;
 }
 
+#endif
+
 static int show_stat(struct seq_file *p, void *v)
 {
 	int i, j;

Commit-ID:  cb85a6ed67e979c59a29b7b4e8217e755b951cf4
Gitweb:     http://git.kernel.org/tip/cb85a6ed67e979c59a29b7b4e8217e755b951cf4
Author:     Martin Schwidefsky <[email protected]>
AuthorDate: Fri, 30 Mar 2012 12:23:08 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 30 Mar 2012 15:43:33 +0200

proc: stats: Use arch_idle_time for idle and iowait times if available

Git commit a25cac5198d4ff28 "proc: Consider NO_HZ when printing idle and iowait times" changes the code for /proc/stat to use get_cpu_idle_time_us and get_cpu_iowait_time_us if the system is running with nohz enabled. For architectures which define arch_idle_time (currently s390 only) this is a change for the worse. The result of arch_idle_time is supposed to be the exact sleep time of the target cpu and should be used instead of the value kept by the scheduler.

Signed-off-by Martin Schwidefsky <[email protected]>

Reviewed-by Michal Hocko <[email protected]>

Reviewed-by Srivatsa S. Bhat <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Thomas Gleixner <[email protected]>

Quoted 9 lines · On Fri, 30 Mar 2012 06:58:25 -0700 tip-bot for Martin Schwidefsky…

Commit-ID:  cb85a6ed67e979c59a29b7b4e8217e755b951cf4
Gitweb:     http://git.kernel.org/tip/cb85a6ed67e979c59a29b7b4e8217e755b951cf4
Author:     Martin Schwidefsky <[email protected]>
AuthorDate: Fri, 30 Mar 2012 12:23:08 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 30 Mar 2012 15:43:33 +0200

proc: stats: Use arch_idle_time for idle and iowait times if available

Quoted 7 lines

Git commit a25cac5198d4ff28 "proc: Consider NO_HZ when printing idle and iowait times" changes the code for /proc/stat to use get_cpu_idle_time_us and get_cpu_iowait_time_us if the system is running with nohz enabled. For architectures which define arch_idle_time (currently s390 only) this is a change for the worse. The result of arch_idle_time is supposed to be the exact sleep time of the target cpu and should be used instead of the value kept by the scheduler.

So it appears that this patch is a superset of "nohz: fix idle ticks in cpu summary line of /proc/stat" (below), yes?

Quoted 5 lines

Signed-off-by Martin Schwidefsky <[email protected]>

Reviewed-by Michal Hocko <[email protected]>

Reviewed-by Srivatsa S. Bhat <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Thomas Gleixner <[email protected]>

No cc:stable? Both 09a1d34f8535ecf9 and a25cac5198d date from September '11 and 09a1d34f8535ecf9 (at least) was a regression.