Message ID | 20210425073451.2557394-1-ray.huang@amd.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | [v4] x86, sched: Fix the AMD CPPC maximum perf on some specific generations | expand |
On Sun, Apr 25, 2021 at 9:35 AM Huang Rui <ray.huang@amd.com> wrote: > > Some AMD Ryzen generations has different calculation method on maximum > perf. 255 is not for all asics, some specific generations should use 166 > as the maximum perf. Otherwise, it will report incorrect frequency value > like below: > > ~ → lscpu | grep MHz > CPU MHz: 3400.000 > CPU max MHz: 7228.3198 > CPU min MHz: 2200.0000 > > Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems") > Fixes: 3c55e94c0ade ("cpufreq: ACPI: Extend frequency tables to cover boost frequencies") > > Reported-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > Tested-by: Jason Bagavatsingham <jason.bagavatsingham@gmail.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=211791 > Signed-off-by: Huang Rui <ray.huang@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Nathan Fontenot <nathan.fontenot@amd.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: x86@kernel.org > Cc: stable@vger.kernel.org > --- > > Changes from V1 -> V2: > - Enhance the commit message. > - Move amd_get_highest_perf() into amd.c. > - Refine the implementation of switch-case. > - Cc stable mail list. > > Changes from V2 -> V3: > - Move the update into cppc_get_perf_caps() to correct the highest perf value in > the API. > > Changes from V3 -> V4: > - Rollback to V2 implementation because acpi_cppc.c will be used by ARM as well. > It's not good to add x86-specific calling there. > - Simplify the implementation of the functions. All of my comments have been addressed, so: Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> and I'm expecting the x86 maintainers to take care of this patch. > --- > arch/x86/include/asm/processor.h | 2 ++ > arch/x86/kernel/cpu/amd.c | 16 ++++++++++++++++ > arch/x86/kernel/smpboot.c | 2 +- > drivers/cpufreq/acpi-cpufreq.c | 6 +++++- > 4 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index f1b9ed5efaa9..908bcaea1361 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -804,8 +804,10 @@ DECLARE_PER_CPU(u64, msr_misc_features_shadow); > > #ifdef CONFIG_CPU_SUP_AMD > extern u32 amd_get_nodes_per_socket(void); > +extern u32 amd_get_highest_perf(void); > #else > static inline u32 amd_get_nodes_per_socket(void) { return 0; } > +static inline u32 amd_get_highest_perf(void) { return 0; } > #endif > > static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves) > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index 347a956f71ca..bc3496669def 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) > break; > } > } > + > +u32 amd_get_highest_perf(void) > +{ > + struct cpuinfo_x86 *c = &boot_cpu_data; > + > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > + return 166; > + > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > + return 166; > + > + return 225; > +} > +EXPORT_SYMBOL_GPL(amd_get_highest_perf); > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 02813a7f3a7c..7bec57d04a87 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -2046,7 +2046,7 @@ static bool amd_set_max_freq_ratio(void) > return false; > } > > - highest_perf = perf_caps.highest_perf; > + highest_perf = amd_get_highest_perf(); > nominal_perf = perf_caps.nominal_perf; > > if (!highest_perf || !nominal_perf) { > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c > index d1bbc16fba4b..7e7450453714 100644 > --- a/drivers/cpufreq/acpi-cpufreq.c > +++ b/drivers/cpufreq/acpi-cpufreq.c > @@ -646,7 +646,11 @@ static u64 get_max_boost_ratio(unsigned int cpu) > return 0; > } > > - highest_perf = perf_caps.highest_perf; > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > + highest_perf = amd_get_highest_perf(); > + else > + highest_perf = perf_caps.highest_perf; > + > nominal_perf = perf_caps.nominal_perf; > > if (!highest_perf || !nominal_perf) { > -- > 2.25.1 >
* Huang Rui <ray.huang@amd.com> wrote: > Some AMD Ryzen generations has different calculation method on maximum > perf. 255 is not for all asics, some specific generations should use 166 > as the maximum perf. Otherwise, it will report incorrect frequency value > like below: > > ~ → lscpu | grep MHz > CPU MHz: 3400.000 > CPU max MHz: 7228.3198 > CPU min MHz: 2200.0000 It would have been useful to also quote the 'after' part. > +u32 amd_get_highest_perf(void) > +{ > + struct cpuinfo_x86 *c = &boot_cpu_data; > + > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > + return 166; > + > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > + return 166; I fixed these stray 4-space tabs. Looks good otherwise - queued up in tip:sched/urgent. Thanks, Ingo
On Sun, 25 Apr 2021, Huang Rui wrote: > Some AMD Ryzen generations has different calculation method on maximum > perf. 255 is not for all asics, some specific generations should use 166 > as the maximum perf. Otherwise, it will report incorrect frequency value > like below: The commit message says '255', but the code: > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) > break; > } > } > + > +u32 amd_get_highest_perf(void) > +{ > + struct cpuinfo_x86 *c = &boot_cpu_data; > + > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > + return 166; > + > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > + return 166; > + > + return 225; > +} says 225? This is probably a typo? In any case they are out of sync. Alexander
* Alexander Monakov <amonakov@ispras.ru> wrote: > On Sun, 25 Apr 2021, Huang Rui wrote: > > > Some AMD Ryzen generations has different calculation method on maximum > > perf. 255 is not for all asics, some specific generations should use 166 > > as the maximum perf. Otherwise, it will report incorrect frequency value > > like below: > > The commit message says '255', but the code: > > > --- a/arch/x86/kernel/cpu/amd.c > > +++ b/arch/x86/kernel/cpu/amd.c > > @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) > > break; > > } > > } > > + > > +u32 amd_get_highest_perf(void) > > +{ > > + struct cpuinfo_x86 *c = &boot_cpu_data; > > + > > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > > + return 166; > > + > > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > > + return 166; > > + > > + return 225; > > +} > > says 225? This is probably a typo? In any case they are out of sync. > > Alexander Ugh - that's indeed a good question ... Thanks, Ingo
On Thu, May 13, 2021 at 06:59:02AM +0800, Ingo Molnar wrote: > > * Alexander Monakov <amonakov@ispras.ru> wrote: > > > On Sun, 25 Apr 2021, Huang Rui wrote: > > > > > Some AMD Ryzen generations has different calculation method on maximum > > > perf. 255 is not for all asics, some specific generations should use 166 > > > as the maximum perf. Otherwise, it will report incorrect frequency value > > > like below: > > > > The commit message says '255', but the code: > > > > > --- a/arch/x86/kernel/cpu/amd.c > > > +++ b/arch/x86/kernel/cpu/amd.c > > > @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) > > > break; > > > } > > > } > > > + > > > +u32 amd_get_highest_perf(void) > > > +{ > > > + struct cpuinfo_x86 *c = &boot_cpu_data; > > > + > > > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > > > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > > > + return 166; > > > + > > > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > > > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > > > + return 166; > > > + > > > + return 225; > > > +} > > > > says 225? This is probably a typo? In any case they are out of sync. > > > > Alexander > > Ugh - that's indeed a good question ... > Ah sorry! It's my typo. It should be 255 (confirmed in the ucode). Alexander, thanks a lot to catch this! Ingo, would you mind to update it from 225 -> 255 while you apply this patch or let me know if you want me to send v5? Thanks, Ray
* Huang Rui <ray.huang@amd.com> wrote: > On Thu, May 13, 2021 at 06:59:02AM +0800, Ingo Molnar wrote: > > > > * Alexander Monakov <amonakov@ispras.ru> wrote: > > > > > On Sun, 25 Apr 2021, Huang Rui wrote: > > > > > > > Some AMD Ryzen generations has different calculation method on maximum > > > > perf. 255 is not for all asics, some specific generations should use 166 > > > > as the maximum perf. Otherwise, it will report incorrect frequency value > > > > like below: > > > > > > The commit message says '255', but the code: > > > > > > > --- a/arch/x86/kernel/cpu/amd.c > > > > +++ b/arch/x86/kernel/cpu/amd.c > > > > @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) > > > > break; > > > > } > > > > } > > > > + > > > > +u32 amd_get_highest_perf(void) > > > > +{ > > > > + struct cpuinfo_x86 *c = &boot_cpu_data; > > > > + > > > > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > > > > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > > > > + return 166; > > > > + > > > > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > > > > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > > > > + return 166; > > > > + > > > > + return 225; > > > > +} > > > > > > says 225? This is probably a typo? In any case they are out of sync. > > > > > > Alexander > > > > Ugh - that's indeed a good question ... > > > > Ah sorry! It's my typo. It should be 255 (confirmed in the ucode). > > Alexander, thanks a lot to catch this! > > Ingo, would you mind to update it from 225 -> 255 while you apply this > patch or let me know if you want me to send v5? No need to send v5, done! I have a system that appears to be affected by this bug: kepler:~> lscpu | grep -i mhz CPU MHz: 4000.000 CPU max MHz: 7140.6250 CPU min MHz: 2200.0000 So I should be able to confirm after a reboot. Thanks, Ingo
On Thu, May 13, 2021 at 06:12:14PM +0800, Ingo Molnar wrote: > > * Huang Rui <ray.huang@amd.com> wrote: > > > On Thu, May 13, 2021 at 06:59:02AM +0800, Ingo Molnar wrote: > > > > > > * Alexander Monakov <amonakov@ispras.ru> wrote: > > > > > > > On Sun, 25 Apr 2021, Huang Rui wrote: > > > > > > > > > Some AMD Ryzen generations has different calculation method on maximum > > > > > perf. 255 is not for all asics, some specific generations should use 166 > > > > > as the maximum perf. Otherwise, it will report incorrect frequency value > > > > > like below: > > > > > > > > The commit message says '255', but the code: > > > > > > > > > --- a/arch/x86/kernel/cpu/amd.c > > > > > +++ b/arch/x86/kernel/cpu/amd.c > > > > > @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) > > > > > break; > > > > > } > > > > > } > > > > > + > > > > > +u32 amd_get_highest_perf(void) > > > > > +{ > > > > > + struct cpuinfo_x86 *c = &boot_cpu_data; > > > > > + > > > > > + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || > > > > > + (c->x86_model >= 0x70 && c->x86_model < 0x80))) > > > > > + return 166; > > > > > + > > > > > + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || > > > > > + (c->x86_model >= 0x40 && c->x86_model < 0x70))) > > > > > + return 166; > > > > > + > > > > > + return 225; > > > > > +} > > > > > > > > says 225? This is probably a typo? In any case they are out of sync. > > > > > > > > Alexander > > > > > > Ugh - that's indeed a good question ... > > > > > > > Ah sorry! It's my typo. It should be 255 (confirmed in the ucode). > > > > Alexander, thanks a lot to catch this! > > > > Ingo, would you mind to update it from 225 -> 255 while you apply this > > patch or let me know if you want me to send v5? > > No need to send v5, done! > > I have a system that appears to be affected by this bug: > > kepler:~> lscpu | grep -i mhz > CPU MHz: 4000.000 > CPU max MHz: 7140.6250 > CPU min MHz: 2200.0000 > > So I should be able to confirm after a reboot. > Thanks! Please feel free to let me know whether it's able to fix your machine. :-) Thanks, Ray
* Ingo Molnar <mingo@kernel.org> wrote: > No need to send v5, done! > > I have a system that appears to be affected by this bug: > > kepler:~> lscpu | grep -i mhz > CPU MHz: 4000.000 > CPU max MHz: 7140.6250 > CPU min MHz: 2200.0000 > > So I should be able to confirm after a reboot. 'CPU max Mhz' seems to be saner now: kepler:~> lscpu | grep -i mhz CPU MHz: 2200.000 CPU max MHz: 4917.9678 CPU min MHz: 2200.0000 Thanks, Ingo
On Thu, May 13, 2021 at 06:39:08PM +0800, Ingo Molnar wrote: > > * Ingo Molnar <mingo@kernel.org> wrote: > > > No need to send v5, done! > > > > I have a system that appears to be affected by this bug: > > > > kepler:~> lscpu | grep -i mhz > > CPU MHz: 4000.000 > > CPU max MHz: 7140.6250 > > CPU min MHz: 2200.0000 > > > > So I should be able to confirm after a reboot. > > 'CPU max Mhz' seems to be saner now: > > kepler:~> lscpu | grep -i mhz > > CPU MHz: 2200.000 > CPU max MHz: 4917.9678 > CPU min MHz: 2200.0000 > Yes, happy to know this :-) Thanks, Ray
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index f1b9ed5efaa9..908bcaea1361 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -804,8 +804,10 @@ DECLARE_PER_CPU(u64, msr_misc_features_shadow); #ifdef CONFIG_CPU_SUP_AMD extern u32 amd_get_nodes_per_socket(void); +extern u32 amd_get_highest_perf(void); #else static inline u32 amd_get_nodes_per_socket(void) { return 0; } +static inline u32 amd_get_highest_perf(void) { return 0; } #endif static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves) diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 347a956f71ca..bc3496669def 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -1170,3 +1170,19 @@ void set_dr_addr_mask(unsigned long mask, int dr) break; } } + +u32 amd_get_highest_perf(void) +{ + struct cpuinfo_x86 *c = &boot_cpu_data; + + if (c->x86 == 0x17 && ((c->x86_model >= 0x30 && c->x86_model < 0x40) || + (c->x86_model >= 0x70 && c->x86_model < 0x80))) + return 166; + + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model < 0x30) || + (c->x86_model >= 0x40 && c->x86_model < 0x70))) + return 166; + + return 225; +} +EXPORT_SYMBOL_GPL(amd_get_highest_perf); diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 02813a7f3a7c..7bec57d04a87 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -2046,7 +2046,7 @@ static bool amd_set_max_freq_ratio(void) return false; } - highest_perf = perf_caps.highest_perf; + highest_perf = amd_get_highest_perf(); nominal_perf = perf_caps.nominal_perf; if (!highest_perf || !nominal_perf) { diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index d1bbc16fba4b..7e7450453714 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -646,7 +646,11 @@ static u64 get_max_boost_ratio(unsigned int cpu) return 0; } - highest_perf = perf_caps.highest_perf; + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) + highest_perf = amd_get_highest_perf(); + else + highest_perf = perf_caps.highest_perf; + nominal_perf = perf_caps.nominal_perf; if (!highest_perf || !nominal_perf) {