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

[Paul Clarke <pc@us.ibm.com>]

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.

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

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

[...]
--
PC