Hi,
On 01/23/2016 02:10 PM, Balbir Singh wrote:
> On Fri, 22 Jan 2016 12:49:05 +0530
> Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com> wrote:
>
>> Create sysfs attributes to export throttle information in
>> /sys/devices/system/cpu/cpufreq/chipN. The newly added sysfs files are as
>> follows:
>>
>> 1)/sys/devices/system/cpu/cpufreq/chip0/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 is reduced to that frequency.
>> # cat /sys/devices/system/cpu/cpufreq/chip0/throttle_frequencies
>> 4023000 0
>> 3990000 0
>> 3956000 1
>> 3923000 0
>> 3890000 0
>> 3857000 2
>> 3823000 0
>> 3790000 0
>> 3757000 2
>> 3724000 1
>> 3690000 1
>> ...
>>
>> 2)/sys/devices/system/cpu/cpufreq/chip0/throttle_reasons
>> This directory contains throttle reason files. Each file gives the
>> total number of times the max frequency is throttled, except for
>> 'throttle_reset', which gives the total number of times the max
>
> Is reset a good name? Ideally a reset, reset's stats.
Is unthrottle_count a better name?
>
>> frequency is unthrottled after being throttled.
>> # cd /sys/devices/system/cpu/cpufreq/chip0/throttle_reasons
>> # cat cpu_over_temperature
>> 7
>> # cat occ_reset
>> 0
>> # cat over_current
>> 0
>> # cat power_cap
>> 0
>> # cat power_supply_failure
>> 0
>> # cat throttle_reset
>> 7
>>
>> 3)/sys/devices/system/cpu/cpufreq/chip0/throttle_stat
>> This gives the total number of events of max frequency throttling to
>> lower frequencies in the turbo range of frequencies and the sub-turbo(at
>> and below nominal) range of frequencies.
>> # cat /sys/devices/system/cpu/cpufreq/chip0/throttle_stat
>> turbo 7
>> sub-turbo 0
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.b...@linux.vnet.ibm.com>
>> Reviewed-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com>
> snip
>
>>
>> +enum throt_reason_type {
>> + NO_THROTTLE = 0,
> NO is not throttled or number_throttle?
It stands for not throttled.
>
>> + POWERCAP,
>> + CPU_OVERTEMP,
>> + POWER_SUPPLY_FAILURE,
>> + OVERCURRENT,
>> + OCC_RESET_THROTTLE,
>> + OCC_MAX_REASON
>> +};
>> +
>> static struct chip {
>> unsigned int id;
>> bool throttled;
>> @@ -62,6 +72,11 @@ static struct chip {
>> u8 throt_reason;
>> cpumask_t mask;
>> struct work_struct throttle;
>> + int throt_turbo;
>
> The enum uses _THROTTLE so why can't the struct member
> be throttle_nominal? throttle_turbo?
Okay will change the struct members to throttle_*
>
>> + int throt_nominal;
>> + int reason[OCC_MAX_REASON];
>> + int *pstate_stat;
>> + struct kobject *kobj;
>> } *chips;
>>
>> static int nr_chips;
>> @@ -196,6 +211,128 @@ static struct freq_attr *powernv_cpu_freq_attr[] = {
>> NULL,
>> };
>>
>> +static inline int get_chip_index(unsigned int id)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < nr_chips; i++)
>> + if (chips[i].id == id)
>> + return i;
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static ssize_t throttle_freq_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + int i, count = 0, id;
>> +
>> + i = kstrtoint(kobj->name + 4, 0, &id);
>
> Why the +4 magic, make it more readable?
Okay. Will do.
>
>> + if (i)
>> + return i;
>> +
>> + id = get_chip_index(id);
>> + if (id < 0) {
>> + pr_warn_once("%s Matching chip-id not found\n", __func__);
>
> The pr_warn_once should also print which chip-id was not found, please
> add that to the print
Okay will do.
>
>> + return id;
>> + }
>> +
>> + for (i = 0; i < powernv_pstate_info.nr_pstates; i++)
>> + count += sprintf(&buf[count], "%d %d\n",
>> + powernv_freqs[i].frequency,
>> + chips[id].pstate_stat[i]);
>> +
>> + return count;
>> +}
>> +
>> +static struct kobj_attribute attr_throttle_frequencies =
>> +__ATTR(throttle_frequencies, 0444, throttle_freq_show, NULL);
>> +
>> +static ssize_t throttle_stat_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + int ret, id, count = 0;
>> +
>> + ret = kstrtoint(kobj->name + 4, 0, &id);
>> + if (ret)
>> + return ret;
>> +
>> + id = get_chip_index(id);
>> + if (id < 0) {
>> + pr_warn_once("%s Matching chip-id not found\n", __func__);
>> + return id;
>> + }
>> +
>
> The above 9 lines look like common code, you can easily collapse it
> instead of repeating it
okay will do.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
