Re: [PATCH v2] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

[John Hubbard]

On 10/30/19 7:39 PM, Michael Ellerman wrote:
> Hi John,
>
> Sorry I didn't reply to this sooner, too many patches :/
>
> John Hubbard <jhubbard@xxxxxxxxxx> writes:
> > The following build warning occurred on powerpc 64-bit builds:
> >
> > drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
> > drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>
> Oddly I don't see that warning in my builds, eg with GCC9:
>
>    https://travis-ci.org/linuxppc/linux/jobs/604870722

This is with a cross-compiler based on gcc 8.1.0, which I got from:
  https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/

I'll put that in the v3 commit description.

> > This is due to putting 1024 bytes on the stack:
> >
> >      unsigned int chip[256];
> >
> > ...and while looking at this, it also has a bug: it fails with a stack
> > overrun, if CONFIG_NR_CPUS > 256.
>
> It _probably_ doesn't, because it only increments the index when the
> chip_id of the CPU changes, ie. it doesn't create a chip for every CPU.
> But I agree it's flaky the way it's written.

I'll soften up the wording accordingly.

> > Fix both problems by dynamically allocating based on CONFIG_NR_CPUS.
>
> Shouldn't it use num_possible_cpus() ?
>
> Given the for loop is over possible CPUs that seems like the upper
> bound. In practice it should be lower because some CPUs will share a
> chip.

OK, I see, that's more consistent with the code, I'll change to that.


thanks,
--
John Hubbard
NVIDIA

> > Fixes: 053819e0bf840 ("cpufreq: powernv: Handle throttling due to Pmax capping at chip level")
> > Cc: Shilpasri G Bhat <shilpa.bhat@xxxxxxxxxxxxxxxxxx>
> > Cc: Preeti U Murthy <preeti@xxxxxxxxxxxxxxxxxx>
> > Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> > Cc: linux-pm@xxxxxxxxxxxxxxx
> > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> > Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> > ---
> >
> > Changes since v1: includes Viresh's review commit fixes.
> >
> >   drivers/cpufreq/powernv-cpufreq.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> > index 6061850e59c9..5b2e968cb5ea 100644
> > --- a/drivers/cpufreq/powernv-cpufreq.c
> > +++ b/drivers/cpufreq/powernv-cpufreq.c
> > @@ -1041,9 +1041,14 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
> >   
> >   static int init_chip_info(void)
> >   {
> > -	unsigned int chip[256];
> > +	unsigned int *chip;
> >   	unsigned int cpu, i;
> >   	unsigned int prev_chip_id = UINT_MAX;
> > +	int ret = 0;
> > +
> > +	chip = kcalloc(CONFIG_NR_CPUS, sizeof(*chip), GFP_KERNEL);
> > +	if (!chip)
> > +		return -ENOMEM;
> >   
> >   	for_each_possible_cpu(cpu) {
> >   		unsigned int id = cpu_to_chip_id(cpu);
> > @@ -1055,8 +1060,10 @@ static int init_chip_info(void)
> >   	}
> >   
> >   	chips = kcalloc(nr_chips, sizeof(struct chip), GFP_KERNEL);
> > -	if (!chips)
> > -		return -ENOMEM;
> > +	if (!chips) {
> > +		ret = -ENOMEM;
> > +		goto free_and_return;
> > +	}
> >   
> >   	for (i = 0; i < nr_chips; i++) {
> >   		chips[i].id = chip[i];
> > @@ -1066,7 +1073,9 @@ static int init_chip_info(void)
> >   			per_cpu(chip_info, cpu) =  &chips[i];
> >   	}
> >   
> > -	return 0;
> > +free_and_return:
> > +	kfree(chip);
> > +	return ret;
> >   }
> >   
> >   static inline void clean_chip_info(void)
> > --
> > 2.23.0