[v8,6/6] cpufreq: powernv: Add sysfs attributes to show throttle stats

Create sysfs attributes to export throttle information in
/sys/devices/system/cpu/cpufreq/chipX. The newly added sysfs files are as
follows:

1)/sys/devices/system/cpu/cpufreq/chipX/throttle_table
This table gives the detailed information on number of times Pmax is
limited to different frequencies due to different throttle reasons.
This table contains all frequencies in rows and all throttle reasons
in columns. Each cell represents the throttle count the Pmax was
limited to the frequency in its row and due to the reason in its
column. The 'Unthrottle' column here gives the count of unthrottling
back to Pmax after the frequency was throttled.
	# cat /sys/devices/system/cpu/cpufreq/chip0/throttle_table
	Frequency	Unthrottle	PowerCap	OverTemp	...
	4322000		0		0		0
	4289000		0		0		0
	4256000		0		0		0
	4222000		0		0		0
	4189000		0		0		0
	4156000		3		0		3
	4123000		4		0		4
	...

2)/sys/devices/system/cpu/cpufreq/chipX/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

3)/sys/devices/system/cpu/cpufreq/chipX/chip-mask
This gives the list of cpus present in the chip.
	# cat /sys/devices/system/cpu/cpufreq/chip0/chip_mask
	0-31

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
Cc: linux-api@vger.kernel.org
---
Changes from v7:
- Replace throttle_frequencies and throttle_reasons/<reason_attributes> 
  sysfs attributes with a 2d table 'throttle_table' which lists the
  all frequencies in rows and throttle reasons in columns.
- Add 'chip_mask' attribute to show the list of cpus in the chip.
- Replace the kobject pointer with the variable in struct chip.
- Add 'pstate' member to struct chip to store last throttled pstate index.
- Fixes in the error-out-paths 'free_*' in init_chip_info() to avoid
  freeing unallocated pointers.
- Explicitly call 'sysfs_remove_group()' while cleaning up before 
  kobject_put()
- Replacements with snprintf(), __ATTR_RO() and container_of()
- Modified commit message and Documentation.

Changes from v6:
- Rename struct chip members 'throt_{nominal/turbo}' to throttle_*
- Rename sysfs throttle_reason attribute 'throttle_reset' to
  'unthrottle_count'
- Add sysfs attribute details in
  Documentation/ABI/testing/sysfs-devices-system-cpu
- Add helper routine get_chip_index_from_kobj() for throttle sysfs
  attribute show() to get chip index from kobject.
- Add the chip id in the pr_warn_once

No changes from v5.

Changes from v4:
- Taken care of Gautham's comments to use inline get_chip_index()

Changes from v3:
- Seperate the patch to contain only the throttle sysfs attribute changes.
- Add helper inline function get_chip_index()

Changes from v2:
- Fixed kbuild test warning.
drivers/cpufreq/powernv-cpufreq.c:609:2: warning: ignoring return
value of 'kstrtoint', declared with attribute warn_unused_result
[-Wunused-result]

Changes from v1:
- Added a kobject to struct chip
- Grouped the throttle reasons under a separate attribute_group and
  exported each reason as individual file.
- Moved the sysfs files from /sys/devices/system/node/nodeN to
  /sys/devices/system/cpu/cpufreq/chipN
- As suggested by Paul Clarke replaced 'Nominal' with 'sub-turbo'.

 Documentation/ABI/testing/sysfs-devices-system-cpu |  66 +++++++
 drivers/cpufreq/powernv-cpufreq.c                  | 197 +++++++++++++++++++--
 2 files changed, 253 insertions(+), 10 deletions(-)

On 03-02-16, 01:11, Shilpasri G Bhat wrote:
>  static int init_chip_info(void)
>  {
>  	unsigned int chip[256];
> -	unsigned int cpu, i;
> +	unsigned int cpu;
>  	unsigned int prev_chip_id = UINT_MAX;
>  	cpumask_t cpu_mask;
> -	int ret = -ENOMEM;
> +	int i, j, ret = -ENOMEM;
>  
>  	core_to_chip_map = kcalloc(cpu_nr_cores(), sizeof(unsigned int),
>  				   GFP_KERNEL);
> @@ -583,12 +711,51 @@ static int init_chip_info(void)
>  		goto free_chip_map;
>  
>  	for (i = 0; i < nr_chips; i++) {
> +		char name[10];
> +
>  		chips[i].id = chip[i];
>  		cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
>  		INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
> +
> +		for (j = 0; j <= OCC_MAX_THROTTLE_STATUS; j++) {
> +			chips[i].reason[j] =
> +			     kcalloc(powernv_pstate_info.nr_pstates,
> +				     sizeof(int), GFP_KERNEL);
> +			if (!chips[i].reason[j]) {
> +				ret = -ENOMEM;
> +				goto free_chip;
> +			}
> +		}
> +
> +		snprintf(name, sizeof(name), "chip%d", chips[i].id);
> +		ret = kobject_init_and_add(&chips[i].kobj,
> +					   get_ktype(cpufreq_global_kobject),

Sorry but why do you need to create a kobject here ? A simple
sysfs_create_group() can create groups (directories) for you.

> +					   cpufreq_global_kobject, name);
> +		if (ret)
> +			goto free_chip;
> +
> +		ret = sysfs_create_group(&chips[i].kobj, &throttle_stat_group);
> +		if (ret) {
> +			pr_info("Chip %d failed to create throttle sysfs group\n",
> +				chips[i].id);
> +			goto free_kobject;
> +		}
>  	}

Hi,

On 02/03/2016 01:57 PM, Viresh Kumar wrote:
> On 03-02-16, 01:11, Shilpasri G Bhat wrote:
>>  static int init_chip_info(void)
>>  {
>>  	unsigned int chip[256];
>> -	unsigned int cpu, i;
>> +	unsigned int cpu;
>>  	unsigned int prev_chip_id = UINT_MAX;
>>  	cpumask_t cpu_mask;
>> -	int ret = -ENOMEM;
>> +	int i, j, ret = -ENOMEM;
>>  
>>  	core_to_chip_map = kcalloc(cpu_nr_cores(), sizeof(unsigned int),
>>  				   GFP_KERNEL);
>> @@ -583,12 +711,51 @@ static int init_chip_info(void)
>>  		goto free_chip_map;
>>  
>>  	for (i = 0; i < nr_chips; i++) {
>> +		char name[10];
>> +
>>  		chips[i].id = chip[i];
>>  		cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
>>  		INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
>> +
>> +		for (j = 0; j <= OCC_MAX_THROTTLE_STATUS; j++) {
>> +			chips[i].reason[j] =
>> +			     kcalloc(powernv_pstate_info.nr_pstates,
>> +				     sizeof(int), GFP_KERNEL);
>> +			if (!chips[i].reason[j]) {
>> +				ret = -ENOMEM;
>> +				goto free_chip;
>> +			}
>> +		}
>> +
>> +		snprintf(name, sizeof(name), "chip%d", chips[i].id);
>> +		ret = kobject_init_and_add(&chips[i].kobj,
>> +					   get_ktype(cpufreq_global_kobject),
> 
> Sorry but why do you need to create a kobject here ? A simple
> sysfs_create_group() can create groups (directories) for you.

I need the chip-id in the <attr>_show(). With just sysfs_create_group() I will
get the cpufreq_global_kobject in the <attr>_show() and I will not be able to
figure out the chip-id.

Thanks and Regards,
Shilpa
> 
>> +					   cpufreq_global_kobject, name);
>> +		if (ret)
>> +			goto free_chip;
>> +
>> +		ret = sysfs_create_group(&chips[i].kobj, &throttle_stat_group);
>> +		if (ret) {
>> +			pr_info("Chip %d failed to create throttle sysfs group\n",
>> +				chips[i].id);
>> +			goto free_kobject;
>> +		}
>>  	}
>

On 03-02-16, 14:12, Shilpasri G Bhat wrote:
> I need the chip-id in the <attr>_show(). With just sysfs_create_group() I will
> get the cpufreq_global_kobject in the <attr>_show() and I will not be able to
> figure out the chip-id.

The more I look at it, the more I am convinced that keeping this
'chip' directory in /sys/devices/system/cpu/cpuX/cpufreq/ makes sense.

So, here is the deal:
- A 'chip' on your platforms can contain multiple group of CPUs, which
  are represented by policies in cpufreq core. i.e. A chip can have
  multiple policies.
- All CPUs present on the same chip are subject to same throttling
  outcomes.
- Right now you are putting the 'chip' directory in cpu/cpufreq/
  directory. Because that directory isn't specific to a policy, but
  entire cpufreq subsystem, you can't get a policy->cpu in the code
  for the kobject in question. And so you are *forced* to create a
  kobject, so that you can do container_of() and get chip->id.
- And then you also need to unnecessarily add another field in the
  chip directory 'chip_mask', that is nothing but an bitwise OR
  operation on policy->related_cpus, so that userspace can know which
  policies/CPUs are managed by the 'chip'.

What I can suggest is:
- Move this directory inside cpuX/cpufreq/ directory, in a similar way
  as to how we create 'stats' directory today.
- You can then get policy->cpu, to get chip->id out of it.
- The only disadvantage here is that the same chip directory will be
  replicated in multiple policies, but that makes it more readable.

Thoughts ?

Hi Viresh,

> 
> What I can suggest is:
> - Move this directory inside cpuX/cpufreq/ directory, in a similar way
>   as to how we create 'stats' directory today.
> - You can then get policy->cpu, to get chip->id out of it.
> - The only disadvantage here is that the same chip directory will be
>   replicated in multiple policies, but that makes it more readable.

Thinking about it, having a sysfs group attached to a policy kobject
looks ok if replication of the same chip information across multiple
policies is not objectionable.

Regarding the table-format, it breaks the sysfs's one-value-per-file
rule. So I would still prefer each throttle reason being a separate
file which gives the number of times the chip frequency was throttled
due to that reason. We can live without the per-frequency
throttle stats listed in the throttle_status.

So, would the following be sysfs group structure be acceptable?

$ls -1 /sys/devices/system/cpu/cpuX/cpufreq/throttle_stats/
unthrottle
powercap
overtemp
supply_fault
overcurrent
occ_reset
turbo_stat
sub_turbo_stat

--
Thanks and Regards
gautham.

On 03-02-16, 17:32, Gautham R Shenoy wrote:
> Regarding the table-format, it breaks the sysfs's one-value-per-file
> rule. So I would still prefer each throttle reason being a separate
> file which gives the number of times the chip frequency was throttled
> due to that reason. We can live without the per-frequency
> throttle stats listed in the throttle_status.
> 
> So, would the following be sysfs group structure be acceptable?
> 
> $ls -1 /sys/devices/system/cpu/cpuX/cpufreq/throttle_stats/
> unthrottle
> powercap
> overtemp
> supply_fault
> overcurrent
> occ_reset
> turbo_stat
> sub_turbo_stat

That was suggested for your convenience only, feel free to keep it the
way you want it.

I forgot about the one-value-per-file thing really, but we are using
that for cpufreq-stats as well.

And now that you have mentioned that to me, why shouldn't this stats
directory be moved to debugfs ? :)

We are never going to perform a store here, isn't it ? And is just for
information, nothing more.

@Rafael: ??

> 
> And now that you have mentioned that to me, why shouldn't this stats
> directory be moved to debugfs ? :)
> 
> We are never going to perform a store here, isn't it ? And is just for
> information, nothing more.
> 

I would very much like to keep the throttle stats either in cpuX/cpufreq or
global cpufreq directory as these are populated by the platform cpufreq driver.

Today we don't have a requirement to a perform a store operation on these files
but we can have it in the future.

Thanks and Regards,
Shilpa

On 03-02-16, 21:54, Shilpasri G Bhat wrote:
> 
> > 
> > And now that you have mentioned that to me, why shouldn't this stats
> > directory be moved to debugfs ? :)
> > 
> > We are never going to perform a store here, isn't it ? And is just for
> > information, nothing more.
> > 
> 
> I would very much like to keep the throttle stats either in cpuX/cpufreq or
> global cpufreq directory as these are populated by the platform cpufreq driver.
> 
> Today we don't have a requirement to a perform a store operation on these files
> but we can have it in the future.

No issues.