Messages in this thread | | | Subject | Re: [PATCH v2] cpufreq: powernv: fix stack bloat and NR_CPUS limitation | From | John Hubbard <> | Date | Wed, 30 Oct 2019 22:17:19 -0700 |
| |
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@nvidia.com> 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@linux.vnet.ibm.com> >> Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: linux-pm@vger.kernel.org >> Cc: linuxppc-dev@lists.ozlabs.org >> Signed-off-by: John Hubbard <jhubbard@nvidia.com> >> --- >> >> 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
| |