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@xxxxxxx> 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

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