| Message ID | 20230530131348.4135-1-wyes.karny@amd.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] cpufreq/amd-pstate: Write CPPC enable bit per-socket | expand |
On Tue, May 30, 2023 at 09:13:48PM +0800, Karny, Wyes wrote: > Currently amd_pstate sets CPPC enable bit in MSR_AMD_CPPC_ENABLE only > for the CPU where the module_init happened. But MSR_AMD_CPPC_ENABLE is > per-socket. This causes CPPC enable bit to set for only one socket for > servers with more than one physical packages. To fix this write > MSR_AMD_CPPC_ENABLE per-socket. > > Also, handle duplicate calls for cppc_enable, because it's called from > per-policy/per-core callbacks and can result in duplicate MSR writes. > > Before the fix: > amd@amd:~$ sudo rdmsr -a 0xc00102b1 | uniq --count > 192 0 > 192 1 > > After the fix: > amd@amd:~$ sudo rdmsr -a 0xc00102b1 | uniq --count > 384 1 > > Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > Signed-off-by: Wyes Karny <wyes.karny@amd.com> > --- > v1 -> v2: > - Made CPPC enable read/write per-socket > > drivers/cpufreq/amd-pstate.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 5a3d4aa0f45a..45b9e359f638 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -63,6 +63,7 @@ static struct cpufreq_driver *current_pstate_driver; > static struct cpufreq_driver amd_pstate_driver; > static struct cpufreq_driver amd_pstate_epp_driver; > static int cppc_state = AMD_PSTATE_DISABLE; > +static bool cppc_enabled; > > /* > * AMD Energy Preference Performance (EPP) > @@ -228,7 +229,28 @@ static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata, > > static inline int pstate_enable(bool enable) > { > - return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable); > + int ret, cpu; > + unsigned long logical_proc_id_mask = 0; > + > + if (enable == cppc_enabled) > + return 0; > + > + for_each_present_cpu(cpu) { > + unsigned long logical_id = topology_logical_die_id(cpu); > + > + if (test_bit(logical_id, &logical_proc_id_mask)) > + continue; > + > + set_bit(logical_id, &logical_proc_id_mask); > + > + ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE, > + enable); Thanks Wyes, that makes a lot more sense to me. The MSR is per package on design. We should only write once per package. Patch is Acked-by: Huang Rui <ray.huang@amd.com> > + if (ret) > + return ret; > + } > + > + cppc_enabled = enable; > + return 0; > } > > static int cppc_enable(bool enable) > @@ -236,6 +258,9 @@ static int cppc_enable(bool enable) > int cpu, ret = 0; > struct cppc_perf_ctrls perf_ctrls; > > + if (enable == cppc_enabled) > + return 0; > + > for_each_present_cpu(cpu) { > ret = cppc_set_enable(cpu, enable); > if (ret) > @@ -251,6 +276,7 @@ static int cppc_enable(bool enable) > } > } > > + cppc_enabled = enable; > return ret; > } > > -- > 2.34.1 >
On Fri, Jun 16, 2023 at 9:03 AM Huang Rui <ray.huang@amd.com> wrote: > > On Tue, May 30, 2023 at 09:13:48PM +0800, Karny, Wyes wrote: > > Currently amd_pstate sets CPPC enable bit in MSR_AMD_CPPC_ENABLE only > > for the CPU where the module_init happened. But MSR_AMD_CPPC_ENABLE is > > per-socket. This causes CPPC enable bit to set for only one socket for > > servers with more than one physical packages. To fix this write > > MSR_AMD_CPPC_ENABLE per-socket. > > > > Also, handle duplicate calls for cppc_enable, because it's called from > > per-policy/per-core callbacks and can result in duplicate MSR writes. > > > > Before the fix: > > amd@amd:~$ sudo rdmsr -a 0xc00102b1 | uniq --count > > 192 0 > > 192 1 > > > > After the fix: > > amd@amd:~$ sudo rdmsr -a 0xc00102b1 | uniq --count > > 384 1 > > > > Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > > Signed-off-by: Wyes Karny <wyes.karny@amd.com> > > --- > > v1 -> v2: > > - Made CPPC enable read/write per-socket > > > > drivers/cpufreq/amd-pstate.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > > index 5a3d4aa0f45a..45b9e359f638 100644 > > --- a/drivers/cpufreq/amd-pstate.c > > +++ b/drivers/cpufreq/amd-pstate.c > > @@ -63,6 +63,7 @@ static struct cpufreq_driver *current_pstate_driver; > > static struct cpufreq_driver amd_pstate_driver; > > static struct cpufreq_driver amd_pstate_epp_driver; > > static int cppc_state = AMD_PSTATE_DISABLE; > > +static bool cppc_enabled; > > > > /* > > * AMD Energy Preference Performance (EPP) > > @@ -228,7 +229,28 @@ static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata, > > > > static inline int pstate_enable(bool enable) > > { > > - return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable); > > + int ret, cpu; > > + unsigned long logical_proc_id_mask = 0; > > + > > + if (enable == cppc_enabled) > > + return 0; > > + > > + for_each_present_cpu(cpu) { > > + unsigned long logical_id = topology_logical_die_id(cpu); > > + > > + if (test_bit(logical_id, &logical_proc_id_mask)) > > + continue; > > + > > + set_bit(logical_id, &logical_proc_id_mask); > > + > > + ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE, > > + enable); > > Thanks Wyes, that makes a lot more sense to me. The MSR is per package on > design. We should only write once per package. > > Patch is > > Acked-by: Huang Rui <ray.huang@amd.com> Applied as 6.5 material, thanks!
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 5a3d4aa0f45a..45b9e359f638 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -63,6 +63,7 @@ static struct cpufreq_driver *current_pstate_driver; static struct cpufreq_driver amd_pstate_driver; static struct cpufreq_driver amd_pstate_epp_driver; static int cppc_state = AMD_PSTATE_DISABLE; +static bool cppc_enabled; /* * AMD Energy Preference Performance (EPP) @@ -228,7 +229,28 @@ static int amd_pstate_set_energy_pref_index(struct amd_cpudata *cpudata, static inline int pstate_enable(bool enable) { - return wrmsrl_safe(MSR_AMD_CPPC_ENABLE, enable); + int ret, cpu; + unsigned long logical_proc_id_mask = 0; + + if (enable == cppc_enabled) + return 0; + + for_each_present_cpu(cpu) { + unsigned long logical_id = topology_logical_die_id(cpu); + + if (test_bit(logical_id, &logical_proc_id_mask)) + continue; + + set_bit(logical_id, &logical_proc_id_mask); + + ret = wrmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_ENABLE, + enable); + if (ret) + return ret; + } + + cppc_enabled = enable; + return 0; } static int cppc_enable(bool enable) @@ -236,6 +258,9 @@ static int cppc_enable(bool enable) int cpu, ret = 0; struct cppc_perf_ctrls perf_ctrls; + if (enable == cppc_enabled) + return 0; + for_each_present_cpu(cpu) { ret = cppc_set_enable(cpu, enable); if (ret) @@ -251,6 +276,7 @@ static int cppc_enable(bool enable) } } + cppc_enabled = enable; return ret; }
Currently amd_pstate sets CPPC enable bit in MSR_AMD_CPPC_ENABLE only for the CPU where the module_init happened. But MSR_AMD_CPPC_ENABLE is per-socket. This causes CPPC enable bit to set for only one socket for servers with more than one physical packages. To fix this write MSR_AMD_CPPC_ENABLE per-socket. Also, handle duplicate calls for cppc_enable, because it's called from per-policy/per-core callbacks and can result in duplicate MSR writes. Before the fix: amd@amd:~$ sudo rdmsr -a 0xc00102b1 | uniq --count 192 0 192 1 After the fix: amd@amd:~$ sudo rdmsr -a 0xc00102b1 | uniq --count 384 1 Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> Signed-off-by: Wyes Karny <wyes.karny@amd.com> --- v1 -> v2: - Made CPPC enable read/write per-socket drivers/cpufreq/amd-pstate.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)