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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxx>
>>> 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@xxxxxxxxxxxxx>
>>> Cc: "Srivatsa S. Bhat" <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
>>> Reported-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
>>> Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
>>> ---
>>>  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@xxxxxxxxxxxxxxxxxx>
> 
> 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

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