Re: [PATCH] cpufreq: powernv: Redesign the presentation of throttle notification

[Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>]

Hi,

On 12/15/2015 02:59 AM, Paul Clarke wrote:
> On 12/13/2015 12:17 PM, Shilpasri G Bhat wrote:
>> Replace the throttling event console messages to perf trace event
>> "power:powernv_throttle" and throttle counter stats which are
>> exported in sysfs. The newly added sysfs files are as follows:
>>
>> 1)/sys/devices/system/node/node0/throttle_frequencies
>>    This gives the throttle stats for each of the available frequencies.
>>    The throttle stat of a frequency is the total number of times the max
>>    frequency was reduced to that frequency.
>>    # cat /sys/devices/system/node/node0/throttle_frequencies
>>    4023000 0
>>    3990000 0
>>    3956000 1
>>    3923000 0
>>    3890000 0
>>    3857000 2
>>    3823000 0
>>    3790000 0
>>    3757000 2
>>    3724000 1
>>    3690000 1
>>    ...
> 
> Is this data useful?  It seems like "elapsed time" at each frequency might be
> more useful, if any.
> 

Yes elapsed time is more useful data here. But the concern here is with the
accuracy of measurement/observation of elapsed time by the kernel. OCC can
throttle/unthrottle the frequency at the granularity of 250us. Although OCC
updates the throttle status to HOMER region immediately there may be a delay in
propagating this message by the opal-poller to the driver.

So instead we might want OCC to give us the throttled elapsed time stat for each
frequency and opal-poller/driver can take the snapshot of this info every n seconds.

>> 2)/sys/devices/system/node/node0/throttle_reasons
>>    This gives the stats for each of the supported throttle reasons.
>>    This gives the total number of times the frequency was throttled due
>>    to each of the reasons.
>>    # cat /sys/devices/system/node/node0/throttle_reasons
>>    No throttling 7
>>    Power Cap 0
>>    Processor Over Temperature 7
>>    Power Supply Failure 0
>>    Over Current 0
>>    OCC Reset 0
>>
>> 3)/sys/devices/system/node/node0/throttle_stat
>>    This gives the total number of throttle events occurred in turbo
>>    range of frequencies and non-turbo(below nominal) range of
>>    frequencies.
> 
> non-turbo should read "at or below nominal".  Maybe "sub-turbo" is a better term(?)
> 
>>    # cat /sys/devices/system/node/node0/throttle_stat
>>    Turbo 7
>>    Nominal 0
> 
> Should this read "Non-turbo" or "Sub-turbo" instead of "Nominal", since the
> events could well occur when already operating below nominal.
> 

Agree. Applied 'sub-turbo' in v2

>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>>   drivers/cpufreq/powernv-cpufreq.c | 186 +++++++++++++++++++++++++++++---------
>>   include/trace/events/power.h      |  22 +++++
>>   2 files changed, 166 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c
>> b/drivers/cpufreq/powernv-cpufreq.c
>> index cb50138..bdde9d6 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -28,6 +28,9 @@
>>   #include <linux/of.h>
>>   #include <linux/reboot.h>
>>   #include <linux/slab.h>
>> +#include <trace/events/power.h>
>> +#include <linux/device.h>
>> +#include <linux/node.h>
>>
>>   #include <asm/cputhreads.h>
>>   #include <asm/firmware.h>
>> @@ -43,12 +46,27 @@
>>   static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
>>   static bool rebooting, throttled, occ_reset;
>>
>> +static char throttle_reason[][30] = {
>> +                    "No throttling",
>> +                    "Power Cap",
>> +                    "Processor Over Temperature",
>> +                    "Power Supply Failure",
>> +                    "Over Current",
>> +                    "OCC Reset"
>> +                     };
> 
> I'm curious if this would be slightly more efficiently implemented as:
> static const char *throttle_reason[] = { ... };
> 
> Do you need 30 characters per string for a reason?
> 
> Regardless, it should be const.

Modified the declaration in v2 version of the patch.

> 
> [...]
> -- 
> PC

Thanks and Regards,
Shilpa