[PATCH v8 0/6] cpufreq: powernv: Redesign the presentation of throttle notification and solve bug-fixes in the driver

In POWER8, OCC(On-Chip-Controller) can throttle the frequency of the
CPU when the chip crosses its thermal and power limits. Currently,
powernv-cpufreq driver detects and reports this event as a console
message. Some machines may not sustain the max turbo frequency in all
conditions and can be throttled frequently. This can lead to the
flooding of console with throttle messages. So this patchset aims to
redesign the presentation of this event via sysfs counters and
tracepoints. And it also fixes couple of bugs reported in the driver.

- Patch [1] fixes a memory leak bug
- Patch [2] fixes the cpu hot-plug bug in powernv_cpufreq_work_fn().
- Patch [3] solves a bug in powernv_cpufreq_throttle_check(), which
  calls in to cpu_to_chip_id() in hot path which reads DT every time
  to find the chip id.
- Patches [4] to [6] will add a perf trace point
  "power:powernv_throttle" and sysfs throttle counter stats in
  /sys/devices/system/cpu/cpufreq/chipN.

Changes from v7:
- Changes in patch[6] involves adding a table to represent the
  throtle stats in frequency X reason layout. Detailed version log
  in the patch.

Changes from v6:
- Changes wrt comments from Balbir Singh and Viresh Kumar. Details in
  the version log of the patches.

Changes from v5:
- Fix kbuild error:
drivers/cpufreq/powernv-cpufreq.c:428:2: error: implicit declaration of
function 'get_online_cpus' [-Werror=implicit-function-declaration]

Changes from v4:
- Fix a hot-plug bug in powernv_cpufreq_work_fn()
- Changes wrt Gautham's and Shreyas's comments 

Changes from v3:
- Add a fix to replace cpu_to_chip_id() with simpler PIR shift to 
  obtain the chip id.
- Break patch2 in to two patches separating the tracepoint and sysfs
  attribute changes.

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]
Shilpasri G Bhat (6):
  cpufre: powernv: Free 'chips' on module exit
  cpufreq: powernv: Hot-plug safe the kworker thread
  cpufreq: powernv: Remove cpu_to_chip_id() from hot-path
  cpufreq: powernv/tracing: Add powernv_throttle tracepoint
  cpufreq: powernv: Replace pr_info with trace print for throttle event
  cpufreq: powernv: Add sysfs attributes to show throttle stats

 Documentation/ABI/testing/sysfs-devices-system-cpu |  66 +++++
 drivers/cpufreq/powernv-cpufreq.c                  | 303 +++++++++++++++++----
 include/trace/events/power.h                       |  22 ++
 kernel/trace/power-traces.c                        |   1 +
 4 files changed, 337 insertions(+), 55 deletions(-)

-- 
1.9.3

This will free the dynamically allocated memory of 'chips' on
module exit.

Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Changes from v7:
- Minor typo fix in the commit message

 drivers/cpufreq/powernv-cpufreq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 547890f..53f980b 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -612,6 +612,7 @@ static void __exit powernv_cpufreq_exit(void)
 	unregister_reboot_notifier(&powernv_cpufreq_reboot_nb);
 	opal_message_notifier_unregister(OPAL_MSG_OCC,
 					 &powernv_cpufreq_opal_nb);
+	kfree(chips);
 	cpufreq_unregister_driver(&powernv_cpufreq_driver);
 }
 module_exit(powernv_cpufreq_exit);

-- 
1.9.3

In the kworker_thread powernv_cpufreq_work_fn(), we can end up
sending an IPI to a cpu going offline. This is a rare corner case
which is fixed using {get/put}_online_cpus(). Along with this fix,
this patch adds changes to do oneshot cpumask_{clear/and} operation.

Suggested-by: Shreyas B Prabhu <redacted>
Suggested-by: Gautham R Shenoy <redacted>
Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
No changes from v7.

 drivers/cpufreq/powernv-cpufreq.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 53f980b..a271b0f 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -28,6 +28,7 @@
 #include <linux/of.h>
 #include <linux/reboot.h>
 #include <linux/slab.h>
+#include <linux/cpu.h>
 
 #include <asm/cputhreads.h>
 #include <asm/firmware.h>

@@ -423,18 +424,19 @@ void powernv_cpufreq_work_fn(struct work_struct *work)
 {
 	struct chip *chip = container_of(work, struct chip, throttle);
 	unsigned int cpu;
-	cpumask_var_t mask;
+	cpumask_t mask;
 
-	smp_call_function_any(&chip->mask,
+	get_online_cpus();
+	cpumask_and(&mask, &chip->mask, cpu_online_mask);
+	smp_call_function_any(&mask,
 			      powernv_cpufreq_throttle_check, NULL, 0);
 
 	if (!chip->restore)
-		return;
+		goto out;
 
 	chip->restore = false;
-	cpumask_copy(mask, &chip->mask);
-	for_each_cpu_and(cpu, mask, cpu_online_mask) {
-		int index, tcpu;
+	for_each_cpu(cpu, &mask) {
+		int index;
 		struct cpufreq_policy policy;
 
 		cpufreq_get_policy(&policy, cpu);

@@ -442,9 +444,10 @@ void powernv_cpufreq_work_fn(struct work_struct *work)
 					       policy.cur,
 					       CPUFREQ_RELATION_C, &index);
 		powernv_cpufreq_target_index(&policy, index);
-		for_each_cpu(tcpu, policy.cpus)
-			cpumask_clear_cpu(tcpu, mask);
+		cpumask_andnot(&mask, &mask, policy.cpus);
 	}
+out:
+	put_online_cpus();
 }
 
 static char throttle_reason[][30] = {

-- 
1.9.3

cpu_to_chip_id() does a DT walk through to find out the chip id by
taking a contended device tree lock. This adds an unnecessary overhead
in a hot path. So instead of calling cpu_to_chip_id() everytime cache
the chip ids for all cores in the array 'core_to_chip_map' and use it
in the hotpath.

Reported-by: Anton Blanchard <redacted>
Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
No changes from v7.

 drivers/cpufreq/powernv-cpufreq.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index a271b0f..c670314 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -43,6 +43,7 @@
 
 static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
 static bool rebooting, throttled, occ_reset;
+static unsigned int *core_to_chip_map;
 
 static struct chip {
 	unsigned int id;

@@ -313,13 +314,14 @@ static inline unsigned int get_nominal_index(void)
 static void powernv_cpufreq_throttle_check(void *data)
 {
 	unsigned int cpu = smp_processor_id();
+	unsigned int chip_id = core_to_chip_map[cpu_core_index_of_thread(cpu)];
 	unsigned long pmsr;
 	int pmsr_pmax, i;
 
 	pmsr = get_pmspr(SPRN_PMSR);
 
 	for (i = 0; i < nr_chips; i++)
-		if (chips[i].id == cpu_to_chip_id(cpu))
+		if (chips[i].id == chip_id)
 			break;
 
 	/* Check for Pmax Capping */

@@ -559,19 +561,29 @@ static int init_chip_info(void)
 	unsigned int chip[256];
 	unsigned int cpu, i;
 	unsigned int prev_chip_id = UINT_MAX;
+	cpumask_t cpu_mask;
+	int ret = -ENOMEM;
 
-	for_each_possible_cpu(cpu) {
+	core_to_chip_map = kcalloc(cpu_nr_cores(), sizeof(unsigned int),
+				   GFP_KERNEL);
+	if (!core_to_chip_map)
+		goto out;
+
+	cpumask_copy(&cpu_mask, cpu_possible_mask);
+	for_each_cpu(cpu, &cpu_mask) {
 		unsigned int id = cpu_to_chip_id(cpu);
 
 		if (prev_chip_id != id) {
 			prev_chip_id = id;
 			chip[nr_chips++] = id;
 		}
+		core_to_chip_map[cpu_core_index_of_thread(cpu)] = id;
+		cpumask_andnot(&cpu_mask, &cpu_mask, cpu_sibling_mask(cpu));
 	}
 
 	chips = kmalloc_array(nr_chips, sizeof(struct chip), GFP_KERNEL);
 	if (!chips)
-		return -ENOMEM;
+		goto free_chip_map;
 
 	for (i = 0; i < nr_chips; i++) {
 		chips[i].id = chip[i];

@@ -582,6 +594,10 @@ static int init_chip_info(void)
 	}
 
 	return 0;
+free_chip_map:
+	kfree(core_to_chip_map);
+out:
+	return ret;
 }
 
 static int __init powernv_cpufreq_init(void)

@@ -616,6 +632,7 @@ static void __exit powernv_cpufreq_exit(void)
 	opal_message_notifier_unregister(OPAL_MSG_OCC,
 					 &powernv_cpufreq_opal_nb);
 	kfree(chips);
+	kfree(core_to_chip_map);
 	cpufreq_unregister_driver(&powernv_cpufreq_driver);
 }
 module_exit(powernv_cpufreq_exit);

-- 
1.9.3

This patch adds the powernv_throttle tracepoint to trace the CPU
frequency throttling event, which is used by the powernv-cpufreq
driver in POWER8.

Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
---
No changes from v7.

 include/trace/events/power.h | 22 ++++++++++++++++++++++
 kernel/trace/power-traces.c  |  1 +
 2 files changed, 23 insertions(+)

diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 284244e..19e5030 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h

@@ -38,6 +38,28 @@ DEFINE_EVENT(cpu, cpu_idle,
 	TP_ARGS(state, cpu_id)
 );
 
+TRACE_EVENT(powernv_throttle,
+
+	TP_PROTO(int chip_id, const char *reason, int pmax),
+
+	TP_ARGS(chip_id, reason, pmax),
+
+	TP_STRUCT__entry(
+		__field(int, chip_id)
+		__string(reason, reason)
+		__field(int, pmax)
+	),
+
+	TP_fast_assign(
+		__entry->chip_id = chip_id;
+		__assign_str(reason, reason);
+		__entry->pmax = pmax;
+	),
+
+	TP_printk("Chip %d Pmax %d %s", __entry->chip_id,
+		  __entry->pmax, __get_str(reason))
+);
+
 TRACE_EVENT(pstate_sample,
 
 	TP_PROTO(u32 core_busy,

diff --git a/kernel/trace/power-traces.c b/kernel/trace/power-traces.c
index eb4220a..81b8745 100644
--- a/kernel/trace/power-traces.c
+++ b/kernel/trace/power-traces.c

@@ -15,4 +15,5 @@
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(suspend_resume);
 EXPORT_TRACEPOINT_SYMBOL_GPL(cpu_idle);
+EXPORT_TRACEPOINT_SYMBOL_GPL(powernv_throttle);
 

-- 
1.9.3

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 <redacted>
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(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index b683e8e..84ff57a 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu

@@ -271,3 +271,69 @@ Description:	Parameters for the CPU cache attributes
 			- WriteBack: data is written only to the cache line and
 				     the modified cache line is written to main
 				     memory only when it is replaced
+
+What:		/sys/devices/system/cpu/cpufreq/chipX/
+Date:		Feb 2016
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+		Linux for PowerPC mailing list <linuxppc-dev@ozlabs.org>
+Description:	POWERNV CPUFreq driver's frequency throttle stats directory for
+		the chip
+
+		This directory contains the CPU frequency throttle attributes
+		for the chip. It is named using the hardware chip-id in the
+		format of 'chip<hw-chip-id>'. This directory contains the below
+		set of attributes:
+		- throttle_table
+		- throttle_stats
+		- chip_mask
+
+What:		/sys/devices/system/cpu/cpufreq/chipX/throttle_table
+Date:		Feb 2016
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+		Linux for PowerPC mailing list <linuxppc-dev@ozlabs.org>
+Description:	POWERNV CPUFreq driver's frequency throttle stats table for the
+		chip
+
+		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         0               0               0
+		...
+
+What:		/sys/devices/system/cpu/cpufreq/chipX/throttle_stats
+Date:		Feb 2016
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+		Linux for PowerPC mailing list <linuxppc-dev@ozlabs.org>
+Description:	POWERNV CPUFreq driver's overall frequency throttle stats for
+		the chip
+
+		This attribute gives the total number of events of max
+		frequency throttling to any lower frequency in the turbo (above
+		nominal) 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
+
+What:		/sys/devices/system/cpu/cpufreq/chipX/chip_mask
+Date:		Feb 2016
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+		Linux for PowerPC mailing list <linuxppc-dev@ozlabs.org>
+Description:	POWERNV CPUFreq driver's attribute to show cpu mask of the chip
+
+		This attribute gives the list of cpus present in the chip.
+		# cat /sys/devices/system/cpu/cpufreq/chip0/chip_mask
+		0-31

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 1bbc10a..0f55e7a 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -60,8 +60,13 @@ static struct chip {
 	bool throttled;
 	bool restore;
 	u8 throttle_reason;
+	s8 pstate;
 	cpumask_t mask;
 	struct work_struct throttle;
+	int throttle_turbo;
+	int throttle_nominal;
+	int *reason[OCC_MAX_THROTTLE_STATUS + 1];
+	struct kobject kobj;
 } *chips;
 
 static int nr_chips;

@@ -196,6 +201,111 @@ 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 inline int get_chip_index_from_kobj(struct kobject *kobj)
+{
+	int ret;
+	struct chip *chip;
+
+	chip = container_of(kobj, struct chip, kobj);
+
+	ret = get_chip_index(chip->id);
+	if (ret < 0)
+		pr_warn_once("%s Matching chip-id not found %d\n", __func__,
+			     chip->id);
+	return ret;
+}
+
+static const char * const column_str[] = {
+	"Frequency",
+	"Unthrottle",
+	"PowerCap",
+	"OverTemp",
+	"PowerFault",
+	"OverCurrent",
+	"OCCReset"
+};
+
+static ssize_t throttle_table_show(struct kobject *kobj,
+				   struct kobj_attribute *attr, char *buf)
+{
+	int id, count = 0, i, j;
+
+	id = get_chip_index_from_kobj(kobj);
+	if (id < 0)
+		return id;
+
+	for (i = 0; i < ARRAY_SIZE(column_str); i++)
+		count += sprintf(&buf[count], "%s\t", column_str[i]);
+	count += sprintf(&buf[count], "\n");
+
+	for (i = 0; i < powernv_pstate_info.nr_pstates; i++) {
+		count += sprintf(&buf[count], "%d\t\t",
+				 powernv_freqs[i].frequency);
+		for (j = 0; j <= OCC_MAX_THROTTLE_STATUS; j++)
+			count += sprintf(&buf[count], "%d\t\t",
+					 chips[id].reason[j][i]);
+		count += sprintf(&buf[count], "\n");
+	}
+
+	return count;
+}
+
+static struct kobj_attribute attr_throttle_table = __ATTR_RO(throttle_table);
+
+static ssize_t throttle_stat_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	int id, count = 0;
+
+	id = get_chip_index_from_kobj(kobj);
+	if (id < 0)
+		return id;
+
+	count += sprintf(&buf[count], "turbo %d\n", chips[id].throttle_turbo);
+	count += sprintf(&buf[count], "sub-turbo %d\n",
+					chips[id].throttle_nominal);
+
+	return count;
+}
+
+static struct kobj_attribute attr_throttle_stat = __ATTR_RO(throttle_stat);
+
+static ssize_t chip_mask_show(struct kobject *kobj,
+			      struct kobj_attribute *attr, char *buf)
+{
+	int id;
+
+	id = get_chip_index_from_kobj(kobj);
+	if (id < 0)
+		return id;
+
+	return cpumap_print_to_pagebuf(true, buf, &chips[id].mask);
+}
+
+static struct kobj_attribute attr_chip_mask = __ATTR_RO(chip_mask);
+
+static struct attribute *throttle_stat_attrs[] = {
+	&attr_throttle_stat.attr,
+	&attr_throttle_table.attr,
+	&attr_chip_mask.attr,
+	NULL
+};
+
+static const struct attribute_group throttle_stat_group = {
+	.attrs = throttle_stat_attrs,
+};
+
 /* Helper routines */
 
 /* Access helpers to power mgt SPR */

@@ -327,13 +437,16 @@ static void powernv_cpufreq_throttle_check(void *data)
 	unsigned int cpu = smp_processor_id();
 	unsigned int chip_id = core_to_chip_map[cpu_core_index_of_thread(cpu)];
 	unsigned long pmsr;
-	int pmsr_pmax, i;
+	int pmsr_pmax, i, index;
 
 	pmsr = get_pmspr(SPRN_PMSR);
 
-	for (i = 0; i < nr_chips; i++)
-		if (chips[i].id == chip_id)
-			break;
+	i = get_chip_index(chip_id);
+	if (unlikely(i < 0)) {
+		pr_warn_once("%s Matching chip-id not found %d\n", __func__,
+			     chip_id);
+		return;
+	}
 
 	/* Check for Pmax Capping */
 	pmsr_pmax = (s8)PMSR_MAX(pmsr);

@@ -341,15 +454,27 @@ static void powernv_cpufreq_throttle_check(void *data)
 		if (chips[i].throttled)
 			goto next;
 		chips[i].throttled = true;
-		if (pmsr_pmax < powernv_pstate_info.nominal)
+		if (pmsr_pmax < powernv_pstate_info.nominal) {
 			pr_warn_once("CPU %d on Chip %u has Pmax reduced below nominal frequency (%d < %d)\n",
 				     cpu, chips[i].id, pmsr_pmax,
 				     powernv_pstate_info.nominal);
+			chips[i].throttle_nominal++;
+		} else {
+			chips[i].throttle_turbo++;
+		}
+
+		index  = powernv_pstate_info.max - pmsr_pmax;
+		if (index >= 0 && index < powernv_pstate_info.nr_pstates) {
+			chips[i].reason[chips[i].throttle_reason][index]++;
+			chips[i].pstate = index;
+		}
+
 		trace_powernv_throttle(chips[i].id,
 				      throttle_reason[chips[i].throttle_reason],
 				      pmsr_pmax);
 	} else if (chips[i].throttled) {
 		chips[i].throttled = false;
+		chips[i].reason[chips[i].throttle_reason][chips[i].pstate]++;
 		trace_powernv_throttle(chips[i].id,
 				      throttle_reason[chips[i].throttle_reason],
 				      pmsr_pmax);

@@ -512,9 +637,12 @@ static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
 			return 0;
 		}
 
-		for (i = 0; i < nr_chips; i++)
-			if (chips[i].id == omsg.chip)
-				break;
+		i = get_chip_index(omsg.chip);
+		if (i < 0) {
+			pr_warn_once("%s Matching chip-id not found %d\n",
+				     __func__, (int)omsg.chip);
+			return i;
+		}
 
 		if (omsg.throttle_status >= 0 &&
 		    omsg.throttle_status <= OCC_MAX_THROTTLE_STATUS)

@@ -556,10 +684,10 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
 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),
+					   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;
+		}
 	}
 
 	return 0;
+
+free_kobject:
+	kobject_put(&chips[i].kobj);
+free_chip:
+	while (--j >= 0)
+		kfree(chips[i].reason[j]);
+	while (--i >= 0) {
+		sysfs_remove_group(&chips[i].kobj, &throttle_stat_group);
+		kobject_put(&chips[i].kobj);
+		for (j = 0; j <= OCC_MAX_THROTTLE_STATUS; j++)
+			kfree(chips[i].reason[j]);
+	}
+	kfree(chips);
 free_chip_map:
 	kfree(core_to_chip_map);
 out:

@@ -623,9 +790,19 @@ module_init(powernv_cpufreq_init);
 
 static void __exit powernv_cpufreq_exit(void)
 {
+	int i, j;
+
 	unregister_reboot_notifier(&powernv_cpufreq_reboot_nb);
 	opal_message_notifier_unregister(OPAL_MSG_OCC,
 					 &powernv_cpufreq_opal_nb);
+
+	for (i = 0; i < nr_chips; i++) {
+		sysfs_remove_group(&chips[i].kobj, &throttle_stat_group);
+		kobject_put(&chips[i].kobj);
+		for (j = 0; j <= OCC_MAX_THROTTLE_STATUS; j++)
+			kfree(chips[i].reason[j]);
+	}
+
 	kfree(chips);
 	kfree(core_to_chip_map);
 	cpufreq_unregister_driver(&powernv_cpufreq_driver);

-- 
1.9.3

Currently we use printk message to notify the throttle event. But this
can flood the console if the cpu is throttled frequently. So replace the
printk with the tracepoint to notify the throttle event. And also events
like throttle below nominal frequency and OCC_RESET are reduced to
pr_warn/pr_warn_once as pointed by MFG to not mark them as critical
messages. This patch adds 'throttle_reason' to struct chip to store the
throttle reason.

Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
No changes from v7.

 drivers/cpufreq/powernv-cpufreq.c | 73 ++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 39 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index c670314..1bbc10a 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -29,6 +29,7 @@
 #include <linux/reboot.h>
 #include <linux/slab.h>
 #include <linux/cpu.h>
+#include <trace/events/power.h>
 
 #include <asm/cputhreads.h>
 #include <asm/firmware.h>

@@ -45,12 +46,22 @@ static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
 static bool rebooting, throttled, occ_reset;
 static unsigned int *core_to_chip_map;
 
+static const char * const throttle_reason[] = {
+	"No throttling",
+	"Power Cap",
+	"Processor Over Temperature",
+	"Power Supply Failure",
+	"Over Current",
+	"OCC Reset"
+};
+
 static struct chip {
 	unsigned int id;
 	bool throttled;
+	bool restore;
+	u8 throttle_reason;
 	cpumask_t mask;
 	struct work_struct throttle;
-	bool restore;
 } *chips;
 
 static int nr_chips;

@@ -331,17 +342,17 @@ static void powernv_cpufreq_throttle_check(void *data)
 			goto next;
 		chips[i].throttled = true;
 		if (pmsr_pmax < powernv_pstate_info.nominal)
-			pr_crit("CPU %d on Chip %u has Pmax reduced below nominal frequency (%d < %d)\n",
-				cpu, chips[i].id, pmsr_pmax,
-				powernv_pstate_info.nominal);
-		else
-			pr_info("CPU %d on Chip %u has Pmax reduced below turbo frequency (%d < %d)\n",
-				cpu, chips[i].id, pmsr_pmax,
-				powernv_pstate_info.max);
+			pr_warn_once("CPU %d on Chip %u has Pmax reduced below nominal frequency (%d < %d)\n",
+				     cpu, chips[i].id, pmsr_pmax,
+				     powernv_pstate_info.nominal);
+		trace_powernv_throttle(chips[i].id,
+				      throttle_reason[chips[i].throttle_reason],
+				      pmsr_pmax);
 	} else if (chips[i].throttled) {
 		chips[i].throttled = false;
-		pr_info("CPU %d on Chip %u has Pmax restored to %d\n", cpu,
-			chips[i].id, pmsr_pmax);
+		trace_powernv_throttle(chips[i].id,
+				      throttle_reason[chips[i].throttle_reason],
+				      pmsr_pmax);
 	}
 
 	/* Check if Psafe_mode_active is set in PMSR. */

@@ -359,7 +370,7 @@ next:
 
 	if (throttled) {
 		pr_info("PMSR = %16lx\n", pmsr);
-		pr_crit("CPU Frequency could be throttled\n");
+		pr_warn("CPU Frequency could be throttled\n");
 	}
 }
 

@@ -452,15 +463,6 @@ out:
 	put_online_cpus();
 }
 
-static char throttle_reason[][30] = {
-					"No throttling",
-					"Power Cap",
-					"Processor Over Temperature",
-					"Power Supply Failure",
-					"Over Current",
-					"OCC Reset"
-				     };
-
 static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
 				   unsigned long msg_type, void *_msg)
 {

@@ -486,7 +488,7 @@ static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
 		 */
 		if (!throttled) {
 			throttled = true;
-			pr_crit("CPU frequency is throttled for duration\n");
+			pr_warn("CPU frequency is throttled for duration\n");
 		}
 
 		break;

@@ -510,23 +512,18 @@ static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
 			return 0;
 		}
 
-		if (omsg.throttle_status &&
+		for (i = 0; i < nr_chips; i++)
+			if (chips[i].id == omsg.chip)
+				break;
+
+		if (omsg.throttle_status >= 0 &&
 		    omsg.throttle_status <= OCC_MAX_THROTTLE_STATUS)
-			pr_info("OCC: Chip %u Pmax reduced due to %s\n",
-				(unsigned int)omsg.chip,
-				throttle_reason[omsg.throttle_status]);
-		else if (!omsg.throttle_status)
-			pr_info("OCC: Chip %u %s\n", (unsigned int)omsg.chip,
-				throttle_reason[omsg.throttle_status]);
-		else
-			return 0;
+			chips[i].throttle_reason = omsg.throttle_status;
 
-		for (i = 0; i < nr_chips; i++)
-			if (chips[i].id == omsg.chip) {
-				if (!omsg.throttle_status)
-					chips[i].restore = true;
-				schedule_work(&chips[i].throttle);
-			}
+		if (!omsg.throttle_status)
+			chips[i].restore = true;
+
+		schedule_work(&chips[i].throttle);
 	}
 	return 0;
 }

@@ -581,16 +578,14 @@ static int init_chip_info(void)
 		cpumask_andnot(&cpu_mask, &cpu_mask, cpu_sibling_mask(cpu));
 	}
 
-	chips = kmalloc_array(nr_chips, sizeof(struct chip), GFP_KERNEL);
+	chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
 	if (!chips)
 		goto free_chip_map;
 
 	for (i = 0; i < nr_chips; i++) {
 		chips[i].id = chip[i];
-		chips[i].throttled = false;
 		cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
 		INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
-		chips[i].restore = false;
 	}
 
 	return 0;

-- 
1.9.3

On 03-02-16, 01:11, Shilpasri G Bhat wrote:

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;
+		}
 	}

-- 
viresh

Hi,

On 02/03/2016 01:57 PM, Viresh Kumar wrote:

On 03-02-16, 01:11, Shilpasri G Bhat wrote:

quoted

 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

quoted

+					   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 ?

-- 
viresh

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.

Hi,

On Tue, Feb 2, 2016 at 8:41 PM, Shilpasri G Bhat
[off-list ref] wrote:

In POWER8, OCC(On-Chip-Controller) can throttle the frequency of the
CPU when the chip crosses its thermal and power limits. Currently,
powernv-cpufreq driver detects and reports this event as a console
message. Some machines may not sustain the max turbo frequency in all
conditions and can be throttled frequently. This can lead to the
flooding of console with throttle messages. So this patchset aims to
redesign the presentation of this event via sysfs counters and
tracepoints. And it also fixes couple of bugs reported in the driver.

- Patch [1] fixes a memory leak bug
- Patch [2] fixes the cpu hot-plug bug in powernv_cpufreq_work_fn().
- Patch [3] solves a bug in powernv_cpufreq_throttle_check(), which
  calls in to cpu_to_chip_id() in hot path which reads DT every time
  to find the chip id.
- Patches [4] to [6] will add a perf trace point
  "power:powernv_throttle" and sysfs throttle counter stats in
  /sys/devices/system/cpu/cpufreq/chipN.

Changes from v7:
- Changes in patch[6] involves adding a table to represent the
  throtle stats in frequency X reason layout. Detailed version log
  in the patch.

Changes from v6:
- Changes wrt comments from Balbir Singh and Viresh Kumar. Details in
  the version log of the patches.

Changes from v5:
- Fix kbuild error:
drivers/cpufreq/powernv-cpufreq.c:428:2: error: implicit declaration of
function 'get_online_cpus' [-Werror=implicit-function-declaration]

Changes from v4:
- Fix a hot-plug bug in powernv_cpufreq_work_fn()
- Changes wrt Gautham's and Shreyas's comments

Changes from v3:
- Add a fix to replace cpu_to_chip_id() with simpler PIR shift to
  obtain the chip id.
- Break patch2 in to two patches separating the tracepoint and sysfs
  attribute changes.

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]
Shilpasri G Bhat (6):
  cpufre: powernv: Free 'chips' on module exit
  cpufreq: powernv: Hot-plug safe the kworker thread
  cpufreq: powernv: Remove cpu_to_chip_id() from hot-path
  cpufreq: powernv/tracing: Add powernv_throttle tracepoint
  cpufreq: powernv: Replace pr_info with trace print for throttle event
  cpufreq: powernv: Add sysfs attributes to show throttle stats

It looks like patches [1-5/6] are not objectionable and I can apply
them without the last one if you want me to.

Thanks,
Rafael

On 03-02-16, 14:40, Rafael J. Wysocki wrote:

It looks like patches [1-5/6] are not objectionable and I can apply
them without the last one if you want me to.

Looks fine to me.

-- 
viresh

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: ??

-- 
viresh

Hi Rafael,

On 02/03/2016 07:10 PM, Rafael J. Wysocki wrote:

Hi,

On Tue, Feb 2, 2016 at 8:41 PM, Shilpasri G Bhat
[off-list ref] wrote:

quoted

In POWER8, OCC(On-Chip-Controller) can throttle the frequency of the
CPU when the chip crosses its thermal and power limits. Currently,
powernv-cpufreq driver detects and reports this event as a console
message. Some machines may not sustain the max turbo frequency in all
conditions and can be throttled frequently. This can lead to the
flooding of console with throttle messages. So this patchset aims to
redesign the presentation of this event via sysfs counters and
tracepoints. And it also fixes couple of bugs reported in the driver.

- Patch [1] fixes a memory leak bug
- Patch [2] fixes the cpu hot-plug bug in powernv_cpufreq_work_fn().
- Patch [3] solves a bug in powernv_cpufreq_throttle_check(), which
  calls in to cpu_to_chip_id() in hot path which reads DT every time
  to find the chip id.
- Patches [4] to [6] will add a perf trace point
  "power:powernv_throttle" and sysfs throttle counter stats in
  /sys/devices/system/cpu/cpufreq/chipN.

Changes from v7:
- Changes in patch[6] involves adding a table to represent the
  throtle stats in frequency X reason layout. Detailed version log
  in the patch.

Changes from v6:
- Changes wrt comments from Balbir Singh and Viresh Kumar. Details in
  the version log of the patches.

Changes from v5:
- Fix kbuild error:
drivers/cpufreq/powernv-cpufreq.c:428:2: error: implicit declaration of
function 'get_online_cpus' [-Werror=implicit-function-declaration]

Changes from v4:
- Fix a hot-plug bug in powernv_cpufreq_work_fn()
- Changes wrt Gautham's and Shreyas's comments

Changes from v3:
- Add a fix to replace cpu_to_chip_id() with simpler PIR shift to
  obtain the chip id.
- Break patch2 in to two patches separating the tracepoint and sysfs
  attribute changes.

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]
Shilpasri G Bhat (6):
  cpufre: powernv: Free 'chips' on module exit
  cpufreq: powernv: Hot-plug safe the kworker thread
  cpufreq: powernv: Remove cpu_to_chip_id() from hot-path
  cpufreq: powernv/tracing: Add powernv_throttle tracepoint
  cpufreq: powernv: Replace pr_info with trace print for throttle event
  cpufreq: powernv: Add sysfs attributes to show throttle stats

It looks like patches [1-5/6] are not objectionable and I can apply
them without the last one if you want me to.

Yes please apply Patch[1-5]. Thanks.

Regards,
Shilpa

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:

quoted

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.

-- 
viresh

On Wed, 2016-02-03 at 01:11 +0530, Shilpasri G Bhat wrote:

cpu_to_chip_id() does a DT walk through to find out the chip id by
taking a contended device tree lock. This adds an unnecessary overhead
in a hot path. So instead of calling cpu_to_chip_id() everytime cache
the chip ids for all cores in the array 'core_to_chip_map' and use it
in the hotpath.
=20
Reported-by: Anton Blanchard <redacted>
Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
No changes from v7.

How about this instead?  It removes the linear lookup and seems a lot
less complex.

Mikey

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cp=

ufreq.c
index 547890f..d63d2cb 100644

--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -52,6 +52,7 @@ static struct chip {
 } *chips;

=20
 static int nr_chips;
+static DEFINE_PER_CPU(unsigned int, chip_id);
=20
 /*
  * Note: The set of pstates consists of contiguous integers, the

@@ -317,9 +318,7 @@ static void powernv_cpufreq_throttle_check(void *data)

=20
 	pmsr =3D get_pmspr(SPRN_PMSR);
=20
-	for (i =3D 0; i < nr_chips; i++)
-		if (chips[i].id =3D=3D cpu_to_chip_id(cpu))
-			break;
+	i =3D this_cpu_read(chip_id);
=20
 	/* Check for Pmax Capping */
 	pmsr_pmax =3D (s8)PMSR_MAX(pmsr);

@@ -560,6 +559,7 @@ static int init_chip_info(void)
 	for_each_possible_cpu(cpu) {
 		unsigned int id =3D cpu_to_chip_id(cpu);

=20
+		per_cpu(chip_id, cpu) =3D nr_chips;
 		if (prev_chip_id !=3D id) {
 			prev_chip_id =3D id;
 			chip[nr_chips++] =3D id;

On Fri, 2016-03-18 at 15:04 +1100, Michael Neuling wrote:

On Wed, 2016-02-03 at 01:11 +0530, Shilpasri G Bhat wrote:
=20

quoted

cpu_to_chip_id() does a DT walk through to find out the chip id by
taking a contended device tree lock. This adds an unnecessary
overhead
in a hot path. So instead of calling cpu_to_chip_id() everytime
cache
the chip ids for all cores in the array 'core_to_chip_map' and use
it
in the hotpath.
=20
Reported-by: Anton Blanchard <redacted>
Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
No changes from v7.

=20
How about this instead?  It removes the linear lookup and seems a lot
less complex.

BTW we never init nr_chips before using it.  We also need something
like.

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cp=

ufreq.c
index d63d2cb..c819ed4 100644

--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -556,6 +556,8 @@ static int init_chip_info(void)
 	unsigned int cpu, i;
 	unsigned int prev_chip_id =3D UINT_MAX;

=20
+	nr_chips =3D 0;
+
 	for_each_possible_cpu(cpu) {
 		unsigned int id =3D cpu_to_chip_id(cpu);
=20

On Fri, Mar 18, 2016 at 5:11 AM, Michael Neuling [off-list ref] wrote:

On Fri, 2016-03-18 at 15:04 +1100, Michael Neuling wrote:

quoted

On Wed, 2016-02-03 at 01:11 +0530, Shilpasri G Bhat wrote:

quoted

quoted

cpu_to_chip_id() does a DT walk through to find out the chip id by
taking a contended device tree lock. This adds an unnecessary
overhead
in a hot path. So instead of calling cpu_to_chip_id() everytime
cache
the chip ids for all cores in the array 'core_to_chip_map' and use
it
in the hotpath.

Reported-by: Anton Blanchard <redacted>
Signed-off-by: Shilpasri G Bhat <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
No changes from v7.

How about this instead?  It removes the linear lookup and seems a lot
less complex.

This has gone in already.  Can you please send a patch on top of it?

Including this part too maybe?

Thanks,
Rafael

From: Michael Neuling <redacted>

"cpufreq: powernv: Remove cpu_to_chip_id() from hot-path" introduced
'core_to_chip_map' array to cache the chip-id of all cores. Replace
this with per_cpu variable that stores the pointer to the chip-array.
This removes the linear lookup and provides a neater and simpler
solution.

Signed-off-by: Michael Neuling <redacted>
Tested-by: Shilpasri G Bhat <redacted>
---

- Rebased the patch on top of linux-pm/linux-next
- nr_chips is defined static, so it will be initialized to zero
- Moved the initialization of the per_cpu variable after 'chips' is
  allocated
- Removed 'core_to_chip_map'

 drivers/cpufreq/powernv-cpufreq.c | 50 +++++++++++++--------------------------
 1 file changed, 17 insertions(+), 33 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 50bf120..a00bcc2 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c

@@ -44,7 +44,6 @@
 
 static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
 static bool rebooting, throttled, occ_reset;
-static unsigned int *core_to_chip_map;
 
 static const char * const throttle_reason[] = {
 	"No throttling",

@@ -65,6 +64,7 @@ static struct chip {
 } *chips;
 
 static int nr_chips;
+static DEFINE_PER_CPU(struct chip *, chip_info);
 
 /*
  * Note: The set of pstates consists of contiguous integers, the

@@ -324,34 +324,31 @@ static inline unsigned int get_nominal_index(void)
 
 static void powernv_cpufreq_throttle_check(void *data)
 {
+	struct chip *chip;
 	unsigned int cpu = smp_processor_id();
-	unsigned int chip_id = core_to_chip_map[cpu_core_index_of_thread(cpu)];
 	unsigned long pmsr;
-	int pmsr_pmax, i;
+	int pmsr_pmax;
 
 	pmsr = get_pmspr(SPRN_PMSR);
-
-	for (i = 0; i < nr_chips; i++)
-		if (chips[i].id == chip_id)
-			break;
+	chip = this_cpu_read(chip_info);
 
 	/* Check for Pmax Capping */
 	pmsr_pmax = (s8)PMSR_MAX(pmsr);
 	if (pmsr_pmax != powernv_pstate_info.max) {
-		if (chips[i].throttled)
+		if (chip->throttled)
 			goto next;
-		chips[i].throttled = true;
+		chip->throttled = true;
 		if (pmsr_pmax < powernv_pstate_info.nominal)
 			pr_warn_once("CPU %d on Chip %u has Pmax reduced below nominal frequency (%d < %d)\n",
-				     cpu, chips[i].id, pmsr_pmax,
+				     cpu, chip->id, pmsr_pmax,
 				     powernv_pstate_info.nominal);
-		trace_powernv_throttle(chips[i].id,
-				      throttle_reason[chips[i].throttle_reason],
+		trace_powernv_throttle(chip->id,
+				      throttle_reason[chip->throttle_reason],
 				      pmsr_pmax);
-	} else if (chips[i].throttled) {
-		chips[i].throttled = false;
-		trace_powernv_throttle(chips[i].id,
-				      throttle_reason[chips[i].throttle_reason],
+	} else if (chip->throttled) {
+		chip->throttled = false;
+		trace_powernv_throttle(chip->id,
+				      throttle_reason[chip->throttle_reason],
 				      pmsr_pmax);
 	}
 

@@ -558,47 +555,34 @@ static int init_chip_info(void)
 	unsigned int chip[256];
 	unsigned int cpu, i;
 	unsigned int prev_chip_id = UINT_MAX;
-	cpumask_t cpu_mask;
-	int ret = -ENOMEM;
-
-	core_to_chip_map = kcalloc(cpu_nr_cores(), sizeof(unsigned int),
-				   GFP_KERNEL);
-	if (!core_to_chip_map)
-		goto out;
 
-	cpumask_copy(&cpu_mask, cpu_possible_mask);
-	for_each_cpu(cpu, &cpu_mask) {
+	for_each_possible_cpu(cpu) {
 		unsigned int id = cpu_to_chip_id(cpu);
 
 		if (prev_chip_id != id) {
 			prev_chip_id = id;
 			chip[nr_chips++] = id;
 		}
-		core_to_chip_map[cpu_core_index_of_thread(cpu)] = id;
-		cpumask_andnot(&cpu_mask, &cpu_mask, cpu_sibling_mask(cpu));
 	}
 
 	chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
 	if (!chips)
-		goto free_chip_map;
+		return -ENOMEM;
 
 	for (i = 0; i < nr_chips; i++) {
 		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_each_cpu(cpu, &chips[i].mask)
+			per_cpu(chip_info, cpu) =  &chips[i];
 	}
 
 	return 0;
-free_chip_map:
-	kfree(core_to_chip_map);
-out:
-	return ret;
 }
 
 static inline void clean_chip_info(void)
 {
 	kfree(chips);
-	kfree(core_to_chip_map);
 }
 
 static inline void unregister_all_notifiers(void)

-- 
1.9.3

On Fri, 2016-03-18 at 15:04 +1100, Michael Neuling wrote:

Except it's not a chip_id, so your patch confused me for a good 2mn ...
Call it chip_idx maybe ? ie, index.

Cheers,
Ben.

On Sat, 2016-03-19 at 09:37 +1100, Benjamin Herrenschmidt wrote:

On Fri, 2016-03-18 at 15:04 +1100, Michael Neuling wrote:

quoted

=20
 static int nr_chips;
+static DEFINE_PER_CPU(unsigned int, chip_id);
=20
 /*
  * Note: The set of pstates consists of contiguous integers, the

@@ -317,9 +318,7 @@ static void powernv_cpufreq_throttle_check(void

*data)
=20
        pmsr =3D get_pmspr(SPRN_PMSR);
=20
-       for (i =3D 0; i < nr_chips; i++)
-               if (chips[i].id =3D=3D cpu_to_chip_id(cpu))
-                       break;
+       i =3D this_cpu_read(chip_id);

=20
Except it's not a chip_id, so your patch confused me for a good 2mn
...
Call it chip_idx maybe ? ie, index.

Yeah, it was a badly named variable but I changed it even more and
Shilpasri rebased it here:

http://patchwork.ozlabs.org/patch/599523/

Mikey

On 18-03-16, 20:28, Shilpasri G Bhat wrote:

From: Michael Neuling <redacted>

"cpufreq: powernv: Remove cpu_to_chip_id() from hot-path" introduced

If the patch is already committed, you should provide its commit id as well.

'core_to_chip_map' array to cache the chip-id of all cores. Replace
this with per_cpu variable that stores the pointer to the chip-array.
This removes the linear lookup and provides a neater and simpler
solution.

Signed-off-by: Michael Neuling <redacted>
Tested-by: Shilpasri G Bhat <redacted>

You are sending this patch and you have updated it as well.. So, you should have
had your signed-off here.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

On Monday, March 21, 2016 12:52:55 PM Viresh Kumar wrote:

On 18-03-16, 20:28, Shilpasri G Bhat wrote:

quoted

From: Michael Neuling <redacted>

"cpufreq: powernv: Remove cpu_to_chip_id() from hot-path" introduced

If the patch is already committed, you should provide its commit id as well.

quoted

'core_to_chip_map' array to cache the chip-id of all cores. Replace
this with per_cpu variable that stores the pointer to the chip-array.
This removes the linear lookup and provides a neater and simpler
solution.

Signed-off-by: Michael Neuling <redacted>
Tested-by: Shilpasri G Bhat <redacted>

You are sending this patch and you have updated it as well.. So, you should have
had your signed-off here.

Right.  Plus if you send a patch from anyone else, you should add your
sigh-off to it anyway, even if it hasn't been modified.

So Shilpasri, please resend with your S-o-b and with the ACK from Viresh.

Thanks,
Rafael