[PATCH] fix idle ticks in cpu summary line of /proc/stat

[PATCH] fix idle ticks in cpu summary line of /proc/stat

[Martin Schwidefsky]

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.

Cc: Michal Hocko <mhocko@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel <linux-kernel@vger.kernel.org>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 kernel/time/tick-sched.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -213,7 +213,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *l
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 	ktime_t now, idle;
 
-	if (!tick_nohz_enabled)
+	if (!tick_nohz_enabled || !cpu_online(cpu))
 		return -1;
 
 	now = ktime_get();
@@ -254,7 +254,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64
 	struct tick_sched *ts = &per_cpu(tick_cpu_sched, cpu);
 	ktime_t now, iowait;
 
-	if (!tick_nohz_enabled)
+	if (!tick_nohz_enabled || !cpu_online(cpu))
 		return -1;
 
 	now = ktime_get();

Re: [PATCH] fix idle ticks in cpu summary line of /proc/stat

[Michal Hocko]

On Sun 11-03-12 18:26:21, Martin Schwidefsky wrote:
> 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.

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?
---
>From ccd68723637d7003124704a3329179a5947c54d2 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
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 get_cpu_idle_time_us and get_cpu_iowait_time_us
should check for offline cpus.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel <linux-kernel@vger.kernel.org>
Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/time/tick-sched.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 7656642..dec767f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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);
 
 			iowait = ktime_add(ts->iowait_sleeptime, delta);
-- 
1.7.9.1


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] fix idle ticks in cpu summary line of /proc/stat

[Martin Schwidefsky]

On Mon, 12 Mar 2012 13:17:26 +0100
Michal Hocko <mhocko@suse.cz> 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?
..
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 7656642..dec767f 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -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);
> 
>  			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?

-- 
blue skies,
   Martin.

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

Re: [PATCH] fix idle ticks in cpu summary line of /proc/stat

[Srivatsa S. Bhat]

On 03/12/2012 07:47 PM, Martin Schwidefsky wrote:

> On Mon, 12 Mar 2012 13:17:26 +0100
> Michal Hocko <mhocko@suse.cz> 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?
> ..
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index 7656642..dec767f 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -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);
>>
>>  			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

Re: [PATCH] fix idle ticks in cpu summary line of /proc/stat

[Michal Hocko]

On Mon 12-03-12 20:18:33, Srivatsa S. Bhat wrote:
> On 03/12/2012 07:47 PM, Martin Schwidefsky wrote:
> 
> > On Mon, 12 Mar 2012 13:17:26 +0100
> > Michal Hocko <mhocko@suse.cz> 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?
> > ..
> >> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> >> index 7656642..dec767f 100644
> >> --- a/kernel/time/tick-sched.c
> >> +++ b/kernel/time/tick-sched.c
> >> @@ -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);
> >>
> >>  			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?

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):

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 121f77c..d437258 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -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)
 
 static u64 get_iowait_time(int cpu)
 {
-	u64 iowait, iowait_time = get_cpu_iowait_time_us(cpu, NULL);
+	u64 iowait, iowait_time = -1ULL;
+	
+	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 */

> 
> 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,

This is a separate issue AFAIU. If there is a possibility that somebody
is calling it like that then it is a bug. I am not familiar with cpu
governors (non-NULL update users) but we shouldn't paper over any bug with
this /proc/stat fix.

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] fix idle ticks in cpu summary line of /proc/stat

[Martin Schwidefsky]

On Mon, 12 Mar 2012 16:39:06 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> 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):
> 
> diff --git a/fs/proc/stat.c b/fs/proc/stat.c
> index 121f77c..d437258 100644
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -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)
> 
>  static u64 get_iowait_time(int cpu)
>  {
> -	u64 iowait, iowait_time = get_cpu_iowait_time_us(cpu, NULL);
> +	u64 iowait, iowait_time = -1ULL;
> +	
> +	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 */
> 

That looks better and should be equivalent.

-- 
blue skies,
   Martin.

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

[PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[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 <mhocko@suse.cz>
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 <tglx@linutronix.de>
Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 fs/proc/stat.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 121f77c..62bda24 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -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;
+
+	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);
-- 
1.7.9.1

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Srivatsa S. Bhat]

On 03/13/2012 01:37 PM, Michal Hocko wrote:

> 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 <mhocko@suse.cz>
> 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 <tglx@linutronix.de>
> Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  fs/proc/stat.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/stat.c b/fs/proc/stat.c
> index 121f77c..62bda24 100644
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -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;
> +
> +	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 <srivatsa.bhat@linux.vnet.ibm.com>
 
Regards,
Srivatsa S. Bhat

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Thomas Gleixner]

On Tue, 13 Mar 2012, Michal Hocko wrote:

> 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 <mhocko@suse.cz>
> 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 <tglx@linutronix.de>
> Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

Martin, does that solve the problem for you ?

Thanks,

	tglx

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Martin Schwidefsky]

Hi Thomas,

On Thu, 29 Mar 2012 12:42:22 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 13 Mar 2012, Michal Hocko wrote:
> 
> > 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 <mhocko@suse.cz>
> > 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 <tglx@linutronix.de>
> > Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> > Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> 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 <schwidefsky@de.ibm.com>

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 <schwidefsky@de.ibm.com>
---
 fs/proc/stat.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -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;

-- 
blue skies,
   Martin.

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

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Michal Hocko]

On Fri 30-03-12 12:23:08, Martin Schwidefsky wrote:
> Hi Thomas,
> 
> On Thu, 29 Mar 2012 12:42:22 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Tue, 13 Mar 2012, Michal Hocko wrote:
> > 
> > > 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 <mhocko@suse.cz>
> > > 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 <tglx@linutronix.de>
> > > Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> > > Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > 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 <schwidefsky@de.ibm.com>
> 
> 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 <schwidefsky@de.ibm.com>

Looks good to me.
Reviewed-by: Michal Hocko <mhocko@suse.cz>

> ---
>  fs/proc/stat.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -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;
> 
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Srivatsa S. Bhat]

On 03/30/2012 03:53 PM, Martin Schwidefsky wrote:

> Hi Thomas,
> 
> On Thu, 29 Mar 2012 12:42:22 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> On Tue, 13 Mar 2012, Michal Hocko wrote:
>>
>>> 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 <mhocko@suse.cz>
>>> 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 <tglx@linutronix.de>
>>> Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>>> Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>
>> 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 <schwidefsky@de.ibm.com>
> 
> 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 <schwidefsky@de.ibm.com>


Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

> ---
>  fs/proc/stat.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -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;
> 

[tip:timers/core] proc: stats: Use arch_idle_time for idle and iowait times if available

[tip-bot for Martin Schwidefsky]

Commit-ID:  cb85a6ed67e979c59a29b7b4e8217e755b951cf4
Gitweb:     http://git.kernel.org/tip/cb85a6ed67e979c59a29b7b4e8217e755b951cf4
Author:     Martin Schwidefsky <schwidefsky@de.ibm.com>
AuthorDate: Fri, 30 Mar 2012 12:23:08 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
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 <schwidefsky@de.ibm.com>
Reviewed-by: Michal Hocko <mhocko@suse.cz>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/20120330122308.18720283@de.ibm.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 fs/proc/stat.c |   34 ++++++++++++++++++++++++++++------
 1 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 6a0c62d..64c3b31 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -18,19 +18,39 @@
 #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)
 {
 	u64 idle, idle_time = get_cpu_idle_time_us(cpu, NULL);
 
-	if (idle_time == -1ULL) {
+	if (idle_time == -1ULL)
 		/* !NO_HZ 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;
@@ -49,6 +69,8 @@ static u64 get_iowait_time(int cpu)
 	return iowait;
 }
 
+#endif
+
 static int show_stat(struct seq_file *p, void *v)
 {
 	int i, j;

Re: [tip:timers/core] proc: stats: Use arch_idle_time for idle and iowait times if available

[Andrew Morton]

On Fri, 30 Mar 2012 06:58:25 -0700
tip-bot for Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> Commit-ID:  cb85a6ed67e979c59a29b7b4e8217e755b951cf4
> Gitweb:     http://git.kernel.org/tip/cb85a6ed67e979c59a29b7b4e8217e755b951cf4
> Author:     Martin Schwidefsky <schwidefsky@de.ibm.com>
> AuthorDate: Fri, 30 Mar 2012 12:23:08 +0200
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> 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.

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

> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Reviewed-by: Michal Hocko <mhocko@suse.cz>
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Link: http://lkml.kernel.org/r/20120330122308.18720283@de.ibm.com
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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



From: Michal Hocko <mhocko@suse.cz>
Subject: nohz: fix idle ticks in cpu summary line of /proc/stat

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 <tglx@linutronix.de>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
Cc: <stable@vger.kernel.org>	[3.2.x]
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

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

diff -puN fs/proc/stat.c~nohz-fix-idle-ticks-in-cpu-summary-line-of-proc-stat fs/proc/stat.c
--- a/fs/proc/stat.c~nohz-fix-idle-ticks-in-cpu-summary-line-of-proc-stat
+++ a/fs/proc/stat.c
@@ -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;
+
+	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);
_

Re: [tip:timers/core] proc: stats: Use arch_idle_time for idle and iowait times if available

[Martin Schwidefsky]

On Fri, 30 Mar 2012 15:54:55 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 30 Mar 2012 06:58:25 -0700
> tip-bot for Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
> 
> > Commit-ID:  cb85a6ed67e979c59a29b7b4e8217e755b951cf4
> > Gitweb:     http://git.kernel.org/tip/cb85a6ed67e979c59a29b7b4e8217e755b951cf4
> > Author:     Martin Schwidefsky <schwidefsky@de.ibm.com>
> > AuthorDate: Fri, 30 Mar 2012 12:23:08 +0200
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > 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.
> 
> So it appears that this patch is a superset of "nohz: fix idle ticks in
> cpu summary line of /proc/stat" (below), yes?

"proc:stats: Use arch_idle_time.." goes on top of "nohz: fix idle ticks..".
If the second patch is applied, s390 does not need the first one anymore.
So for s390 the second one is a superset of the first, on x86 the first
patch is the important one.
 
> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Reviewed-by: Michal Hocko <mhocko@suse.cz>
> > Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > Link: http://lkml.kernel.org/r/20120330122308.18720283@de.ibm.com
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> No cc:stable?  Both 09a1d34f8535ecf9 and a25cac5198d date from
> September '11 and 09a1d34f8535ecf9 (at least) was a regression.
 
The regression is fixed by "proc:stats: User arch_idle_time..", the
second patch gets us back the improved values for s390, you could argue
if this is a regression or not.
Anyway I would not complain if both patches are included the stable
releases. 

-- 
blue skies,
   Martin.

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

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Martin Schwidefsky]

On Tue, 13 Mar 2012 14:02:35 +0530
"Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> wrote:

> On 03/13/2012 01:37 PM, Michal Hocko wrote:
> 
> > 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 <mhocko@suse.cz>
> > 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 <tglx@linutronix.de>
> > Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> > Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > ---
> >  fs/proc/stat.c |   14 ++++++++++----
> >  1 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/proc/stat.c b/fs/proc/stat.c
> > index 121f77c..62bda24 100644
> > --- a/fs/proc/stat.c
> > +++ b/fs/proc/stat.c
> > @@ -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;
> > +
> > +	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 <srivatsa.bhat@linux.vnet.ibm.com>

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?

-- 
blue skies,
   Martin.

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

Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat

[Srivatsa S. Bhat]

On 07/18/2012 05:22 PM, Martin Schwidefsky wrote:
> On Tue, 13 Mar 2012 14:02:35 +0530
> "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 
>> On 03/13/2012 01:37 PM, Michal Hocko wrote:
>>
>>> 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 <mhocko@suse.cz>
>>> 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 <tglx@linutronix.de>
>>> Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
>>> Reported-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>> Signed-off-by: Michal Hocko <mhocko@suse.cz>
>>> ---
>>>  fs/proc/stat.c |   14 ++++++++++----
>>>  1 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/proc/stat.c b/fs/proc/stat.c
>>> index 121f77c..62bda24 100644
>>> --- a/fs/proc/stat.c
>>> +++ b/fs/proc/stat.c
>>> @@ -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;
>>> +
>>> +	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 <srivatsa.bhat@linux.vnet.ibm.com>
> 
> 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?
> 

Oh, right! Thomas, could you kindly pick up Michal's patch please?

Martin had outlined the relationship between the 2 patches here:
http://thread.gmane.org/gmane.linux.kernel/1265374/focus=1276336

Regards,
Srivatsa S. Bhat