[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linaro-dev
Subject:    [PATCH 2/2] Remove hardcoded c and p states
From:       amit.arora () linaro ! org (Amit Arora)
Date:       2010-07-30 10:00:16
Message-ID: AANLkTin5WujtfQ6fsfHOefKdHDJXtGeNnqRKHnuXffrL () mail ! gmail ! com
[Download RAW message or body]

On Thu, Jul 29, 2010 at 6:18 PM, Amit Kucheria <amit.kucheria at linaro.org> wrote:
> On 10 Jul 29, Amit Arora wrote:
>> +/**
>> + * print_generic_cstates() - Prints the list of supported C-states.
>> + *
>> + * This functions uses standard sysfs interface of the cpuidle framework
>> + * to extract the information of the C-states supported by the Linux
>> + * kernel.
>> + **/
>> +void print_generic_cstates(void)
>> +{
>> + ? ? DIR *dir;
>> + ? ? struct dirent *entry;
>> +
>> + ? ? dir = opendir("/sys/devices/system/cpu/cpu0/cpuidle");
>
> Is this safe for multi-core cases? Is it safe to assume that the c-states on
> all cores will be the same?

Am not sure about this. Are you aware of any case where different
cores may have different number of supported c-states ?

BTW, this doesn't change the powertop output for c-states (Avg
residency %, etc). For that we go through each core to find out
supported cstates and related information (time, desc etc.).

But, yes, if we may have different c-states for different cpus/cores,
it will make sense to go through each core to find out supported
c-state levels.

> Also, I see that powertop shows "Avg. residency" in each C-state. Is that an
> average of the the residency of each core in a particular C-state?

Yes. It is an average of all the cores. read_data_cpuidle() walks
through all the cpuN directories under /sys/devices/system/cpu/ and
collates this information from each core for a particular c-state
level.
And main() (which calls read_data_cpuidle via read_data) in the while
loop, takes an average of the time duration for each state depending
on number of online CPUs.


>> ? ? ? do_proc_irq();
>> ? ? ? do_proc_irq();
>> ? ? ? do_cpufreq_stats();
>> @@ -924,8 +989,8 @@ int main(int argc, char **argv)
>> ? ? ? printf("\n\n");
>> ?#if defined (__I386__)
>> ? ? ? print_intel_cstates();
>
> If we now have print_generic_cstates (below), then can't we get rid of the
> #ifdef'ery for I386 vs. others?

i386 does some extra work in finding out which states are supported by
the CPU (from cpuidle sysfs interface) and which ones are supported by
BIOS (it uses x86 cpuid() call for this).

So, even if we keep common function for all archs, we will need
"#ifdef __i386__" inside that function.


Thanks for looking at the patches!
--
Regards,
Amit Arora


[prev in list] [next in list] [prev in thread] [next in thread]