Re: [Powertop] [PATCH] Allow frequency stats when cpuidle is not enabled

[Chris Ferron <chris.e.ferron at linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]

On 11/23/2012 07:30 AM, Sergey Senozhatsky wrote:
> On (11/22/12 13:05), Rajagopal Venkat wrote:
>> Powertop fails to display frequency stats when cpuidle subsystem
>> is not enabled. Fix it.
>>
>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat(a)linaro.org>
>> ---
>>   src/cpu/cpu_linux.cpp | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
> thanks for your patch.
>
> frankly, looking at measurement_start()/measurement_end() I've a feeling that it's
> better to refactor these functions. both of them are performing different parsings of
> P/C states, and with your patch we jump over 50% of functionality, which looks to me
> like a reason to make measurement_start()/measurement_end() smaller by moving
> P/C states parsing to separate functions:
Sorry for the late response, but I think i agree with Sergey is connect 
here. Besides I personally am not a huge fan of using "goto" statements 
unless there really no other alternatives.
>
> for example (a bit ugly function names)
>
> measurement_start()
> {
> 	parse_idle_on_measure_start();
> 	parse_stats_on_measure_start();
> }
>
> and similar for measurement_end().
I think this concept may work just fine.
>
>
>
> what do you, guys, think?
Can you refactor your change to eliminate the "goto" statements, and 
apply something like what Sergey has suggested?
>
> 	-ss
>
>
>> diff --git a/src/cpu/cpu_linux.cpp b/src/cpu/cpu_linux.cpp
>> index d6caf45..b8dbbc5 100644
>> --- a/src/cpu/cpu_linux.cpp
>> +++ b/src/cpu/cpu_linux.cpp
>> @@ -63,7 +63,7 @@ void cpu_linux::measurement_start(void)
>>   
>>   	dir = opendir(filename);
>>   	if (!dir)
>> -		return;
>> +		goto cpufreq;
>>   
>>   	/* For each C-state, there is a stateX directory which
>>   	 * contains a 'usage' and a 'time' (duration) file */
>> @@ -112,6 +112,7 @@ void cpu_linux::measurement_start(void)
>>   	}
>>   	closedir(dir);
>>   
>> +cpufreq:
>>   	last_stamp = 0;
>>   
>>   	for (i = 0; i < children.size(); i++)
>> @@ -149,7 +150,7 @@ void cpu_linux::measurement_end(void)
>>   
>>   	dir = opendir(filename);
>>   	if (!dir)
>> -		return;
>> +		goto cpufreq;
>>   
>>   	/* For each C-state, there is a stateX directory which
>>   	 * contains a 'usage' and a 'time' (duration) file */
>> @@ -188,6 +189,7 @@ void cpu_linux::measurement_end(void)
>>   	}
>>   	closedir(dir);
>>   
>> +cpufreq:
>>   	sprintf(filename, "/sys/devices/system/cpu/cpu%i/cpufreq/stats/time_in_state", number);
>>   
>>   	file.open(filename, ios::in);
>> -- 
>> 1.7.11.3
>>
>> _______________________________________________
>> PowerTop mailing list
>> PowerTop(a)lists.01.org
>> https://lists.01.org/mailman/listinfo/powertop
>>