Adjustments for preferred core detection
Subsystems (6)
cover Adjustments for preferred core detection
From: Mario Limonciello <[email protected]>
Preferred core detection is fragile in that any CPU that reports less than 255 for any core is assumed to be a preferred core design. This might not always be true, so it's better to check all CPUs and see that varying values are actually reported.
Furthermore, preferred core detection isn't used by acpi-cpufreq. So incorrect frequencies are used unless amd-pstate is active.
This series moves preferred core detection out of amd-pstate in a more robust fashion. It also removes some tech debt of hardcoded values for platforms that are actually preferred core platforms.
This branch is based off v6.11-rc5.
Mario Limonciello (8):
x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c x86/amd: Rename amd_get_highest_perf() to
amd_get_boost_ratio_numerator()
ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn x86/amd: Move amd_get_highest_perf() out of amd-pstate x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()
cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into
amd_get_boost_ratio_numerator()
cpufreq: amd-pstate: Optimize amd_pstate_update_limits()
cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
arch/x86/include/asm/processor.h | 14 +-- arch/x86/kernel/acpi/cppc.c | 159 ++++++++++++++++++++++++++++++- arch/x86/kernel/cpu/amd.c | 16 ---- drivers/cpufreq/acpi-cpufreq.c | 12 ++- drivers/cpufreq/amd-pstate.c | 132 ++++++------------------- include/acpi/cppc_acpi.h | 10 ++ 6 files changed, 203 insertions(+), 140 deletions(-)
Signature · 1 line
2.43.0
PATCH 1/8 x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c
From: Mario Limonciello <[email protected]>
To prepare to let amd_get_highest_perf() detect preferred cores it will require CPPC functions. Move amd_get_highest_perf() to cppc.c to prepare for 'preferred core detection' rework.
No functional changes intended.
--- arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++++++ arch/x86/kernel/cpu/amd.c | 16 ---------------- 2 files changed, 16 insertions(+), 16 deletions(-)
arch/x86/kernel/acpi/cppc.c +16 -0
@@ -116,3 +116,19 @@ void init_freq_invariance_cppc(void) init_done = true;mutex_unlock (&freq_invariance_lock); } + +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 255 ; +} +EXPORT_SYMBOL_GPL (amd_get_highest_perf);
arch/x86/kernel/cpu/amd.c +0 -16
@@ -1190,22 +1190,6 @@ unsigned long amd_get_dr_addr_mask(unsigned int dr) }EXPORT_SYMBOL_GPL (amd_get_dr_addr_mask); -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 255 ; -} -EXPORT_SYMBOL_GPL (amd_get_highest_perf); -static void zenbleed_check_cpu (void *unused) {struct cpuinfo_x86 *c = &cpu_data (smp_processor_id ());
Signature · 1 line
2.43.0
RE: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 86 lines
-----Original Message-----
From: Mario Limonciello <[email protected]>
Sent: Tuesday, August 27, 2024 5:14 AM
To: Borislav Petkov <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Yuan, Perry <[email protected]>
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; Rafael J . Wysocki <[email protected]>; open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; open list:ACPI <linux- [email protected]>; open list:CPU FREQUENCY SCALING FRAMEWORK <[email protected]>; Limonciello, Mario <[email protected]>
Subject: [PATCH 1/8] x86/amd: Moveamd_get_highest_perf()fromamd.ctocppc.cFrom: Mario Limonciello <[email protected]>
To prepare to let
amd_get_highest_perf()detect preferred cores it will require CPPC functions. Moveamd_get_highest_perf()tocppc.cto prepare for 'preferred core detection' rework.No functional changes intended.
Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++++++ arch/x86/kernel/cpu/amd.c | 16 ---------------- 2 files changed, 16 insertions(+), 16 deletions(-)arch/x86/kernel/acpi/cppc.c index renamed from arch/x86/kernel/acpi/cppc.c +0
-0
ff8f25faca3dd..7ec8f2ce859c8100644arch/x86/kernel/acpi/cppc.c +6
-0@@ -116,3 +116,19 @@ void init_freq_invariance_cppc(void) init_done = true;mutex_unlock (&freq_invariance_lock); } + +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)))arch/x86/kernel/acpi/cppc.c +3
-0+return 166 ; + + if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model <
0x30) ||
+ (c->x86_model>=0x40&&c->x86_model<0x70)))arch/x86/kernel/acpi/cppc.c +5
-0+return 166 ; + +return 255 ; +} +EXPORT_SYMBOL_GPL (amd_get_highest_perf);arch/x86/kernel/cpu/amd.c index renamed from arch/x86/kernel/cpu/amd.c +0
-0
1e0fe5f8ab84e..015971adadfc7100644arch/x86/kernel/cpu/amd.c +0
-5@@ -1190,22 +1190,6 @@ unsigned long amd_get_dr_addr_mask(unsigned int dr) } EXPORT_SYMBOL_GPL(amd_get_dr_addr_mask); -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)))arch/x86/kernel/cpu/amd.c +0
-3-return 166 ; - - if (c->x86 == 0x19 && ((c->x86_model >= 0x20 && c->x86_model <
0x30) ||
- (c->x86_model>=0x40&&c->x86_model<0x70)))arch/x86/kernel/cpu/amd.c +0
-7-return 166 ; - -return 255 ; -} -EXPORT_SYMBOL_GPL (amd_get_highest_perf); -static void zenbleed_check_cpu (void *unused) {struct cpuinfo_x86 *c = &cpu_data (smp_processor_id ()); --
Quoted 2 lines
2.43.0
LGTM, thanks!
Re: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c
In reply to Mario LimoncielloQuoted 9 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
To prepare to let
amd_get_highest_perf()detect preferred cores it will require CPPC functions. Moveamd_get_highest_perf()tocppc.cto prepare for 'preferred core detection' rework.No functional changes intended.
Signed-off-by Mario Limonciello <[email protected]>
--
Thanks and Regards
gautham.
Quoted 59 lines
--- arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++++++ arch/x86/kernel/cpu/amd.c | 16 ---------------- 2 files changed, 16 insertions(+), 16 deletions(-)arch/x86/kernel/acpi/cppc.c +16
-0@@ -116,3 +116,19 @@ void init_freq_invariance_cppc(void) init_done = true;mutex_unlock (&freq_invariance_lock); } + +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 255 ; +} +EXPORT_SYMBOL_GPL (amd_get_highest_perf);arch/x86/kernel/cpu/amd.c +0
-16@@ -1190,22 +1190,6 @@ unsigned long amd_get_dr_addr_mask(unsigned int dr) }EXPORT_SYMBOL_GPL (amd_get_dr_addr_mask); -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 255 ; -} -EXPORT_SYMBOL_GPL (amd_get_highest_perf); -static void zenbleed_check_cpu (void *unused) {struct cpuinfo_x86 *c = &cpu_data (smp_processor_id ());Signature · 1 line
2.43.0
kernel test robot
build botRe: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c
In reply to Mario Limonciellokernel test robot noticed the following build errors:
Hi Mario,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827] [cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240826211358.2694603-2-superm1%40kernel.org patch subject: [PATCH 1/8] x86/amd: Move amd_get_highest_perf() from amd.c to cppc.c config: x86_64-randconfig-074-20240828 (https://download.01.org/0day-ci/archive/20240828/[email protected]/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> arch/x86/kernel/acpi/cppc.c:120:5: error: redefinition of 'amd_get_highest_perf'
120 | u32 amd_get_highest_perf(void)
| ^~~~~~~~~~~~~~~~~~~~
In file included from arch/x86/include/asm/cpufeature.h:5,
from arch/x86/include/asm/thread_info.h:59,
from include/linux/thread_info.h:60,
from include/linux/spinlock.h:60,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from include/acpi/cppc_acpi.h:13,
from arch/x86/kernel/acpi/cppc.c:7:
arch/x86/include/asm/processor.h:708:19: note: previous definition of 'amd_get_highest_perf' with type 'u32(void)' {aka 'unsigned int(void)'}
708 | static inline u32 amd_get_highest_perf(void) { return 0; }
| ^~~~~~~~~~~~~~~~~~~~vim +/amd_get_highest_perf +120 arch/x86/kernel/acpi/cppc.c
119
> 120 u32 amd_get_highest_perf(void)
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
PATCH 2/8 x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
The function name is ambiguous because it returns an intermediate value for calculating maximum frequency rather than the CPPC 'Highest Perf' register.
Rename the function to clarify its use and allow the function to return errors. Adjust the consumer in acpi-cpufreq to catch errors.
--- arch/x86/include/asm/processor.h | 3 --- arch/x86/kernel/acpi/cppc.c | 38 +++++++++++++++++++++++--------- drivers/cpufreq/acpi-cpufreq.c | 12 +++++++--- include/acpi/cppc_acpi.h | 6 +++++ 4 files changed, 43 insertions(+), 16 deletions(-)
arch/x86/include/asm/processor.h +0 -3
@@ -691,8 +691,6 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu) }#ifdef CONFIG_CPU_SUP_AMD -extern u32 amd_get_highest_perf (void ); -/* * Issue a DIV 0/1 insn to clear any division data from previous DIV * operations.
@@ -705,7 +703,6 @@ static __always_inline void amd_clear_divider(void)extern void amd_check_microcode (void );#else -static inline u32 amd_get_highest_perf (void ) {return 0 ; }static inline void amd_clear_divider (void ) { }static inline void amd_check_microcode (void ) { }#endif
arch/x86/kernel/acpi/cppc.c +28 -10
@@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)return ; } - highest_perf =amd_get_highest_perf (); + rc =amd_get_boost_ratio_numerator (0 , &highest_perf); +if (rc) +pr_debug ("Could not retrieve highest performance\n" ); nominal_perf = perf_caps.nominal_perf; -if (!highest_perf || !nominal_perf) { -pr_debug ("Could not retrieve highest or nominal performance\n" ); +if (!nominal_perf) { +pr_debug ("Could not retrieve nominal performance\n" );return ; }
@@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)mutex_unlock (&freq_invariance_lock); } -u32 amd_get_highest_perf (void ) +/** +* amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation +* @cpu: CPU to get numerator for. +* @numerator: Output variable for numerator. +* +* Determine the numerator to use for calculating the boost ratio on +* a CPU. On systems that support preferred cores, this will be a hardcoded +* value. On other systems this will the highest performance register value. +* +* Return: 0 for success, negative error code otherwise. +*/ +int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {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 ; + (c->x86_model >=0x70 && c->x86_model <0x80 ))) { + *numerator =166 ; +return 0 ; + }if (c->x86 ==0x19 && ((c->x86_model >=0x20 && c->x86_model <0x30 ) || - (c->x86_model >=0x40 && c->x86_model <0x70 ))) -return 166 ; + (c->x86_model >=0x40 && c->x86_model <0x70 ))) { + *numerator =166 ; +return 0 ; + } + *numerator =255 ; -return 255 ; +return 0 ; } -EXPORT_SYMBOL_GPL (amd_get_highest_perf); +EXPORT_SYMBOL_GPL (amd_get_boost_ratio_numerator);
drivers/cpufreq/acpi-cpufreq.c +9 -3
@@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)return 0 ; } -if (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) - highest_perf =amd_get_highest_perf (); -else +if (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) { + ret =amd_get_boost_ratio_numerator (cpu, &highest_perf); +if (ret) { +pr_debug ("CPU%d: Unable to get boost ratio numerator (%d)\n" , + cpu, ret); +return 0 ; + } + }else { highest_perf = perf_caps.highest_perf; + } nominal_perf = perf_caps.nominal_perf;
include/acpi/cppc_acpi.h +6 -0
@@ -136,6 +136,12 @@ struct cppc_cpudata {cpumask_var_t shared_cpu_map; }; +#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator); +#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; } +#endif /* !CONFIG_CPU_SUP_AMD */ +#ifdef CONFIG_ACPI_CPPC_LIB extern int cppc_get_desired_perf (int cpunum,u64 *desired_perf);extern int cppc_get_nominal_perf (int cpunum,u64 *nominal_perf);
Signature · 1 line
2.43.0
Re: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()
Hello Mario,
Quoted 10 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The function name is ambiguous because it returns an intermediate value for calculating maximum frequency rather than the CPPC 'Highest Perf' register.
Rename the function to clarify its use and allow the function to return errors. Adjust the consumer in acpi-cpufreq to catch errors.
Signed-off-by Mario Limonciello <[email protected]>
[..snip..]
arch/x86/kernel/acpi/cppc.c static void amd_set_max_freq_ratio(void) · line 82
Quoted 8 lines
arch/x86/kernel/acpi/cppc.c +1
-1@@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)return ; } - highest_perf =amd_get_highest_perf (); + rc =amd_get_boost_ratio_numerator (0 , &highest_perf);
The variable is still named highest_perf, here! I suppose that will change in a subsequent patch?
arch/x86/kernel/acpi/cppc.c static void amd_set_max_freq_ratio(void) · line 84
Quoted 2 lines
arch/x86/kernel/acpi/cppc.c +2
-0+if (rc) +pr_debug ("Could not retrieve highest performance\n" );
I understand that amd_get_boost_ratio_numerator() always returns a 0 in this patch and thus rc == 0, which means we never enter this "if" condition.
However, when rc is non-zero, shouldn't this function return after printing the debug message?
--
Thanks and Regards
gautham.
Quoted 95 lines
arch/x86/kernel/acpi/cppc.c +25
-9nominal_perf = perf_caps.nominal_perf; -if (!highest_perf || !nominal_perf) { -pr_debug ("Could not retrieve highest or nominal performance\n" ); +if (!nominal_perf) { +pr_debug ("Could not retrieve nominal performance\n" );return ; }@@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)mutex_unlock (&freq_invariance_lock); } -u32 amd_get_highest_perf (void ) +/** +* amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation +* @cpu: CPU to get numerator for. +* @numerator: Output variable for numerator. +* +* Determine the numerator to use for calculating the boost ratio on +* a CPU. On systems that support preferred cores, this will be a hardcoded +* value. On other systems this will the highest performance register value. +* +* Return: 0 for success, negative error code otherwise. +*/ +int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {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 ; + (c->x86_model >=0x70 && c->x86_model <0x80 ))) { + *numerator =166 ; +return 0 ; + }if (c->x86 ==0x19 && ((c->x86_model >=0x20 && c->x86_model <0x30 ) || - (c->x86_model >=0x40 && c->x86_model <0x70 ))) -return 166 ; + (c->x86_model >=0x40 && c->x86_model <0x70 ))) { + *numerator =166 ; +return 0 ; + } + *numerator =255 ; -return 255 ; +return 0 ; } -EXPORT_SYMBOL_GPL (amd_get_highest_perf); +EXPORT_SYMBOL_GPL (amd_get_boost_ratio_numerator);drivers/cpufreq/acpi-cpufreq.c +9
-3@@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)return 0 ; } -if (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) - highest_perf =amd_get_highest_perf (); -else +if (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) { + ret =amd_get_boost_ratio_numerator (cpu, &highest_perf); +if (ret) { +pr_debug ("CPU%d: Unable to get boost ratio numerator (%d)\n" , + cpu, ret); +return 0 ; + } + }else { highest_perf = perf_caps.highest_perf; + } nominal_perf = perf_caps.nominal_perf;include/acpi/cppc_acpi.h +6
-0@@ -136,6 +136,12 @@ struct cppc_cpudata {cpumask_var_t shared_cpu_map; }; +#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator); +#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; } +#endif /* !CONFIG_CPU_SUP_AMD */ +#ifdef CONFIG_ACPI_CPPC_LIB extern int cppc_get_desired_perf (int cpunum,u64 *desired_perf);extern int cppc_get_nominal_perf (int cpunum,u64 *nominal_perf);Signature · 1 line
2.43.0
Mario Limonciello
maintainerRe: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()
Quoted 36 lines · Gautham R. Shenoy
Hello Mario,
Quoted 10 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The function name is ambiguous because it returns an intermediate value for calculating maximum frequency rather than the CPPC 'Highest Perf' register.
Rename the function to clarify its use and allow the function to return errors. Adjust the consumer in acpi-cpufreq to catch errors.
Signed-off-by Mario Limonciello <[email protected]>[..snip..]
arch/x86/kernel/acpi/cppc.c static void amd_set_max_freq_ratio(void) · line 82
Quoted 8 lines
arch/x86/kernel/acpi/cppc.c +1
-1@@ -79,11 +79,13 @@ static void amd_set_max_freq_ratio(void)return ; } - highest_perf =amd_get_highest_perf (); + rc =amd_get_boost_ratio_numerator (0 , &highest_perf);The variable is still named
highest_perf, here! I suppose that will change in a subsequent patch?arch/x86/kernel/acpi/cppc.c static void amd_set_max_freq_ratio(void) · line 84
Quoted 2 lines
arch/x86/kernel/acpi/cppc.c +2
-0+if (rc) +pr_debug ("Could not retrieve highest performance\n" );I understand that
amd_get_boost_ratio_numerator()always returns a 0 in this patch and thus rc == 0, which means we never enter this "if" condition.
Quoted 2 lines
However, when rc is non-zero, shouldn't this function return after printing the debug message?
Both good points. Will fix for v2.
Quoted 103 lines
--
Thanks and Regards
gautham.Quoted 95 lines
arch/x86/kernel/acpi/cppc.c +25
-9nominal_perf = perf_caps.nominal_perf; -if (!highest_perf || !nominal_perf) { -pr_debug ("Could not retrieve highest or nominal performance\n" ); +if (!nominal_perf) { +pr_debug ("Could not retrieve nominal performance\n" );return ; }@@ -117,18 +119,34 @@ void init_freq_invariance_cppc(void)mutex_unlock (&freq_invariance_lock); } -u32 amd_get_highest_perf (void ) +/** +* amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation +* @cpu: CPU to get numerator for. +* @numerator: Output variable for numerator. +* +* Determine the numerator to use for calculating the boost ratio on +* a CPU. On systems that support preferred cores, this will be a hardcoded +* value. On other systems this will the highest performance register value. +* +* Return: 0 for success, negative error code otherwise. +*/ +int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {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 ; + (c->x86_model >=0x70 && c->x86_model <0x80 ))) { + *numerator =166 ; +return 0 ; + }if (c->x86 ==0x19 && ((c->x86_model >=0x20 && c->x86_model <0x30 ) || - (c->x86_model >=0x40 && c->x86_model <0x70 ))) -return 166 ; + (c->x86_model >=0x40 && c->x86_model <0x70 ))) { + *numerator =166 ; +return 0 ; + } + *numerator =255 ; -return 255 ; +return 0 ; } -EXPORT_SYMBOL_GPL (amd_get_highest_perf); +EXPORT_SYMBOL_GPL (amd_get_boost_ratio_numerator);drivers/cpufreq/acpi-cpufreq.c +9
-3@@ -642,10 +642,16 @@ static u64 get_max_boost_ratio(unsigned int cpu)return 0 ; } -if (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) - highest_perf =amd_get_highest_perf (); -else +if (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) { + ret =amd_get_boost_ratio_numerator (cpu, &highest_perf); +if (ret) { +pr_debug ("CPU%d: Unable to get boost ratio numerator (%d)\n" , + cpu, ret); +return 0 ; + } + }else { highest_perf = perf_caps.highest_perf; + } nominal_perf = perf_caps.nominal_perf;include/acpi/cppc_acpi.h +6
-0@@ -136,6 +136,12 @@ struct cppc_cpudata {cpumask_var_t shared_cpu_map; }; +#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator); +#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; } +#endif /* !CONFIG_CPU_SUP_AMD */ +#ifdef CONFIG_ACPI_CPPC_LIB extern int cppc_get_desired_perf (int cpunum,u64 *desired_perf);extern int cppc_get_nominal_perf (int cpunum,u64 *nominal_perf);Signature · 1 line
2.43.0
kernel test robot
build botRe: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator()
In reply to Mario Limonciellokernel test robot noticed the following build errors:
Hi Mario,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827] [cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240826211358.2694603-3-superm1%40kernel.org patch subject: [PATCH 2/8] x86/amd: Rename amd_get_highest_perf() to amd_get_boost_ratio_numerator() config: x86_64-randconfig-074-20240828 (https://download.01.org/0day-ci/archive/20240828/[email protected]/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> arch/x86/kernel/acpi/cppc.c:133:5: error: redefinition of 'amd_get_boost_ratio_numerator'
133 | int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/x86/kernel/acpi/cppc.c:7:
include/acpi/cppc_acpi.h:142:19: note: previous definition of 'amd_get_boost_ratio_numerator' with type 'int(unsigned int, u64 *)' {aka 'int(unsigned int, long long unsigned int *)'}
142 | static inline int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) { return -ENODEV; }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~vim +/amd_get_boost_ratio_numerator +133 arch/x86/kernel/acpi/cppc.c
121
122 /**
123 * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
124 * @cpu: CPU to get numerator for.
125 * @numerator: Output variable for numerator.
126 *
127 * Determine the numerator to use for calculating the boost ratio on
128 * a CPU. On systems that support preferred cores, this will be a hardcoded
129 * value. On other systems this will the highest performance register value.
130 *
131 * Return: 0 for success, negative error code otherwise.
132 */
> 133 int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator)
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
PATCH 3/8 ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
If the boost ratio isn't calculated properly for the system for any reason this can cause other problems that are non-obvious.
Raise all messages to warn instead.
--- arch/x86/kernel/acpi/cppc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/kernel/acpi/cppc.c +4 -4
@@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void) rc =cppc_get_perf_caps (0 , &perf_caps);if (rc) { -pr_debug ("Could not retrieve perf counters (%d)\n" , rc); +pr_warn ("Could not retrieve perf counters (%d)\n" , rc);return ; } rc =amd_get_boost_ratio_numerator (0 , &highest_perf);if (rc) -pr_debug ("Could not retrieve highest performance\n" ); +pr_warn ("Could not retrieve highest performance\n" ); nominal_perf = perf_caps.nominal_perf;if (!nominal_perf) { -pr_debug ("Could not retrieve nominal performance\n" ); +pr_warn ("Could not retrieve nominal performance\n" );return ; }
@@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)/* midpoint between max_boost and max_P */ perf_ratio = (perf_ratio +SCHED_CAPACITY_SCALE ) >>1 ;if (!perf_ratio) { -pr_debug ("Non-zero highest/nominal perf values led to a 0 ratio\n" ); +pr_warn ("Non-zero highest/nominal perf values led to a 0 ratio\n" );return ; }
Signature · 1 line
2.43.0
RE: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 63 lines
-----Original Message-----
From: Mario Limonciello <[email protected]>
Sent: Tuesday, August 27, 2024 5:14 AM
To: Borislav Petkov <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Yuan, Perry <[email protected]>
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; Rafael J . Wysocki <[email protected]>; open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; open list:ACPI <linux- [email protected]>; open list:CPU FREQUENCY SCALING FRAMEWORK <[email protected]>; Limonciello, Mario <[email protected]>; Yuan, Perry <[email protected]>
Subject: [PATCH 3/8] ACPI: CPPC: Adjust debug messages inamd_set_max_freq_ratio()to warnFrom: Mario Limonciello <[email protected]>
If the boost ratio isn't calculated properly for the system for any reason this can cause other problems that are non-obvious.
Raise all messages to warn instead.
Suggested-by Perry Yuan <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)arch/x86/kernel/acpi/cppc.c index renamed from arch/x86/kernel/acpi/cppc.c +0
-0
1d631ac5ec328..e94507110ca24100644arch/x86/kernel/acpi/cppc.c +4
-4@@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void) rc =cppc_get_perf_caps (0 , &perf_caps);if (rc) { -pr_debug ("Could not retrieve perf counters (%d)\n" , rc); +pr_warn ("Could not retrieve perf counters (%d)\n" , rc);return ; } rc =amd_get_boost_ratio_numerator (0 , &highest_perf);if (rc) -pr_debug ("Could not retrieve highest performance\n" ); +pr_warn ("Could not retrieve highest performance\n" ); nominal_perf = perf_caps.nominal_perf;if (!nominal_perf) { -pr_debug ("Could not retrieve nominal performance\n" ); +pr_warn ("Could not retrieve nominal performance\n" );return ; }@@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)/* midpoint between max_boost and max_P */ perf_ratio = (perf_ratio +SCHED_CAPACITY_SCALE ) >>1 ;if (!perf_ratio) { -pr_debug ("Non-zero highest/nominal perf values led to a 0 ratio\n"); + pr_warn("Non-zero highest/nominal perf values led to a 0 ratio\n");return ;}
Quoted 3 lines
--
2.43.0
LGTM, it is good to show the warning message once there are some perf values are invalid. That will help to debug the issue from customer report log.
Best Regards.
Perry.
Re: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn
In reply to Mario LimoncielloQuoted 39 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
If the boost ratio isn't calculated properly for the system for any reason this can cause other problems that are non-obvious.
Raise all messages to warn instead.
Suggested-by Perry Yuan <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)arch/x86/kernel/acpi/cppc.c +3
-3@@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void) rc =cppc_get_perf_caps (0 , &perf_caps);if (rc) { -pr_debug ("Could not retrieve perf counters (%d)\n" , rc); +pr_warn ("Could not retrieve perf counters (%d)\n" , rc);return ; } rc =amd_get_boost_ratio_numerator (0 , &highest_perf);if (rc) -pr_debug ("Could not retrieve highest performance\n" ); +pr_warn ("Could not retrieve highest performance\n" ); nominal_perf = perf_caps.nominal_perf;if (!nominal_perf) { -pr_debug ("Could not retrieve nominal performance\n" ); +pr_warn ("Could not retrieve nominal performance\n" );return ; }
arch/x86/kernel/acpi/cppc.c static void amd_set_max_freq_ratio(void) · line 97
Quoted 6 lines
arch/x86/kernel/acpi/cppc.c +1
-1@@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)/* midpoint between max_boost and max_P */ perf_ratio = (perf_ratio +SCHED_CAPACITY_SCALE ) >>1 ;if (!perf_ratio) { -pr_debug ("Non-zero highest/nominal perf values led to a 0 ratio\n" ); +pr_warn ("Non-zero highest/nominal perf values led to a 0 ratio\n" );return ;
Aside:perf_ratio is a u64, and SCHED_CAPACITY_SCALE is (1L << 10). Thus, is it even possible to have !perf_ratio?
Otherwise, I am ok with this promotion of pr_debug to pr_warn.
--
Thanks and Regards
gautham.
Mario Limonciello
maintainerRe: [PATCH 3/8] ACPI: CPPC: Adjust debug messages in amd_set_max_freq_ratio() to warn
Quoted 51 lines · Gautham R. Shenoy
Quoted 39 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
If the boost ratio isn't calculated properly for the system for any reason this can cause other problems that are non-obvious.
Raise all messages to warn instead.
Suggested-by Perry Yuan <[email protected]>Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)arch/x86/kernel/acpi/cppc.c +3
-3@@ -75,17 +75,17 @@ static void amd_set_max_freq_ratio(void) rc =cppc_get_perf_caps (0 , &perf_caps);if (rc) { -pr_debug ("Could not retrieve perf counters (%d)\n" , rc); +pr_warn ("Could not retrieve perf counters (%d)\n" , rc);return ; } rc =amd_get_boost_ratio_numerator (0 , &highest_perf);if (rc) -pr_debug ("Could not retrieve highest performance\n" ); +pr_warn ("Could not retrieve highest performance\n" ); nominal_perf = perf_caps.nominal_perf;if (!nominal_perf) { -pr_debug ("Could not retrieve nominal performance\n" ); +pr_warn ("Could not retrieve nominal performance\n" );return ; }arch/x86/kernel/acpi/cppc.c static void amd_set_max_freq_ratio(void) · line 97
Quoted 6 lines
arch/x86/kernel/acpi/cppc.c +1
-1@@ -93,7 +93,7 @@ static void amd_set_max_freq_ratio(void)/* midpoint between max_boost and max_P */ perf_ratio = (perf_ratio +SCHED_CAPACITY_SCALE ) >>1 ;if (!perf_ratio) { -pr_debug ("Non-zero highest/nominal perf values led to a 0 ratio\n" ); +pr_warn ("Non-zero highest/nominal perf values led to a 0 ratio\n" );return ;Aside:
perf_ratiois a u64, andSCHED_CAPACITY_SCALEis (1L << 10). Thus, is it even possible to have !perf_ratio?
Quoted 1 line
Otherwise, I am ok with this promotion of
pr_debugtopr_warn.
You're right; I don't see this is possible. I'll tear it out in a prerequisite patch in v2.
Quoted 6 lines
Reviewed-by Gautham R. Shenoy <[email protected]>--
Thanks and Regards
gautham.
PATCH 4/8 x86/amd: Move amd_get_highest_perf() out of amd-pstate
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
amd_get_highest_perf() is a helper used to get the highest perf value on AMD systems. It's used in amd-pstate as part of preferred core handling, but applicable for acpi-cpufreq as well.
Move it out to cppc handling code.
--- arch/x86/kernel/acpi/cppc.c | 30 ++++++++++++++++++++++++++++++ drivers/cpufreq/amd-pstate.c | 34 ++-------------------------------- include/acpi/cppc_acpi.h | 2 ++ 3 files changed, 34 insertions(+), 32 deletions(-)
arch/x86/kernel/acpi/cppc.c +30 -0
@@ -119,6 +119,36 @@ void init_freq_invariance_cppc(void)mutex_unlock (&freq_invariance_lock); } +/* +* Get the highest performance register value. +* @cpu: CPU from which to get highest performance. +* @highest_perf: Return address for highest performance value. +* +* Return: 0 for success, negative error code otherwise. +*/ +int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) +{ +u64 val; +int ret; + +if (cpu_feature_enabled (X86_FEATURE_CPPC )) { + ret =rdmsrl_safe_on_cpu (cpu,MSR_AMD_CPPC_CAP1 , &val); +if (ret) + goto out; + + val =AMD_CPPC_HIGHEST_PERF (val); + }else { + ret =cppc_get_highest_perf (cpu, &val); +if (ret) + goto out; + } + +WRITE_ONCE (*highest_perf, (u32 )val); +out: +return ret; +} +EXPORT_SYMBOL_GPL (amd_get_highest_perf); +/** * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation * @cpu: CPU to get numerator for.
drivers/cpufreq/amd-pstate.c +2 -32
@@ -803,36 +803,6 @@ static void amd_pstste_sched_prefcore_workfn(struct work_struct *work) } static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn); -/* -* Get the highest performance register value. -* @cpu: CPU from which to get highest performance. -* @highest_perf: Return address. -* -* Return: 0 for success, -EIO otherwise. -*/ -static int amd_pstate_get_highest_perf (int cpu,u32 *highest_perf) -{ -int ret; - -if (cpu_feature_enabled (X86_FEATURE_CPPC )) { -u64 cap1; - - ret =rdmsrl_safe_on_cpu (cpu,MSR_AMD_CPPC_CAP1 , &cap1); -if (ret) -return ret; -WRITE_ONCE (*highest_perf,AMD_CPPC_HIGHEST_PERF (cap1)); - }else { -u64 cppc_highest_perf; - - ret =cppc_get_highest_perf (cpu, &cppc_highest_perf); -if (ret) -return ret; -WRITE_ONCE (*highest_perf, cppc_highest_perf); - } - -return (ret); -} -#define CPPC_MAX_PERF U8_MAXstatic void amd_pstate_init_prefcore (struct amd_cpudata *cpudata)
@@ -840,7 +810,7 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)int ret, prio;u32 highest_perf; - ret =amd_pstate_get_highest_perf (cpudata->cpu, &highest_perf); + ret =amd_get_highest_perf (cpudata->cpu, &highest_perf);if (ret)return ;
@@ -879,7 +849,7 @@ static void amd_pstate_update_limits(unsigned int cpu)if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) goto free_cpufreq_put; - ret =amd_pstate_get_highest_perf (cpu, &cur_high); + ret =amd_get_highest_perf (cpu, &cur_high);if (ret) goto free_cpufreq_put;
include/acpi/cppc_acpi.h +2 -0
@@ -137,8 +137,10 @@ struct cppc_cpudata { };#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf);extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator);#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) {return -ENODEV ; }static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; }#endif /* !CONFIG_CPU_SUP_AMD */
Signature · 1 line
2.43.0
RE: [PATCH 4/8] x86/amd: Move amd_get_highest_perf() out of amd-pstate
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 149 lines
-----Original Message-----
From: Mario Limonciello <[email protected]>
Sent: Tuesday, August 27, 2024 5:14 AM
To: Borislav Petkov <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Yuan, Perry <[email protected]>
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; Rafael J . Wysocki <[email protected]>; open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; open list:ACPI <linux- [email protected]>; open list:CPU FREQUENCY SCALING FRAMEWORK <[email protected]>; Limonciello, Mario <[email protected]>
Subject: [PATCH 4/8] x86/amd: Moveamd_get_highest_perf()out of amd- pstateFrom: Mario Limonciello <[email protected]>
amd_get_highest_perf()is a helper used to get the highest perf value on AMD systems. It's used in amd-pstate as part of preferred core handling, but applicable for acpi-cpufreq as well.Move it out to cppc handling code.
Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 30 ++++++++++++++++++++++++++++++ drivers/cpufreq/amd-pstate.c | 34 ++-------------------------------- include/acpi/cppc_acpi.h | 2 ++ 3 files changed, 34 insertions(+), 32 deletions(-)arch/x86/kernel/acpi/cppc.c index renamed from arch/x86/kernel/acpi/cppc.c +0
-0
e94507110ca24..5a6c01a1b0d95100644arch/x86/kernel/acpi/cppc.c +29
-0@@ -119,6 +119,36 @@ void init_freq_invariance_cppc(void)mutex_unlock (&freq_invariance_lock); } +/* +* Get the highest performance register value. +* @cpu: CPU from which to get highest performance. +* @highest_perf: Return address for highest performance value. +* +* Return: 0 for success, negative error code otherwise. +*/ +int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) { +u64 val; +int ret; + +if (cpu_feature_enabled (X86_FEATURE_CPPC )) { + ret =rdmsrl_safe_on_cpu (cpu,MSR_AMD_CPPC_CAP1 , &val); +if (ret) + goto out; + + val =AMD_CPPC_HIGHEST_PERF (val); + }else { + ret =cppc_get_highest_perf (cpu, &val); +if (ret) + goto out; + } + +WRITE_ONCE (*highest_perf, (u32 )val); +out: +return ret; +} +EXPORT_SYMBOL_GPL (amd_get_highest_perf); +/** * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
* @cpu: CPU to get numerator for.drivers/cpufreq/amd-pstate.c +0
-0@@ -803,36 +803,6 @@ static void
amd_pstste_sched_prefcore_workfn(struct work_struct *work)} staticDECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);drivers/cpufreq/amd-pstate.c +0
-14-/* -* Get the highest performance register value. -* @cpu: CPU from which to get highest performance. -* @highest_perf: Return address. -* -* Return: 0 for success, -EIO otherwise. -*/ -static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf) -{ -int ret; - -if (cpu_feature_enabled (X86_FEATURE_CPPC )) { -u64 cap1; - - ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1,&cap1);
drivers/cpufreq/amd-pstate.c +0
-3-if (ret) -return ret; - WRITE_ONCE(*highest_perf,
AMD_CPPC_HIGHEST_PERF(cap1));drivers/cpufreq/amd-pstate.c +0
-12- }else { -u64 cppc_highest_perf; - - ret =cppc_get_highest_perf (cpu, &cppc_highest_perf); -if (ret) -return ret; -WRITE_ONCE (*highest_perf, cppc_highest_perf); - } - -return (ret); -} -#define CPPC_MAX_PERF U8_MAX static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata) @@ -840,7 +810,7 @@ static void
amd_pstate_init_prefcore(struct amd_cpudata *cpudata)int ret, prio; u32 highest_perf;drivers/cpufreq/amd-pstate.c +2
-2- ret =amd_pstate_get_highest_perf (cpudata->cpu, &highest_perf); + ret =amd_get_highest_perf (cpudata->cpu, &highest_perf);if (ret)return ;@@ -879,7 +849,7 @@ static void amd_pstate_update_limits(unsigned int cpu)if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) goto free_cpufreq_put; - ret =amd_pstate_get_highest_perf (cpu, &cur_high); + ret =amd_get_highest_perf (cpu, &cur_high);if (ret) goto free_cpufreq_put;include/acpi/cppc_acpi.h index renamed from include/acpi/cppc_acpi.h +0
-0
f25a881cd46dd..2246ce0630362100644include/acpi/cppc_acpi.h +3
-1@@ -137,8 +137,10 @@ struct cppc_cpudata { };#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf);extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator);#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_highest_perf (unsigned int cpu,u32 +*highest_perf) {return -ENODEV ; }static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; }#endif /* !CONFIG_CPU_SUP_AMD */ --
Quoted 2 lines
2.43.0
LGTM, thank you.
Best Regards.
Perry.
Re: [PATCH 4/8] x86/amd: Move amd_get_highest_perf() out of amd-pstate
In reply to Mario LimoncielloQuoted 3 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
amd_get_highest_perf()is a helper used to get the highest perf
Nit: amd_pstate_get_highest_perf() is the existing helper, that is being moved to the cppc handling code and being renamed as amd_get_highest_perf().
Quoted 4 lines
value on AMD systems. It's used in amd-pstate as part of preferred core handling, but applicable for acpi-cpufreq as well.
Move it out to cppc handling code.
Otherwise, looks good to me.
--
Thanks and Regards
gautham.
Quoted 126 lines
Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 30 ++++++++++++++++++++++++++++++ drivers/cpufreq/amd-pstate.c | 34 ++-------------------------------- include/acpi/cppc_acpi.h | 2 ++ 3 files changed, 34 insertions(+), 32 deletions(-)arch/x86/kernel/acpi/cppc.c +30
-0@@ -119,6 +119,36 @@ void init_freq_invariance_cppc(void)mutex_unlock (&freq_invariance_lock); } +/* +* Get the highest performance register value. +* @cpu: CPU from which to get highest performance. +* @highest_perf: Return address for highest performance value. +* +* Return: 0 for success, negative error code otherwise. +*/ +int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) +{ +u64 val; +int ret; + +if (cpu_feature_enabled (X86_FEATURE_CPPC )) { + ret =rdmsrl_safe_on_cpu (cpu,MSR_AMD_CPPC_CAP1 , &val); +if (ret) + goto out; + + val =AMD_CPPC_HIGHEST_PERF (val); + }else { + ret =cppc_get_highest_perf (cpu, &val); +if (ret) + goto out; + } + +WRITE_ONCE (*highest_perf, (u32 )val); +out: +return ret; +} +EXPORT_SYMBOL_GPL (amd_get_highest_perf); +/** * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation * @cpu: CPU to get numerator for. drivers/cpufreq/amd-pstate.c +2
-32@@ -803,36 +803,6 @@ static void amd_pstste_sched_prefcore_workfn(struct work_struct *work) } static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn); -/* -* Get the highest performance register value. -* @cpu: CPU from which to get highest performance. -* @highest_perf: Return address. -* -* Return: 0 for success, -EIO otherwise. -*/ -static int amd_pstate_get_highest_perf (int cpu,u32 *highest_perf) -{ -int ret; - -if (cpu_feature_enabled (X86_FEATURE_CPPC )) { -u64 cap1; - - ret =rdmsrl_safe_on_cpu (cpu,MSR_AMD_CPPC_CAP1 , &cap1); -if (ret) -return ret; -WRITE_ONCE (*highest_perf,AMD_CPPC_HIGHEST_PERF (cap1)); - }else { -u64 cppc_highest_perf; - - ret =cppc_get_highest_perf (cpu, &cppc_highest_perf); -if (ret) -return ret; -WRITE_ONCE (*highest_perf, cppc_highest_perf); - } - -return (ret); -} -#define CPPC_MAX_PERF U8_MAXstatic void amd_pstate_init_prefcore (struct amd_cpudata *cpudata)@@ -840,7 +810,7 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)int ret, prio;u32 highest_perf; - ret =amd_pstate_get_highest_perf (cpudata->cpu, &highest_perf); + ret =amd_get_highest_perf (cpudata->cpu, &highest_perf);if (ret)return ;@@ -879,7 +849,7 @@ static void amd_pstate_update_limits(unsigned int cpu)if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) goto free_cpufreq_put; - ret =amd_pstate_get_highest_perf (cpu, &cur_high); + ret =amd_get_highest_perf (cpu, &cur_high);if (ret) goto free_cpufreq_put;include/acpi/cppc_acpi.h +2
-0@@ -137,8 +137,10 @@ struct cppc_cpudata { };#ifdef CONFIG_CPU_SUP_AMD +extern int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf);extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator);#else /* !CONFIG_CPU_SUP_AMD */ +static inline int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) {return -ENODEV ; }static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; }#endif /* !CONFIG_CPU_SUP_AMD */ Signature · 1 line
2.43.0
PATCH 5/8 x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
AMD systems that support preferred cores will use "166" as their numerator for max frequency calculations instead of "255".
Add a function for detecting preferred cores by looking at the highest perf value on all cores.
If preferred cores are enabled return 166 and if disabled the value in the highest perf register. As the function will be called multiple times, cache the values for the boost numerator and if preferred cores will be enabled in global variables.
--- arch/x86/kernel/acpi/cppc.c | 89 ++++++++++++++++++++++++++++++++---- drivers/cpufreq/amd-pstate.c | 34 ++++++-------- include/acpi/cppc_acpi.h | 2 + 3 files changed, 94 insertions(+), 31 deletions(-)
arch/x86/kernel/acpi/cppc.c +79 -10
@@ -9,6 +9,16 @@#include <asm/processor.h> #include <asm/topology.h> +#define CPPC_HIGHEST_PERF_PREFCORE 166 + +enum amd_pref_core { +AMD_PREF_CORE_UNKNOWN =0 , +AMD_PREF_CORE_SUPPORTED , +AMD_PREF_CORE_UNSUPPORTED , +}; +static enum amd_pref_core amd_pref_core_detected; +static u64 boost_numerator; +/* Refer to drivers/acpi/cppc_acpi.c for the description of functions */ bool cpc_supported_by_cpu (void )
@@ -149,6 +159,66 @@ int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf) }EXPORT_SYMBOL_GPL (amd_get_highest_perf); +/** +* amd_detect_prefcore: Detect if CPUs in the system support preferred cores +* @detected: Output variable for the result of the detection. +* +* Determine whether CPUs in the system support preferred cores. On systems +* that support preferred cores, different highest perf values will be found +* on different cores. On other systems, the highest perf value will be the +* same on all cores. +* +* The result of the detection will be stored in the 'detected' parameter. +* +* Return: 0 for success, negative error code otherwise +*/ +int amd_detect_prefcore (bool *detected) +{ +int cpu, count =0 ; +u64 highest_perf[2 ] = {0 }; + +if (WARN_ON (!detected)) +return -EINVAL ; + +switch (amd_pref_core_detected) { +case AMD_PREF_CORE_SUPPORTED : + *detected = true; +return 0 ; +case AMD_PREF_CORE_UNSUPPORTED : + *detected = false; +return 0 ; +default : +break ; + } + +for_each_present_cpu (cpu) { +u32 tmp; +int ret; + + ret =amd_get_highest_perf (cpu, &tmp); +if (ret) +return ret; + +if (!count || (count ==1 && tmp != highest_perf[0 ])) + highest_perf[count++] = tmp; + +if (count ==2 ) +break ; + } + + *detected = (count ==2 ); + boost_numerator = highest_perf[0 ]; + + amd_pref_core_detected = *detected ?AMD_PREF_CORE_SUPPORTED : +AMD_PREF_CORE_UNSUPPORTED ; + +pr_debug ("AMD CPPC preferred core is %ssupported (highest perf: 0x%llx)\n" , + *detected ?"" :"un" , highest_perf[0 ]); + +return 0 ; +} +EXPORT_SYMBOL_GPL (amd_detect_prefcore); +/** * amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation * @cpu: CPU to get numerator for.
@@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);*/ int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) { -struct cpuinfo_x86 *c = &boot_cpu_data; +bool prefcore; +int ret; -if (c->x86 ==0x17 && ((c->x86_model >=0x30 && c->x86_model <0x40 ) || - (c->x86_model >=0x70 && c->x86_model <0x80 ))) { - *numerator =166 ; -return 0 ; - } + ret =amd_detect_prefcore (&prefcore); +if (ret) +return ret; -if (c->x86 ==0x19 && ((c->x86_model >=0x20 && c->x86_model <0x30 ) || - (c->x86_model >=0x40 && c->x86_model <0x70 ))) { - *numerator =166 ; +/* without preferred cores, return the highest perf register value */ +if (!prefcore) { + *numerator = boost_numerator;return 0 ; } - *numerator =255 ; + *numerator =CPPC_HIGHEST_PERF_PREFCORE ;return 0 ; }
drivers/cpufreq/amd-pstate.c +13 -21
@@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);static void amd_pstate_init_prefcore (struct amd_cpudata *cpudata) { -int ret, prio; -u32 highest_perf; - - ret =amd_get_highest_perf (cpudata->cpu, &highest_perf); -if (ret) +/* user disabled or not detected */ +if (!amd_pstate_prefcore)return ; cpudata->hw_prefcore = true; -/* check if CPPC preferred core feature is enabled*/ -if (highest_perf <CPPC_MAX_PERF ) - prio = (int )highest_perf; -else { -pr_debug ("AMD CPPC preferred core is unsupported!\n" ); - cpudata->hw_prefcore = false; -return ; - } - -if (!amd_pstate_prefcore) -return ;/* * The priorities can be set regardless of whether or not * sched_set_itmt_support(true) has been called and it is valid to * update them at any time after it has been called. */ -sched_set_itmt_core_prio (prio, cpudata->cpu); +sched_set_itmt_core_prio ((int )READ_ONCE (cpudata->highest_perf), cpudata->cpu);schedule_work (&sched_prefcore_work); }
@@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) cpudata->cpu = policy->cpu; -amd_pstate_init_prefcore (cpudata); - ret =amd_pstate_init_perf (cpudata);if (ret) goto free_cpudata1; +amd_pstate_init_prefcore (cpudata); + ret =amd_pstate_init_freq (cpudata);if (ret) goto free_cpudata1;
@@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) cpudata->cpu = policy->cpu; cpudata->epp_policy =0 ; -amd_pstate_init_prefcore (cpudata); - ret =amd_pstate_init_perf (cpudata);if (ret) goto free_cpudata1; +amd_pstate_init_prefcore (cpudata); + ret =amd_pstate_init_freq (cpudata);if (ret) goto free_cpudata1;
@@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)static_call_update (amd_pstate_update_perf, cppc_update_perf); } +if (amd_pstate_prefcore) { + ret =amd_detect_prefcore (&amd_pstate_prefcore); +if (ret) +return ret; + } +/* enable amd pstate feature */ ret =amd_pstate_enable (true);if (ret) {
include/acpi/cppc_acpi.h +2 -0
@@ -137,10 +137,12 @@ struct cppc_cpudata { };#ifdef CONFIG_CPU_SUP_AMD +extern int amd_detect_prefcore (bool *detected);extern int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf);extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator);#else /* !CONFIG_CPU_SUP_AMD */ static inline int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) {return -ENODEV ; } +static inline int amd_detect_prefcore (bool *detected) {return -ENODEV ; }static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; }#endif /* !CONFIG_CPU_SUP_AMD */
Signature · 1 line
2.43.0
Re: [PATCH 5/8] x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()
Quoted 15 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
AMD systems that support preferred cores will use "166" as their numerator for max frequency calculations instead of "255".
Add a function for detecting preferred cores by looking at the highest perf value on all cores.
If preferred cores are enabled return 166 and if disabled the value in the highest perf register. As the function will be called multiple times, cache the values for the boost numerator and if preferred cores will be enabled in global variables.
Signed-off-by Mario Limonciello <[email protected]>---
[..snip..]
Quoted 20 lines
/**
*amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
* @cpu: CPU to get numerator for.arch/x86/kernel/acpi/cppc.c +5
-6@@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);*/ int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) { -struct cpuinfo_x86 *c = &boot_cpu_data; +bool prefcore; +int ret; -if (c->x86 ==0x17 && ((c->x86_model >=0x30 && c->x86_model <0x40 ) || - (c->x86_model >=0x70 && c->x86_model <0x80 ))) { - *numerator =166 ; -return 0 ; - } + ret =amd_detect_prefcore (&prefcore); +if (ret) +return ret;
arch/x86/kernel/acpi/cppc.c EXPORT_SYMBOL_GPL(amd_get_highest_perf); · line 247
Quoted 10 lines
arch/x86/kernel/acpi/cppc.c +4
-4-if (c->x86 ==0x19 && ((c->x86_model >=0x20 && c->x86_model <0x30 ) || - (c->x86_model >=0x40 && c->x86_model <0x70 ))) { - *numerator =166 ; +/* without preferred cores, return the highest perf register value */ +if (!prefcore) { + *numerator = boost_numerator;return 0 ; } - *numerator =255 ; + *numerator =CPPC_HIGHEST_PERF_PREFCORE ;
Interesting. So even when the user boots a system that supports preferred-cores with "amd_preferred=disable", amd_get_boost_ratio_numerator() will return CPPC_HIGHEST_PERF_PREFCORE as the call prefcore == true here.
I suppose that is as intended, since even though the user may not want to use the preferred core logic for task-scheduling/load-balancing, the numerator for the boost-ratio is purely dependent on the h/w capability.
Is this understanding correct? If so, can this be included as a comment in the code ?
The rest of the patch looks good to me.
--
Thanks and Regards
gautham.
Quoted 106 lines
(quoted diff) +0
-0return 0; }drivers/cpufreq/amd-pstate.c +13
-21@@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);static void amd_pstate_init_prefcore (struct amd_cpudata *cpudata) { -int ret, prio; -u32 highest_perf; - - ret =amd_get_highest_perf (cpudata->cpu, &highest_perf); -if (ret) +/* user disabled or not detected */ +if (!amd_pstate_prefcore)return ; cpudata->hw_prefcore = true; -/* check if CPPC preferred core feature is enabled*/ -if (highest_perf <CPPC_MAX_PERF ) - prio = (int )highest_perf; -else { -pr_debug ("AMD CPPC preferred core is unsupported!\n" ); - cpudata->hw_prefcore = false; -return ; - } - -if (!amd_pstate_prefcore) -return ;/* * The priorities can be set regardless of whether or not * sched_set_itmt_support(true) has been called and it is valid to * update them at any time after it has been called. */ -sched_set_itmt_core_prio (prio, cpudata->cpu); +sched_set_itmt_core_prio ((int )READ_ONCE (cpudata->highest_perf), cpudata->cpu);schedule_work (&sched_prefcore_work); }@@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) cpudata->cpu = policy->cpu; -amd_pstate_init_prefcore (cpudata); - ret =amd_pstate_init_perf (cpudata);if (ret) goto free_cpudata1; +amd_pstate_init_prefcore (cpudata); + ret =amd_pstate_init_freq (cpudata);if (ret) goto free_cpudata1;@@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) cpudata->cpu = policy->cpu; cpudata->epp_policy =0 ; -amd_pstate_init_prefcore (cpudata); - ret =amd_pstate_init_perf (cpudata);if (ret) goto free_cpudata1; +amd_pstate_init_prefcore (cpudata); + ret =amd_pstate_init_freq (cpudata);if (ret) goto free_cpudata1;@@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)static_call_update (amd_pstate_update_perf, cppc_update_perf); } +if (amd_pstate_prefcore) { + ret =amd_detect_prefcore (&amd_pstate_prefcore); +if (ret) +return ret; + } +/* enable amd pstate feature */ ret =amd_pstate_enable (true);if (ret) {include/acpi/cppc_acpi.h +2
-0@@ -137,10 +137,12 @@ struct cppc_cpudata { };#ifdef CONFIG_CPU_SUP_AMD +extern int amd_detect_prefcore (bool *detected);extern int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf);extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator);#else /* !CONFIG_CPU_SUP_AMD */ static inline int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) {return -ENODEV ; } +static inline int amd_detect_prefcore (bool *detected) {return -ENODEV ; }static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; }#endif /* !CONFIG_CPU_SUP_AMD */ Signature · 1 line
2.43.0
Mario Limonciello
maintainerRe: [PATCH 5/8] x86/amd: Detect preferred cores in amd_get_boost_ratio_numerator()
Quoted 57 lines · Gautham R. Shenoy
Quoted 15 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
AMD systems that support preferred cores will use "166" as their numerator for max frequency calculations instead of "255".
Add a function for detecting preferred cores by looking at the highest perf value on all cores.
If preferred cores are enabled return 166 and if disabled the value in the highest perf register. As the function will be called multiple times, cache the values for the boost numerator and if preferred cores will be enabled in global variables.
Signed-off-by Mario Limonciello <[email protected]>---
[..snip..]
Quoted 20 lines
/**
*amd_get_boost_ratio_numerator: Get the numerator to use for boost ratio calculation
* @cpu: CPU to get numerator for.arch/x86/kernel/acpi/cppc.c +5
-6@@ -162,20 +232,19 @@ EXPORT_SYMBOL_GPL(amd_get_highest_perf);*/ int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) { -struct cpuinfo_x86 *c = &boot_cpu_data; +bool prefcore; +int ret; -if (c->x86 ==0x17 && ((c->x86_model >=0x30 && c->x86_model <0x40 ) || - (c->x86_model >=0x70 && c->x86_model <0x80 ))) { - *numerator =166 ; -return 0 ; - } + ret =amd_detect_prefcore (&prefcore); +if (ret) +return ret;arch/x86/kernel/acpi/cppc.c EXPORT_SYMBOL_GPL(amd_get_highest_perf); · line 247
Quoted 10 lines
arch/x86/kernel/acpi/cppc.c +4
-4-if (c->x86 ==0x19 && ((c->x86_model >=0x20 && c->x86_model <0x30 ) || - (c->x86_model >=0x40 && c->x86_model <0x70 ))) { - *numerator =166 ; +/* without preferred cores, return the highest perf register value */ +if (!prefcore) { + *numerator = boost_numerator;return 0 ; } - *numerator =255 ; + *numerator =CPPC_HIGHEST_PERF_PREFCORE ;Interesting. So even when the user boots a system that supports preferred-cores with "amd_preferred=disable",
amd_get_boost_ratio_numerator()will returnCPPC_HIGHEST_PERF_PREFCOREas the call prefcore == true here.
Quoted 8 lines
I suppose that is as intended, since even though the user may not want to use the preferred core logic for task-scheduling/load-balancing, the numerator for the boost-ratio is purely dependent on the h/w capability.
Is this understanding correct? If so, can this be included as a comment in the code ?
Yup, you got it all right. I'll fold some of this into the function comment for v2.
Quoted 116 lines
The rest of the patch looks good to me.
Reviewed-by Gautham R. Shenoy <[email protected]>--
Thanks and Regards
gautham.Quoted 106 lines
(quoted diff) +0
-0return 0; }drivers/cpufreq/amd-pstate.c +13
-21@@ -807,32 +807,18 @@ static DECLARE_WORK(sched_prefcore_work, amd_pstste_sched_prefcore_workfn);static void amd_pstate_init_prefcore (struct amd_cpudata *cpudata) { -int ret, prio; -u32 highest_perf; - - ret =amd_get_highest_perf (cpudata->cpu, &highest_perf); -if (ret) +/* user disabled or not detected */ +if (!amd_pstate_prefcore)return ; cpudata->hw_prefcore = true; -/* check if CPPC preferred core feature is enabled*/ -if (highest_perf <CPPC_MAX_PERF ) - prio = (int )highest_perf; -else { -pr_debug ("AMD CPPC preferred core is unsupported!\n" ); - cpudata->hw_prefcore = false; -return ; - } - -if (!amd_pstate_prefcore) -return ;/* * The priorities can be set regardless of whether or not * sched_set_itmt_support(true) has been called and it is valid to * update them at any time after it has been called. */ -sched_set_itmt_core_prio (prio, cpudata->cpu); +sched_set_itmt_core_prio ((int )READ_ONCE (cpudata->highest_perf), cpudata->cpu);schedule_work (&sched_prefcore_work); }@@ -998,12 +984,12 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy) cpudata->cpu = policy->cpu; -amd_pstate_init_prefcore (cpudata); - ret =amd_pstate_init_perf (cpudata);if (ret) goto free_cpudata1; +amd_pstate_init_prefcore (cpudata); + ret =amd_pstate_init_freq (cpudata);if (ret) goto free_cpudata1;@@ -1453,12 +1439,12 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy) cpudata->cpu = policy->cpu; cpudata->epp_policy =0 ; -amd_pstate_init_prefcore (cpudata); - ret =amd_pstate_init_perf (cpudata);if (ret) goto free_cpudata1; +amd_pstate_init_prefcore (cpudata); + ret =amd_pstate_init_freq (cpudata);if (ret) goto free_cpudata1;@@ -1903,6 +1889,12 @@ static int __init amd_pstate_init(void)static_call_update (amd_pstate_update_perf, cppc_update_perf); } +if (amd_pstate_prefcore) { + ret =amd_detect_prefcore (&amd_pstate_prefcore); +if (ret) +return ret; + } +/* enable amd pstate feature */ ret =amd_pstate_enable (true);if (ret) {include/acpi/cppc_acpi.h +2
-0@@ -137,10 +137,12 @@ struct cppc_cpudata { };#ifdef CONFIG_CPU_SUP_AMD +extern int amd_detect_prefcore (bool *detected);extern int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf);extern int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator);#else /* !CONFIG_CPU_SUP_AMD */ static inline int amd_get_highest_perf (unsigned int cpu,u32 *highest_perf) {return -ENODEV ; } +static inline int amd_detect_prefcore (bool *detected) {return -ENODEV ; }static inline int amd_get_boost_ratio_numerator (unsigned int cpu,u64 *numerator) {return -ENODEV ; }#endif /* !CONFIG_CPU_SUP_AMD */ Signature · 1 line
2.43.0
PATCH 6/8 cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
The special case in amd_pstate_highest_perf_set() is the value used for calculating the boost numerator. Merge this into amd_get_boost_ratio_numerator() and then use that to calculate boost ratio.
This allows dropping more special casing of the highest perf value.
--- arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++ drivers/cpufreq/amd-pstate.c | 49 +++++++----------------------------- 2 files changed, 25 insertions(+), 40 deletions(-)
arch/x86/kernel/acpi/cppc.c +16 -0
@@ -9,6 +9,7 @@#include <asm/processor.h> #include <asm/topology.h> +#define CPPC_HIGHEST_PERF_PERFORMANCE 196#define CPPC_HIGHEST_PERF_PREFCORE 166enum amd_pref_core {
@@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) *numerator = boost_numerator;return 0 ; } + +/* +* For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f, +* the highest performance level is set to 196. +* https://bugzilla.kernel.org/show_bug.cgi?id=218759 +*/ +if (cpu_feature_enabled (X86_FEATURE_ZEN4 )) { +switch (boot_cpu_data.x86_model) { + case 0x70 ... 0x7f: + *numerator =CPPC_HIGHEST_PERF_PERFORMANCE ; +return 0 ; +default : +break ; + } + } *numerator =CPPC_HIGHEST_PERF_PREFCORE ;return 0 ;
drivers/cpufreq/amd-pstate.c +9 -40
88 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()
Hello Mario,
Quoted 95 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The special case in
amd_pstate_highest_perf_set()is the value used for calculating the boost numerator. Merge this intoamd_get_boost_ratio_numerator()and then use that to calculate boost ratio.This allows dropping more special casing of the highest perf value.
Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++ drivers/cpufreq/amd-pstate.c | 49 +++++++----------------------------- 2 files changed, 25 insertions(+), 40 deletions(-)arch/x86/kernel/acpi/cppc.c +16
-0@@ -9,6 +9,7 @@#include <asm/processor.h> #include <asm/topology.h> +#define CPPC_HIGHEST_PERF_PERFORMANCE 196#define CPPC_HIGHEST_PERF_PREFCORE 166enum amd_pref_core {@@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) *numerator = boost_numerator;return 0 ; } + +/* +* For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f, +* the highest performance level is set to 196. +* https://bugzilla.kernel.org/show_bug.cgi?id=218759 +*/ +if (cpu_feature_enabled (X86_FEATURE_ZEN4 )) { +switch (boot_cpu_data.x86_model) { + case 0x70 ... 0x7f: + *numerator =CPPC_HIGHEST_PERF_PERFORMANCE ; +return 0 ; +default : +break ; + } + } *numerator =CPPC_HIGHEST_PERF_PREFCORE ;return 0 ;drivers/cpufreq/amd-pstate.c +0
-22@@ -52,8 +52,6 @@#define AMD_PSTATE_TRANSITION_LATENCY 20000#define AMD_PSTATE_TRANSITION_DELAY 1000#define AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY 600 -#define CPPC_HIGHEST_PERF_PERFORMANCE 196 -#define CPPC_HIGHEST_PERF_DEFAULT 166#define AMD_CPPC_EPP_PERFORMANCE 0x00#define AMD_CPPC_EPP_BALANCE_PERFORMANCE 0x80@@ -372,43 +370,17 @@ static inline int amd_pstate_enable(bool enable)return static_call (amd_pstate_enable)(enable); } -static u32 amd_pstate_highest_perf_set (struct amd_cpudata *cpudata) -{ -struct cpuinfo_x86 *c = &cpu_data (0 ); - -/* -* For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f, -* the highest performance level is set to 196. -* https://bugzilla.kernel.org/show_bug.cgi?id=218759 -*/ -if (c->x86 ==0x19 && (c->x86_model >=0x70 && c->x86_model <=0x7f )) -return CPPC_HIGHEST_PERF_PERFORMANCE ; - -return CPPC_HIGHEST_PERF_DEFAULT ; -} -static int pstate_init_perf (struct amd_cpudata *cpudata) {u64 cap1; -u32 highest_perf;int ret =rdmsrl_safe_on_cpu (cpudata->cpu,MSR_AMD_CPPC_CAP1 , &cap1);if (ret)return ret; -/* For platforms that do not support the preferred core feature, the -* highest_pef may be configured with 166 or 255, to avoid max frequency -* calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as -* the default max perf.
drivers/cpufreq/amd-pstate.c static inline int amd_pstate_enable(bool enable) · line 383
Quoted 10 lines
drivers/cpufreq/amd-pstate.c +2
-8-*/ -if (cpudata->hw_prefcore) - highest_perf =amd_pstate_highest_perf_set (cpudata); -else - highest_perf =AMD_CPPC_HIGHEST_PERF (cap1); - -WRITE_ONCE (cpudata->highest_perf, highest_perf); -WRITE_ONCE (cpudata->max_limit_perf, highest_perf); +WRITE_ONCE (cpudata->highest_perf,AMD_CPPC_HIGHEST_PERF (cap1)); +WRITE_ONCE (cpudata->max_limit_perf,AMD_CPPC_HIGHEST_PERF (cap1));
So henceforth, cpudata->highest_perf is expected to cache the value of CPPC.highest_perf and not the boost_ratio_numerator. There are couple of user-visible changes due to this.
1. On platforms where preferred-core is supported, previously the sysfs file /sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perf would report the boost_ratio_numerator. Henceforth it will report CPPC.highest_perf.
I hope there are no userspace tools that compute the boost_ratio using the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.
2. The amd_pstate_prefcore_ranking and amd_pstate_highest_perf will show the same values on all platforms, and henceforth are redundant.
Shouldn't this be documented?
The rest of the patch looks good to me.
--
Thanks and Regards
gautham.
Mario Limonciello
maintainerRe: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()
Quoted 122 lines · Gautham R. Shenoy
Hello Mario,
Quoted 95 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
The special case in
amd_pstate_highest_perf_set()is the value used for calculating the boost numerator. Merge this intoamd_get_boost_ratio_numerator()and then use that to calculate boost ratio.This allows dropping more special casing of the highest perf value.
Signed-off-by Mario Limonciello <[email protected]>--- arch/x86/kernel/acpi/cppc.c | 16 ++++++++++++ drivers/cpufreq/amd-pstate.c | 49 +++++++----------------------------- 2 files changed, 25 insertions(+), 40 deletions(-)arch/x86/kernel/acpi/cppc.c +16
-0@@ -9,6 +9,7 @@#include <asm/processor.h> #include <asm/topology.h> +#define CPPC_HIGHEST_PERF_PERFORMANCE 196#define CPPC_HIGHEST_PERF_PREFCORE 166enum amd_pref_core {@@ -244,6 +245,21 @@ int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator) *numerator = boost_numerator;return 0 ; } + +/* +* For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f, +* the highest performance level is set to 196. +* https://bugzilla.kernel.org/show_bug.cgi?id=218759 +*/ +if (cpu_feature_enabled (X86_FEATURE_ZEN4 )) { +switch (boot_cpu_data.x86_model) { + case 0x70 ... 0x7f: + *numerator =CPPC_HIGHEST_PERF_PERFORMANCE ; +return 0 ; +default : +break ; + } + } *numerator =CPPC_HIGHEST_PERF_PREFCORE ;return 0 ;drivers/cpufreq/amd-pstate.c +0
-22@@ -52,8 +52,6 @@#define AMD_PSTATE_TRANSITION_LATENCY 20000#define AMD_PSTATE_TRANSITION_DELAY 1000#define AMD_PSTATE_FAST_CPPC_TRANSITION_DELAY 600 -#define CPPC_HIGHEST_PERF_PERFORMANCE 196 -#define CPPC_HIGHEST_PERF_DEFAULT 166#define AMD_CPPC_EPP_PERFORMANCE 0x00#define AMD_CPPC_EPP_BALANCE_PERFORMANCE 0x80@@ -372,43 +370,17 @@ static inline int amd_pstate_enable(bool enable)return static_call (amd_pstate_enable)(enable); } -static u32 amd_pstate_highest_perf_set (struct amd_cpudata *cpudata) -{ -struct cpuinfo_x86 *c = &cpu_data (0 ); - -/* -* For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7f, -* the highest performance level is set to 196. -* https://bugzilla.kernel.org/show_bug.cgi?id=218759 -*/ -if (c->x86 ==0x19 && (c->x86_model >=0x70 && c->x86_model <=0x7f )) -return CPPC_HIGHEST_PERF_PERFORMANCE ; - -return CPPC_HIGHEST_PERF_DEFAULT ; -} -static int pstate_init_perf (struct amd_cpudata *cpudata) {u64 cap1; -u32 highest_perf;int ret =rdmsrl_safe_on_cpu (cpudata->cpu,MSR_AMD_CPPC_CAP1 , &cap1);if (ret)return ret; -/* For platforms that do not support the preferred core feature, the -* highest_pef may be configured with 166 or 255, to avoid max frequency -* calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as -* the default max perf. drivers/cpufreq/amd-pstate.c static inline int amd_pstate_enable(bool enable) · line 383
Quoted 10 lines
drivers/cpufreq/amd-pstate.c +2
-8-*/ -if (cpudata->hw_prefcore) - highest_perf =amd_pstate_highest_perf_set (cpudata); -else - highest_perf =AMD_CPPC_HIGHEST_PERF (cap1); - -WRITE_ONCE (cpudata->highest_perf, highest_perf); -WRITE_ONCE (cpudata->max_limit_perf, highest_perf); +WRITE_ONCE (cpudata->highest_perf,AMD_CPPC_HIGHEST_PERF (cap1)); +WRITE_ONCE (cpudata->max_limit_perf,AMD_CPPC_HIGHEST_PERF (cap1));So henceforth,
cpudata->highest_perfis expected to cache the value of CPPC.highest_perf and not theboost_ratio_numerator. There are couple of user-visible changes due to this.1. On platforms where preferred-core is supported, previously the sysfs file
/sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perfwould report theboost_ratio_numerator. Henceforth it will report CPPC.highest_perf.
Quoted 7 lines
I hope there are no userspace tools that compute the
boost_ratiousing the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.2. The
amd_pstate_prefcore_rankingandamd_pstate_highest_perfwill show the same values on all platforms, and henceforth are redundant.
Good observations here. I'm not aware of any tools trying to replicate this calculation. With the redundancy I would actually argue we should just drop the sysfs file 'amd_pstate_prefcore_ranking'.
Thoughts?
Quoted 2 lines
Shouldn't this be documented?
I noticed amd_pstate_prefcore_ranking wasn't properly documented in amd-pstate.rst in the first place. If the decision is not to drop the sysfs file, then I'll add a section for it.
Quoted 9 lines
The rest of the patch looks good to me.
--
Thanks and Regards
gautham.
Re: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()
Quoted 1 line · Mario Limonciello
On 8/27/2024 11:52, Gautham R. Shenoy wrote:
[..snip..]
Quoted 7 lines
Quoted 7 lines
So henceforth,
cpudata->highest_perfis expected to cache the value of CPPC.highest_perf and not theboost_ratio_numerator. There are couple of user-visible changes due to this.
Quoted 5 lines
Quoted 5 lines
1. On platforms where preferred-core is supported, previously the sysfs file
/sys/devices/system/cpu/cpuX/cpufreq/amd_pstate_highest_perfwould report theboost_ratio_numerator. Henceforth it will report CPPC.highest_perf.
One other side effect is that the highest_perf sysfs file will now reveal the differential highest_perf, even when "amd_prefcore=false", while earlier all the cores would report CPPC_HIGHEST_PERF_DEFAULT.
I think we may be better off reporting the boost-numerator here, but that's really not the highest_perf :(
Quoted 9 lines
Quoted 8 lines
I hope there are no userspace tools that compute the
boost_ratiousing the syfs amd_pstate_highest_perf/amd_pstate_nominal_perf.2. The
amd_pstate_prefcore_rankingandamd_pstate_highest_perfwill show the same values on all platforms, and henceforth are redundant.
Quoted 6 lines
Good observations here. I'm not aware of any tools trying to replicate this calculation.
With the redundancy I would actually argue we should just drop the sysfs file 'amd_pstate_prefcore_ranking'.Thoughts?
Looking at the code again, I realize that I was wrong. cpudata->prefcore_ranking also gets updated in amd_pstate_update_min_max_limits() and reflects the dynamic preference.
While cpudata->highest_perf value indicates the initial boot-time preference.
Hence it makes sense to amd_pstate_prefcore_ranking.
Quoted 7 lines
Quoted 3 lines
Shouldn't this be documented?
I noticed
amd_pstate_prefcore_rankingwasn't properly documented inamd-pstate.rstin the first place. If the decision is not to drop the sysfs file, then I'll add a section for it.
Thanks.
Quoted 6 lines
Quoted 6 lines
The rest of the patch looks good to me.
--
Thanks and Regards
gautham.
kernel test robot
build botRe: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator()
In reply to Mario Limonciellokernel test robot noticed the following build warnings:
Hi Mario,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge tip/x86/core tip/master linus/master v6.11-rc5 next-20240827] [cannot apply to tip/auto-latest]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/x86-amd-Move-amd_get_highest_perf-from-amd-c-to-cppc-c/20240827-051648 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20240826211358.2694603-7-superm1%40kernel.org patch subject: [PATCH 6/8] cpufreq: amd-pstate: Merge amd_pstate_highest_perf_set() into amd_get_boost_ratio_numerator() config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240828/[email protected]/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240828/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/cpufreq/amd-pstate.c:402:38: warning: variable 'highest_perf' is uninitialized when used here [-Wuninitialized]
402 | WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
| ^~~~~~~~~~~~
include/asm-generic/rwonce.h:61:18: note: expanded from macro 'WRITE_ONCE'
61 | __WRITE_ONCE(x, val); \
| ^~~
include/asm-generic/rwonce.h:55:33: note: expanded from macro '__WRITE_ONCE'
55 | *(volatile typeof(x) *)&(x) = (val); \
| ^~~
drivers/cpufreq/amd-pstate.c:395:18: note: initialize the variable 'highest_perf' to silence this warning
395 | u32 highest_perf;
| ^
| = 01 warning generated.
vim +/highest_perf +402 drivers/cpufreq/amd-pstate.c
ec437d71db77a1 Huang Rui 2021-12-24 391
e059c184da47e9 Huang Rui 2021-12-24 392 static int cppc_init_perf(struct amd_cpudata *cpudata)
e059c184da47e9 Huang Rui 2021-12-24 393 {
e059c184da47e9 Huang Rui 2021-12-24 394 struct cppc_perf_caps cppc_perf;
bedadcfb011fef Perry Yuan 2022-08-30 395 u32 highest_perf;
e059c184da47e9 Huang Rui 2021-12-24 396
e059c184da47e9 Huang Rui 2021-12-24 397 int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
e059c184da47e9 Huang Rui 2021-12-24 398 if (ret)
e059c184da47e9 Huang Rui 2021-12-24 399 return ret;
e059c184da47e9 Huang Rui 2021-12-24 400
347b3754cc9780 Mario Limonciello 2024-08-26 401 WRITE_ONCE(cpudata->highest_perf, cppc_perf.highest_perf);
febab20caebac9 Wyes Karny 2023-11-17 @402 WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
e059c184da47e9 Huang Rui 2021-12-24 403 WRITE_ONCE(cpudata->nominal_perf, cppc_perf.nominal_perf);
e059c184da47e9 Huang Rui 2021-12-24 404 WRITE_ONCE(cpudata->lowest_nonlinear_perf,
e059c184da47e9 Huang Rui 2021-12-24 405 cppc_perf.lowest_nonlinear_perf);
e059c184da47e9 Huang Rui 2021-12-24 406 WRITE_ONCE(cpudata->lowest_perf, cppc_perf.lowest_perf);
e571a5e2068ef5 Meng Li 2024-01-19 407 WRITE_ONCE(cpudata->prefcore_ranking, cppc_perf.highest_perf);
febab20caebac9 Wyes Karny 2023-11-17 408 WRITE_ONCE(cpudata->min_limit_perf, cppc_perf.lowest_perf);
e059c184da47e9 Huang Rui 2021-12-24 409
2dd6d0ebf74049 Wyes Karny 2023-03-07 410 if (cppc_state == AMD_PSTATE_ACTIVE)
2dd6d0ebf74049 Wyes Karny 2023-03-07 411 return 0;
2dd6d0ebf74049 Wyes Karny 2023-03-07 412
2dd6d0ebf74049 Wyes Karny 2023-03-07 413 ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf);
2dd6d0ebf74049 Wyes Karny 2023-03-07 414 if (ret) {
2dd6d0ebf74049 Wyes Karny 2023-03-07 415 pr_warn("failed to get auto_sel, ret: %d\n", ret);
e059c184da47e9 Huang Rui 2021-12-24 416 return 0;
e059c184da47e9 Huang Rui 2021-12-24 417 }
e059c184da47e9 Huang Rui 2021-12-24 418
2dd6d0ebf74049 Wyes Karny 2023-03-07 419 ret = cppc_set_auto_sel(cpudata->cpu,
2dd6d0ebf74049 Wyes Karny 2023-03-07 420 (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1);
2dd6d0ebf74049 Wyes Karny 2023-03-07 421
2dd6d0ebf74049 Wyes Karny 2023-03-07 422 if (ret)
2dd6d0ebf74049 Wyes Karny 2023-03-07 423 pr_warn("failed to set auto_sel, ret: %d\n", ret);
2dd6d0ebf74049 Wyes Karny 2023-03-07 424
2dd6d0ebf74049 Wyes Karny 2023-03-07 425 return ret;
2dd6d0ebf74049 Wyes Karny 2023-03-07 426 }
2dd6d0ebf74049 Wyes Karny 2023-03-07 427 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
PATCH 7/8 cpufreq: amd-pstate: Optimize amd_pstate_update_limits()
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
Don't take and release the mutex when prefcore isn't present and avoid initialization of variables that will be initially set in the function.
--- drivers/cpufreq/amd-pstate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/cpufreq/amd-pstate.c +5 -5
@@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)int ret;bool highest_perf_changed = false; -mutex_lock (&amd_pstate_driver_lock); -if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) - goto free_cpufreq_put; +if (!amd_pstate_prefcore) +return ; +mutex_lock (&amd_pstate_driver_lock); ret =amd_get_highest_perf (cpu, &cur_high);if (ret) goto free_cpufreq_put; prev_high =READ_ONCE (cpudata->prefcore_ranking); -if (prev_high != cur_high) { - highest_perf_changed = true; + highest_perf_changed = (prev_high != cur_high); +if (highest_perf_changed) {WRITE_ONCE (cpudata->prefcore_ranking, cur_high);if (cur_high <CPPC_MAX_PERF )
Signature · 1 line
2.43.0
RE: [PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 51 lines
-----Original Message-----
From: Mario Limonciello <[email protected]>
Sent: Tuesday, August 27, 2024 5:14 AM
To: Borislav Petkov <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Yuan, Perry <[email protected]>
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; Rafael J . Wysocki <[email protected]>; open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; open list:ACPI <linux- [email protected]>; open list:CPU FREQUENCY SCALING FRAMEWORK <[email protected]>; Limonciello, Mario <[email protected]>
Subject: [PATCH 7/8] cpufreq: amd-pstate: Optimizeamd_pstate_update_limits()From: Mario Limonciello <[email protected]>
Don't take and release the mutex when prefcore isn't present and avoid initialization of variables that will be initially set in the function.
Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/cpufreq/amd-pstate.c +5
-5@@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)int ret;bool highest_perf_changed = false; -mutex_lock (&amd_pstate_driver_lock); -if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) - goto free_cpufreq_put; +if (!amd_pstate_prefcore) +return ; +mutex_lock (&amd_pstate_driver_lock); ret =amd_get_highest_perf (cpu, &cur_high);if (ret) goto free_cpufreq_put; prev_high =READ_ONCE (cpudata->prefcore_ranking); -if (prev_high != cur_high) { - highest_perf_changed = true; + highest_perf_changed = (prev_high != cur_high); +if (highest_perf_changed) {WRITE_ONCE (cpudata->prefcore_ranking, cur_high);
Quoted 4 lines
if (
cur_high<CPPC_MAX_PERF)
--
2.43.0
Best Regards.
Perry.
Re: [PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()
In reply to Mario LimoncielloQuoted 17 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
Don't take and release the mutex when prefcore isn't present and avoid initialization of variables that will be initially set in the function.
Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/cpufreq/amd-pstate.c +0
-0
drivers/cpufreq/amd-pstate.c static void amd_pstate_update_limits(unsigned int cpu) · line 802
Quoted 8 lines
drivers/cpufreq/amd-pstate.c +2
-3@@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)int ret;bool highest_perf_changed = false; -mutex_lock (&amd_pstate_driver_lock); -if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) - goto free_cpufreq_put; +if (!amd_pstate_prefcore) +return ;
Looks good to me.
Wondering if it is worth maintaining a static key for amd_pstate_prefcore. Anyway it doesn't change after boot.
--
Thanks and Regards
gautham.
Mario Limonciello
maintainerRe: [PATCH 7/8] cpufreq: amd-pstate: Optimize amd_pstate_update_limits()
Quoted 29 lines · Gautham R. Shenoy
Quoted 17 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
Don't take and release the mutex when prefcore isn't present and avoid initialization of variables that will be initially set in the function.
Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/cpufreq/amd-pstate.c +0
-0drivers/cpufreq/amd-pstate.c static void amd_pstate_update_limits(unsigned int cpu) · line 802
Quoted 8 lines
drivers/cpufreq/amd-pstate.c +2
-3@@ -798,17 +798,17 @@ static void amd_pstate_update_limits(unsigned int cpu)int ret;bool highest_perf_changed = false; -mutex_lock (&amd_pstate_driver_lock); -if ((!amd_pstate_prefcore) || (!cpudata->hw_prefcore)) - goto free_cpufreq_put; +if (!amd_pstate_prefcore) +return ;Looks good to me.
Quoted 2 lines
Wondering if it is worth maintaining a static key for
amd_pstate_prefcore. Anyway it doesn't change after boot.
As there is a kernel command line option how would you pass the early param parsing result over without a static variable?
Quoted 3 lines
Reviewed-by Gautham R. Shenoy <[email protected]>
Thanks!
Quoted 4 lines
--
Thanks and Regards
gautham.
PATCH 8/8 cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
As the global variable is cleared when preferred cores is not present the local variable use isn't needed in many functions. Drop it where possible. No intended functional changes.
--- drivers/cpufreq/amd-pstate.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/cpufreq/amd-pstate.c +1 -6
@@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,static ssize_t show_amd_pstate_hw_prefcore (struct cpufreq_policy *policy,char *buf) { -bool hw_prefcore; -struct amd_cpudata *cpudata = policy->driver_data; - - hw_prefcore =READ_ONCE (cpudata->hw_prefcore); - -return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (hw_prefcore)); +return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (amd_pstate_prefcore)); } static ssize_t show_energy_performance_available_preferences(
Signature · 1 line
2.43.0
RE: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 46 lines
-----Original Message-----
From: Mario Limonciello <[email protected]>
Sent: Tuesday, August 27, 2024 5:14 AM
To: Borislav Petkov <[email protected]>; Shenoy, Gautham Ranjal <[email protected]>; Yuan, Perry <[email protected]>
Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; Rafael J . Wysocki <[email protected]>; open list:X86 ARCHITECTURE (32-BIT AND 64-BIT) <[email protected]>; open list:ACPI <linux- [email protected]>; open list:CPU FREQUENCY SCALING FRAMEWORK <[email protected]>; Limonciello, Mario <[email protected]>
Subject: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata-Quoted 1 line
hw_prefcoreFrom: Mario Limonciello <[email protected]>
As the global variable is cleared when preferred cores is not present the local variable use isn't needed in many functions.
Drop it where possible. No intended functional changes.Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/cpufreq/amd-pstate.c +0
-0@@ -1112,12 +1112,7 @@ static ssize_tshow_amd_pstate_prefcore_ranking(struct
cpufreq_policy*policy, staticssize_tshow_amd_pstate_hw_prefcore(structcpufreq_policy*policy,
char *buf)
{drivers/cpufreq/amd-pstate.c +2
-7-bool hw_prefcore; -struct amd_cpudata *cpudata = policy->driver_data; - - hw_prefcore =READ_ONCE (cpudata->hw_prefcore); - -return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (hw_prefcore)); +return sysfs_emit (buf,"%s\n" , +str_enabled_disabled (amd_pstate_prefcore)); } static ssize_t show_energy_performance_available_preferences( --
Quoted 2 lines
2.43.0
Best Regards.
Perry.
Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
In reply to Mario LimoncielloQuoted 16 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
As the global variable is cleared when preferred cores is not present the local variable use isn't needed in many functions. Drop it where possible. No intended functional changes.
Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/cpufreq/amd-pstate.c +0
-0
drivers/cpufreq/amd-pstate.c static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy, · line 1115
Quoted 10 lines
drivers/cpufreq/amd-pstate.c +1
-6@@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,static ssize_t show_amd_pstate_hw_prefcore (struct cpufreq_policy *policy,char *buf) { -bool hw_prefcore; -struct amd_cpudata *cpudata = policy->driver_data; - - hw_prefcore =READ_ONCE (cpudata->hw_prefcore); - -return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (hw_prefcore)); +return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (amd_pstate_prefcore));
If the user boots with "amd_prefcore=disable" on a system that supports preferred-core, pror to this patchset, cpudata->hw_prefcore would be true and thus, amd_pstate_hw_prefcore would show "enabled".
With this patchset, it would show "disabled". Or am I missing something?
Quoted 6 lines
drivers/cpufreq/amd-pstate.c +0
-0} static ssize_t show_energy_performance_available_preferences(Signature · 1 line
2.43.0
--
Thanks and Regards
gautham.
Mario Limonciello
maintainerRe: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
Quoted 24 lines · Gautham R. Shenoy
Quoted 23 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
As the global variable is cleared when preferred cores is not present the local variable use isn't needed in many functions. Drop it where possible. No intended functional changes.
Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/cpufreq/amd-pstate.c +0
-4@@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,static ssize_t show_amd_pstate_hw_prefcore (struct cpufreq_policy *policy,char *buf) { -bool hw_prefcore; -struct amd_cpudata *cpudata = policy->driver_data; - - hw_prefcore =READ_ONCE (cpudata->hw_prefcore);
Quoted 9 lines
drivers/cpufreq/amd-pstate.c static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy, · line 1115
Quoted 3 lines
drivers/cpufreq/amd-pstate.c +1
-2- -return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (hw_prefcore)); +return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (amd_pstate_prefcore));If the user boots with "amd_prefcore=disable" on a system that supports preferred-core, pror to this patchset,
cpudata->hw_prefcorewould be true and thus,amd_pstate_hw_prefcorewould show "enabled".
Yes; you're right about the previous behavior.
Quoted 1 line
With this patchset, it would show "disabled". Or am I missing something?
This appears to be another case we don't have documentation for the sysfs file amd_pstate_hw_prefcore.
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a sched_ext based userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.
Whichever direction we agree to go; I'll add documentation for v2.
Quoted 13 lines
Quoted 9 lines
drivers/cpufreq/amd-pstate.c +0
-0} static ssize_t show_energy_performance_available_preferences(Signature · 1 line
2.43.0--
Thanks and Regards
gautham.
Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
Quoted 35 lines · Mario Limonciello
Quoted 24 lines · Gautham R. Shenoy
Quoted 23 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
As the global variable is cleared when preferred cores is not present the local variable use isn't needed in many functions. Drop it where possible. No intended functional changes.
Signed-off-by Mario Limonciello <[email protected]>--- drivers/cpufreq/amd-pstate.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/cpufreq/amd-pstate.c +0
-4@@ -1112,12 +1112,7 @@ static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,static ssize_t show_amd_pstate_hw_prefcore (struct cpufreq_policy *policy,char *buf) { -bool hw_prefcore; -struct amd_cpudata *cpudata = policy->driver_data; - - hw_prefcore =READ_ONCE (cpudata->hw_prefcore);Quoted 9 lines
drivers/cpufreq/amd-pstate.c static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy, · line 1115
Quoted 3 lines
drivers/cpufreq/amd-pstate.c +1
-2- -return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (hw_prefcore)); +return sysfs_emit (buf,"%s\n" ,str_enabled_disabled (amd_pstate_prefcore));If the user boots with "amd_prefcore=disable" on a system that supports preferred-core, pror to this patchset,
cpudata->hw_prefcorewould be true and thus,amd_pstate_hw_prefcorewould show "enabled".
Quoted 6 lines
Yes; you're right about the previous behavior.
Quoted 1 line
With this patchset, it would show "disabled". Or am I missing something?
This appears to be another case we don't have documentation for the sysfs file
amd_pstate_hw_prefcore.
:(
Quoted 8 lines
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a
sched_extbased userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.
The commandline parameter currently impacts only the fair sched-class tasks since the preference information gets used only during load-balancing.
IMO, the same should continue with sched-ext, i.e. if the user has explicitly disabled prefcore support via commandline, the no sched-ext scheduler should use the preference information to make task placement decisions. However, I would like to see what the sched-ext folks have to say. Adding some of them to the Cc list.
Quoted 2 lines
Whichever direction we agree to go; I'll add documentation for v2.
Yes please.
--
Thanks and Regards
gautham.
Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
...
Quoted 12 lines
Quoted 7 lines
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a
sched_extbased userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.The commandline parameter currently impacts only the fair sched-class tasks since the preference information gets used only during load-balancing.
Quoted 5 lines
IMO, the same should continue with sched-ext, i.e. if the user has explicitly disabled prefcore support via commandline, the no sched-ext scheduler should use the preference information to make task placement decisions. However, I would like to see what the sched-ext folks have to say. Adding some of them to the Cc list.
IMHO it makes more sense to reflect the real state of prefcore support from a "system" perspective, more than a "hardware" perspective, so if it's disabled via boot command line it should show disabled.
From a user-space scheduler perspective we should be fine either way, as long as the ABI is clearly documented, since we also have access to /proc/cmdline and we would be able to figure out if the user has disabled it via cmdline (however, the preference is still to report the actual system status).
Question: having prefcore enabled affects also the value of scaling_max_freq? Like an lscpu -e, for example, would show a higher max frequency for the specific preferred cores? (this is another useful information from a sched_ext scheduler perspective).
Thanks,
-Andrea
Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
Hello Andrea,
Quoted 24 lines · Andrea Righi
On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
...Quoted 12 lines
Quoted 7 lines
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a
sched_extbased userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.The commandline parameter currently impacts only the fair sched-class tasks since the preference information gets used only during load-balancing.
Quoted 5 lines
IMO, the same should continue with sched-ext, i.e. if the user has explicitly disabled prefcore support via commandline, the no sched-ext scheduler should use the preference information to make task placement decisions. However, I would like to see what the sched-ext folks have to say. Adding some of them to the Cc list.
IMHO it makes more sense to reflect the real state of prefcore support from a "system" perspective, more than a "hardware" perspective, so if it's disabled via boot command line it should show disabled.
Quoted 5 lines
From a user-space scheduler perspective we should be fine either way, as long as the ABI is clearly documented, since we also have access to
/proc/cmdlineand we would be able to figure out if the user has disabled it via cmdline (however, the preference is still to report the actual system status).
Thank you for confirming this.
Quoted 5 lines
Question: having prefcore enabled affects also the value of
scaling_max_freq? Like anlscpu -e, for example, would show a higher max frequency for the specific preferred cores? (this is another useful information from asched_extscheduler perspective).
Since the scaling_max_freq is computed based on the boost-numerator, at least from this patchset, the numerator would be the same across all kinds of cores, and thus the scaling_max_freq reported will be the same across all the cores.
Quoted 3 lines
Thanks,
-Andrea
--
Thanks and Regards
gautham.
Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
Quoted 41 lines · Gautham R. Shenoy
Hello Andrea,
Quoted 24 lines · Andrea Righi
On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
...Quoted 12 lines
Quoted 7 lines
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a
sched_extbased userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.The commandline parameter currently impacts only the fair sched-class tasks since the preference information gets used only during load-balancing.
Quoted 5 lines
IMO, the same should continue with sched-ext, i.e. if the user has explicitly disabled prefcore support via commandline, the no sched-ext scheduler should use the preference information to make task placement decisions. However, I would like to see what the sched-ext folks have to say. Adding some of them to the Cc list.
IMHO it makes more sense to reflect the real state of prefcore support from a "system" perspective, more than a "hardware" perspective, so if it's disabled via boot command line it should show disabled.
Quoted 5 lines
From a user-space scheduler perspective we should be fine either way, as long as the ABI is clearly documented, since we also have access to
/proc/cmdlineand we would be able to figure out if the user has disabled it via cmdline (however, the preference is still to report the actual system status).Thank you for confirming this.
Quoted 5 lines
Question: having prefcore enabled affects also the value of
scaling_max_freq? Like anlscpu -e, for example, would show a higher max frequency for the specific preferred cores? (this is another useful information from asched_extscheduler perspective).
Quoted 4 lines
Since the
scaling_max_freqis computed based on the boost-numerator, at least from this patchset, the numerator would be the same across all kinds of cores, and thus thescaling_max_freqreported will be the same across all the cores.
I see, so IIUC from user-space the most reliable way to detect the fastest cores is to check amd_pstate_highest_perf / amd_pstate_max_freq, right? I'm trying to figure out a way to abstract and generalize the concept of "fast cores" in sched_ext.
Also, is this something that has changed recently? I see this on an AMD Ryzen Threadripper PRO 7975WX 32-Cores running a 6.8 kernel:
$ uname -r 6.8.0-40-generic
$ grep . /sys/devices/system/cpu/cpu*/cpufreq/cpuinfo_max_freq /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu1/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu2/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu3/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu4/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu5/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu6/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu7/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu8/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu9/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu10/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu11/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu12/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu13/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu14/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu15/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu16/cpufreq/cpuinfo_max_freq:6161000 /sys/devices/system/cpu/cpu17/cpufreq/cpuinfo_max_freq:6321000 /sys/devices/system/cpu/cpu18/cpufreq/cpuinfo_max_freq:6001000 /sys/devices/system/cpu/cpu19/cpufreq/cpuinfo_max_freq:6646000 /sys/devices/system/cpu/cpu20/cpufreq/cpuinfo_max_freq:5837000 /sys/devices/system/cpu/cpu21/cpufreq/cpuinfo_max_freq:6482000 /sys/devices/system/cpu/cpu22/cpufreq/cpuinfo_max_freq:5517000 /sys/devices/system/cpu/cpu23/cpufreq/cpuinfo_max_freq:5677000 /sys/devices/system/cpu/cpu24/cpufreq/cpuinfo_max_freq:6966000 /sys/devices/system/cpu/cpu25/cpufreq/cpuinfo_max_freq:7775000 /sys/devices/system/cpu/cpu26/cpufreq/cpuinfo_max_freq:6806000 /sys/devices/system/cpu/cpu27/cpufreq/cpuinfo_max_freq:7775000 /sys/devices/system/cpu/cpu28/cpufreq/cpuinfo_max_freq:7130000 /sys/devices/system/cpu/cpu29/cpufreq/cpuinfo_max_freq:7451000 /sys/devices/system/cpu/cpu30/cpufreq/cpuinfo_max_freq:7290000 /sys/devices/system/cpu/cpu31/cpufreq/cpuinfo_max_freq:7611000 /sys/devices/system/cpu/cpu32/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu33/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu34/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu35/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu36/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu37/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu38/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu39/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu40/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu41/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu42/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu43/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu44/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu45/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu46/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu47/cpufreq/cpuinfo_max_freq:5352000 /sys/devices/system/cpu/cpu48/cpufreq/cpuinfo_max_freq:6161000 /sys/devices/system/cpu/cpu49/cpufreq/cpuinfo_max_freq:6321000 /sys/devices/system/cpu/cpu50/cpufreq/cpuinfo_max_freq:6001000 /sys/devices/system/cpu/cpu51/cpufreq/cpuinfo_max_freq:6646000 /sys/devices/system/cpu/cpu52/cpufreq/cpuinfo_max_freq:5837000 /sys/devices/system/cpu/cpu53/cpufreq/cpuinfo_max_freq:6482000 /sys/devices/system/cpu/cpu54/cpufreq/cpuinfo_max_freq:5517000 /sys/devices/system/cpu/cpu55/cpufreq/cpuinfo_max_freq:5677000 /sys/devices/system/cpu/cpu56/cpufreq/cpuinfo_max_freq:6966000 /sys/devices/system/cpu/cpu57/cpufreq/cpuinfo_max_freq:7775000 /sys/devices/system/cpu/cpu58/cpufreq/cpuinfo_max_freq:6806000 /sys/devices/system/cpu/cpu59/cpufreq/cpuinfo_max_freq:7775000 /sys/devices/system/cpu/cpu60/cpufreq/cpuinfo_max_freq:7130000 /sys/devices/system/cpu/cpu61/cpufreq/cpuinfo_max_freq:7451000 /sys/devices/system/cpu/cpu62/cpufreq/cpuinfo_max_freq:7290000 /sys/devices/system/cpu/cpu63/cpufreq/cpuinfo_max_freq:7611000
Thanks,
-Andrea
Mario Limonciello
maintainerRe: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
Quoted 47 lines · Andrea Righi
Quoted 41 lines · Gautham R. Shenoy
Hello Andrea,
Quoted 24 lines · Andrea Righi
On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
...Quoted 12 lines
Quoted 7 lines
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a
sched_extbased userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.The commandline parameter currently impacts only the fair sched-class tasks since the preference information gets used only during load-balancing.
Quoted 5 lines
IMO, the same should continue with sched-ext, i.e. if the user has explicitly disabled prefcore support via commandline, the no sched-ext scheduler should use the preference information to make task placement decisions. However, I would like to see what the sched-ext folks have to say. Adding some of them to the Cc list.
IMHO it makes more sense to reflect the real state of prefcore support from a "system" perspective, more than a "hardware" perspective, so if it's disabled via boot command line it should show disabled.
Quoted 5 lines
From a user-space scheduler perspective we should be fine either way, as
long as the ABI is clearly documented, since we also have access to/proc/cmdlineand we would be able to figure out if the user has disabled it via cmdline (however, the preference is still to report the actual system status).Thank you for confirming this.
Quoted 5 lines
Question: having prefcore enabled affects also the value of
scaling_max_freq? Like anlscpu -e, for example, would show a higher max frequency for the specific preferred cores? (this is another useful information from asched_extscheduler perspective).Quoted 4 lines
Since the
scaling_max_freqis computed based on the boost-numerator, at least from this patchset, the numerator would be the same across all kinds of cores, and thus thescaling_max_freqreported will be the same across all the cores.
Quoted 4 lines
I see, so IIUC from user-space the most reliable way to detect the fastest cores is to check
amd_pstate_highest_perf/amd_pstate_max_freq, right? I'm trying to figure out a way to abstract and generalize the concept of "fast cores" insched_ext.
Right now the best way to do this is to look at the amd_pstate_precore_ranking file.
In this series there has been some discussion of dropping it though in favor of looking at the highest perf file. I don't believe we're concluded one way or another on it yet though.
Quoted 6 lines
Also, is this something that has changed recently? I see this on an AMD Ryzen Threadripper PRO 7975WX 32-Cores running a 6.8 kernel:
$ uname -r 6.8.0-40-generic
You're missing the preferred core patches on this kernel. They landed in 6.9, it's better to upgrade to 6.10.y or 6.11-rc.
Re: [PATCH 8/8] cpufreq: amd-pstate: Drop some uses of cpudata->hw_prefcore
Quoted 48 lines · Mario Limonciello
Quoted 47 lines · Andrea Righi
Quoted 45 lines · Gautham R. Shenoy
Hello Andrea,
Quoted 24 lines · Andrea Righi
On Wed, Aug 28, 2024 at 10:38:45AM +0530, Gautham R. Shenoy wrote:
...Quoted 12 lines
Quoted 7 lines
I had thought this was a malfunction in the behavior that it reflected the current status, not the hardware /capability/.
Which one makes more sense for userspace? In my mind the most likely consumer of this information would be something a
sched_extbased userspace scheduler. They would need to know whether the scheduler was using preferred cores; not whether the hardware supported it.The commandline parameter currently impacts only the fair sched-class tasks since the preference information gets used only during load-balancing.
Quoted 5 lines
IMO, the same should continue with sched-ext, i.e. if the user has explicitly disabled prefcore support via commandline, the no sched-ext scheduler should use the preference information to make task placement decisions. However, I would like to see what the sched-ext folks have to say. Adding some of them to the Cc list.
IMHO it makes more sense to reflect the real state of prefcore support from a "system" perspective, more than a "hardware" perspective, so if it's disabled via boot command line it should show disabled.
Quoted 5 lines
From a user-space scheduler perspective we should be fine either way, as
long as the ABI is clearly documented, since we also have access to/proc/cmdlineand we would be able to figure out if the user has disabled it via cmdline (however, the preference is still to report the actual system status).Thank you for confirming this.
Quoted 5 lines
Question: having prefcore enabled affects also the value of
scaling_max_freq? Like anlscpu -e, for example, would show a higher max frequency for the specific preferred cores? (this is another useful information from asched_extscheduler perspective).Since the
scaling_max_freqis computed based on the boost-numerator, at least from this patchset, the numerator would be the same across all kinds of cores, and thus thescaling_max_freqreported will be the same across all the cores.
Quoted 7 lines
Quoted 4 lines
I see, so IIUC from user-space the most reliable way to detect the fastest cores is to check
amd_pstate_highest_perf/amd_pstate_max_freq, right? I'm trying to figure out a way to abstract and generalize the concept of "fast cores" insched_ext.Right now the best way to do this is to look at the
amd_pstate_precore_rankingfile.
Ok.
Quoted 6 lines
In this series there has been some discussion of dropping it though in favor of looking at the highest perf file. I don't believe we're concluded one way or another on it yet though.
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Also, is this something that has changed recently? I see this on an AMD Ryzen Threadripper PRO 7975WX 32-Cores running a 6.8 kernel:
$ uname -r 6.8.0-40-genericYou're missing the preferred core patches on this kernel. They landed in 6.9, it's better to upgrade to 6.10.y or 6.11-rc.
So, if I move to 6.9+ I should see the same max frequency across all the CPUs and I can use amd_pstate_precore_ranking to determine the subset of fast cores.
Thanks for the clarification.
-Andrea