Re: [PATCH] cpufreq:powernv: Fix init_chip_info initialization in numa=off

[Pratik Sampat]

On 27/07/21 11:46 am, Gautham R Shenoy wrote:
> On Mon, Jul 26, 2021 at 10:37:57PM +0530, Pratik R. Sampat wrote:
> > In the numa=off kernel command-line configuration init_chip_info() loops
> > around the number of chips and attempts to copy the cpumask of that node
> > which is NULL for all iterations after the first chip.
> >
> > Hence, store the cpu mask for each chip instead of derving cpumask from
> > node while populating the "chips" struct array and copy that to the
> > chips[i].mask
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 053819e0bf84 ("cpufreq: powernv: Handle throttling due to Pmax capping at chip level")
> > Signed-off-by: Pratik R. Sampat <psampat@xxxxxxxxxxxxx>
> > Reported-by: Shirisha Ganta <shirisha.ganta1@xxxxxxx>
> > ---
> >   drivers/cpufreq/powernv-cpufreq.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> > index 005600cef273..8ec10d9aed8f 100644
> > --- a/drivers/cpufreq/powernv-cpufreq.c
> > +++ b/drivers/cpufreq/powernv-cpufreq.c
> > @@ -1046,12 +1046,20 @@ static int init_chip_info(void)
> >   	unsigned int *chip;
> >   	unsigned int cpu, i;
> >   	unsigned int prev_chip_id = UINT_MAX;
> > +	cpumask_t *chip_cpu_mask;
> >   	int ret = 0;
> >
> >   	chip = kcalloc(num_possible_cpus(), sizeof(*chip), GFP_KERNEL);
> >   	if (!chip)
> >   		return -ENOMEM;
> >
> > +	/* Allocate a chip cpu mask large enough to fit mask for all chips */
> > +	chip_cpu_mask = kcalloc(32, sizeof(cpumask_t), GFP_KERNEL);
> I suppose by 32 you mean the maximum number of chips possible. You
> could use a #define for that.

ack, I could #define the constant.

> Otherwise, the patch looks good to me.
>
> Reviewed-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>
Thanks
Pratik

> > +	if (!chip_cpu_mask) {
> > +		ret = -ENOMEM;
> > +		goto free_and_return;
> > +	}
> > +
> >   	for_each_possible_cpu(cpu) {
> >   		unsigned int id = cpu_to_chip_id(cpu);
> >
> > @@ -1059,22 +1067,25 @@ static int init_chip_info(void)
> >   			prev_chip_id = id;
> >   			chip[nr_chips++] = id;
> >   		}
> > +		cpumask_set_cpu(cpu, &chip_cpu_mask[nr_chips-1]);
> >   	}
> >
> >   	chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
> >   	if (!chips) {
> >   		ret = -ENOMEM;
> > -		goto free_and_return;
> > +		goto out_chip_cpu_mask;
> >   	}
> >
> >   	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, &chip_cpu_mask[i]);
> >   		INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
> >   		for_each_cpu(cpu, &chips[i].mask)
> >   			per_cpu(chip_info, cpu) =  &chips[i];
> >   	}
> >
> > +out_chip_cpu_mask:
> > +	kfree(chip_cpu_mask);
> >   free_and_return:
> >   	kfree(chip);
> >   	return ret;
> > --
> > 2.31.1