| Message ID | CAJoAoZmfxs8wwy1XsfN9qzLyf-ifQK-qPE4jeBhnKKzjoHSXmA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
+ few IBM guys who have been working on this. On 19-10-16, 15:02, Emily Shaffer wrote: > Fixed assumption that node_id==chip_id in powernv-cpufreq.c:init_chip_info; > explicitly use node ID where necessary. > > Tested: All CPUs report in /sys/devices/system/cpu*/cpufreq/throttle_stats > > Effort: platforms/arch-powerpc > Google-Bug-Id: 26979978 Is this relevant upstream? > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > Change-Id: I22eb626b32fbb8053b3bbb9c75e677c700d0c2fb Gerrit id isn't required for upstream.. > --- > drivers/cpufreq/powernv-cpufreq.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/cpufreq/powernv-cpufreq.c > b/drivers/cpufreq/powernv-cpufreq.c > index d3ffde8..3750b58 100644 > --- a/drivers/cpufreq/powernv-cpufreq.c > +++ b/drivers/cpufreq/powernv-cpufreq.c > @@ -911,32 +911,47 @@ static struct cpufreq_driver powernv_cpufreq_driver = { > > static int init_chip_info(void) > { > - unsigned int chip[256]; > + int rc = 0; > unsigned int cpu, i; > unsigned int prev_chip_id = UINT_MAX; > + unsigned int *chip, *node; > + > + chip = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL); > + node = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL); > + if (!chip || !node) { > + rc = -ENOMEM; > + goto out; > + } > > 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; > + node[nr_chips] = cpu_to_node(cpu); > + chip[nr_chips] = id; > + nr_chips++; > } > } > > chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL); > - if (!chips) > - return -ENOMEM; > + if (!chips) { > + rc = -ENOMEM; > + goto out; > + } > > for (i = 0; i < nr_chips; i++) { > chips[i].id = chip[i]; > - cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i])); > + cpumask_copy(&chips[i].mask, cpumask_of_node(node[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; > +out: > + kfree(node); > + kfree(chip); > + return rc; > } > > static inline void clean_chip_info(void) > -- > 2.8.0.rc3.226.g39d4020
Hi, On 10/20/2016 09:29 AM, Viresh Kumar wrote: > + few IBM guys who have been working on this. > > On 19-10-16, 15:02, Emily Shaffer wrote: >> Fixed assumption that node_id==chip_id in powernv-cpufreq.c:init_chip_info; >> explicitly use node ID where necessary. Thanks for the bug fix. I agree that the node-ids should not be assumed to be always be equal to chip-ids. But I think it would be better to get rid of cpumask_of_node() as it has problems when the powernv-cpufreq driver is initialized with offline cpus, like reported in the post below. https://patchwork.kernel.org/patch/8887591/ (^^ This should also solve the node_id=chip_id problem) Since throttle stats are common for all cpus in the chip, so we are better of not using cpumask_of_node() instead define something like cpumask_of_chip() where the driver doesn't have to compute chip cpumask. Thanks and Regards, Shilpa >> >> Tested: All CPUs report in /sys/devices/system/cpu*/cpufreq/throttle_stats >> >> Effort: platforms/arch-powerpc >> Google-Bug-Id: 26979978 > > Is this relevant upstream? > >> >> Signed-off-by: Emily Shaffer <emilyshaffer@google.com> >> Change-Id: I22eb626b32fbb8053b3bbb9c75e677c700d0c2fb > > Gerrit id isn't required for upstream.. > >> --- >> drivers/cpufreq/powernv-cpufreq.c | 27 +++++++++++++++++++++------ >> 1 file changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c >> b/drivers/cpufreq/powernv-cpufreq.c >> index d3ffde8..3750b58 100644 >> --- a/drivers/cpufreq/powernv-cpufreq.c >> +++ b/drivers/cpufreq/powernv-cpufreq.c >> @@ -911,32 +911,47 @@ static struct cpufreq_driver powernv_cpufreq_driver = { >> >> static int init_chip_info(void) >> { >> - unsigned int chip[256]; >> + int rc = 0; >> unsigned int cpu, i; >> unsigned int prev_chip_id = UINT_MAX; >> + unsigned int *chip, *node; >> + >> + chip = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL); >> + node = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL); >> + if (!chip || !node) { >> + rc = -ENOMEM; >> + goto out; >> + } >> >> 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; >> + node[nr_chips] = cpu_to_node(cpu); >> + chip[nr_chips] = id; >> + nr_chips++; >> } >> } >> >> chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL); >> - if (!chips) >> - return -ENOMEM; >> + if (!chips) { >> + rc = -ENOMEM; >> + goto out; >> + } >> >> for (i = 0; i < nr_chips; i++) { >> chips[i].id = chip[i]; >> - cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i])); >> + cpumask_copy(&chips[i].mask, cpumask_of_node(node[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; >> +out: >> + kfree(node); >> + kfree(chip); >> + return rc; >> } >> >> static inline void clean_chip_info(void) >> -- >> 2.8.0.rc3.226.g39d4020 >
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c index d3ffde8..3750b58 100644 --- a/drivers/cpufreq/powernv-cpufreq.c +++ b/drivers/cpufreq/powernv-cpufreq.c @@ -911,32 +911,47 @@ static struct cpufreq_driver powernv_cpufreq_driver = { static int init_chip_info(void) { - unsigned int chip[256]; + int rc = 0; unsigned int cpu, i; unsigned int prev_chip_id = UINT_MAX; + unsigned int *chip, *node; + + chip = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL); + node = kcalloc(num_possible_cpus(), sizeof(unsigned int), GFP_KERNEL); + if (!chip || !node) { + rc = -ENOMEM; + goto out; + } 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; + node[nr_chips] = cpu_to_node(cpu); + chip[nr_chips] = id; + nr_chips++; } } chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL); - if (!chips) - return -ENOMEM; + if (!chips) { + rc = -ENOMEM; + goto out; + } for (i = 0; i < nr_chips; i++) { chips[i].id = chip[i]; - cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i])); + cpumask_copy(&chips[i].mask, cpumask_of_node(node[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; +out: + kfree(node); + kfree(chip); + return rc; } static inline void clean_chip_info(void)
Fixed assumption that node_id==chip_id in powernv-cpufreq.c:init_chip_info; explicitly use node ID where necessary. Tested: All CPUs report in /sys/devices/system/cpu*/cpufreq/throttle_stats Effort: platforms/arch-powerpc Google-Bug-Id: 26979978 Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Change-Id: I22eb626b32fbb8053b3bbb9c75e677c700d0c2fb --- drivers/cpufreq/powernv-cpufreq.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)