Re: [PATCH] cpufreq: powernv: Use node ID in init_chip_info

[Shilpasri G Bhat]

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 
>> 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([i].mask, cpumask_of_node(chip[i]));
>> +   cpumask_copy([i].mask, cpumask_of_node(node[i]));
>> INIT_WORK([i].throttle, powernv_cpufreq_work_fn);
>> for_each_cpu(cpu, [i].mask)
>> per_cpu(chip_info, cpu) =  [i];
>> }
>>
>> -   return 0;
>> +out:
>> +   kfree(node);
>> +   kfree(chip);
>> +   return rc;
>>  }
>>
>>  static inline void clean_chip_info(void)
>> -- 
>> 2.8.0.rc3.226.g39d4020
> 

Re: [PATCH] cpufreq: powernv: Use node ID in init_chip_info

[Viresh Kumar]

+ 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 
> 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([i].mask, cpumask_of_node(chip[i]));
> +   cpumask_copy([i].mask, cpumask_of_node(node[i]));
> INIT_WORK([i].throttle, powernv_cpufreq_work_fn);
> for_each_cpu(cpu, [i].mask)
> per_cpu(chip_info, cpu) =  [i];
> }
> 
> -   return 0;
> +out:
> +   kfree(node);
> +   kfree(chip);
> +   return rc;
>  }
> 
>  static inline void clean_chip_info(void)
> -- 
> 2.8.0.rc3.226.g39d4020

-- 
viresh

[PATCH] cpufreq: powernv: Use node ID in init_chip_info

[Emily Shaffer]

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 
Change-Id: I22eb626b32fbb8053b3bbb9c75e677c700d0c2fb
---
 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([i].mask, cpumask_of_node(chip[i]));
+   cpumask_copy([i].mask, cpumask_of_node(node[i]));
INIT_WORK([i].throttle, powernv_cpufreq_work_fn);
for_each_cpu(cpu, [i].mask)
per_cpu(chip_info, cpu) =  [i];
}

-   return 0;
+out:
+   kfree(node);
+   kfree(chip);
+   return rc;
 }

 static inline void clean_chip_info(void)
-- 
2.8.0.rc3.226.g39d4020