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

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

[Chris Ferron]

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

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

[Sergey Senozhatsky]

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

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:


for example (a bit ugly function names)

measurement_start()
{
	parse_idle_on_measure_start();
	parse_stats_on_measure_start();
}

and similar for measurement_end().




what do you, guys, think?

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

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

[Rajagopal Venkat]

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

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

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