| Message ID | 20250117101457.1530653-3-zhenglifeng1@huawei.com (mailing list archive) |
|---|---|
| State | Queued |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | cpufreq: Fix some boost errors related to CPU online and offline. | expand |
On 17-01-25, 18:14, Lifeng Zheng wrote: > In cpufreq_online() of cpufreq.c, the per-policy boost flag is already set > to mirror the cpufreq_driver boost during init but using freq_table to > judge if the policy has boost frequency. There are two drawbacks to this > approach: > > 1. It doesn't work for the cpufreq drivers that do not use a frequency > table. For now, acpi-cpufreq and amd-pstate have to enable boost in policy > initialization. And cppc_cpufreq never set policy to boost when going > online no matter what the cpufreq_driver boost flag is. > > 2. If the cpu goes offline when cpufreq_driver boost enabled and then goes > online when cpufreq_driver boost disabled, the per-policy boost flag will > unreasonably remain true. > > Running set_boost at the end of the online process is a more generic way > for all cpufreq drivers. > > Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> > --- > drivers/cpufreq/cpufreq.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 5882d7f5e3c1..5a3566c2eb8d 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1409,10 +1409,6 @@ static int cpufreq_online(unsigned int cpu) > goto out_free_policy; > } > > - /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > - if (cpufreq_boost_enabled() && policy_has_boost_freq(policy)) > - policy->boost_enabled = true; > - > /* > * The initialization has succeeded and the policy is online. > * If there is a problem with its frequency table, take it > @@ -1573,6 +1569,18 @@ static int cpufreq_online(unsigned int cpu) > if (new_policy && cpufreq_thermal_control_enabled(cpufreq_driver)) > policy->cdev = of_cpufreq_cooling_register(policy); > > + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > + if (policy->boost_enabled != cpufreq_boost_enabled()) { > + policy->boost_enabled = cpufreq_boost_enabled(); > + ret = cpufreq_driver->set_boost(policy, policy->boost_enabled); I though you agreed to do some optimization here ? > + if (ret) { > + /* If the set_boost fails, the online operation is not affected */ > + pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, > + policy->boost_enabled ? "enable" : "disable"); > + policy->boost_enabled = !policy->boost_enabled; > + } > + } > + > pr_debug("initialization complete\n"); > > return 0; > -- > 2.33.0
On 2025/1/20 17:01, Viresh Kumar wrote: > On 17-01-25, 18:14, Lifeng Zheng wrote: >> In cpufreq_online() of cpufreq.c, the per-policy boost flag is already set >> to mirror the cpufreq_driver boost during init but using freq_table to >> judge if the policy has boost frequency. There are two drawbacks to this >> approach: >> >> 1. It doesn't work for the cpufreq drivers that do not use a frequency >> table. For now, acpi-cpufreq and amd-pstate have to enable boost in policy >> initialization. And cppc_cpufreq never set policy to boost when going >> online no matter what the cpufreq_driver boost flag is. >> >> 2. If the cpu goes offline when cpufreq_driver boost enabled and then goes >> online when cpufreq_driver boost disabled, the per-policy boost flag will >> unreasonably remain true. >> >> Running set_boost at the end of the online process is a more generic way >> for all cpufreq drivers. >> >> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> >> --- >> drivers/cpufreq/cpufreq.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index 5882d7f5e3c1..5a3566c2eb8d 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -1409,10 +1409,6 @@ static int cpufreq_online(unsigned int cpu) >> goto out_free_policy; >> } >> >> - /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ >> - if (cpufreq_boost_enabled() && policy_has_boost_freq(policy)) >> - policy->boost_enabled = true; >> - >> /* >> * The initialization has succeeded and the policy is online. >> * If there is a problem with its frequency table, take it >> @@ -1573,6 +1569,18 @@ static int cpufreq_online(unsigned int cpu) >> if (new_policy && cpufreq_thermal_control_enabled(cpufreq_driver)) >> policy->cdev = of_cpufreq_cooling_register(policy); >> >> + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ >> + if (policy->boost_enabled != cpufreq_boost_enabled()) { >> + policy->boost_enabled = cpufreq_boost_enabled(); >> + ret = cpufreq_driver->set_boost(policy, policy->boost_enabled); > > I though you agreed to do some optimization here ? Sorry. Do I miss something here? > >> + if (ret) { >> + /* If the set_boost fails, the online operation is not affected */ >> + pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, >> + policy->boost_enabled ? "enable" : "disable"); >> + policy->boost_enabled = !policy->boost_enabled; >> + } >> + } >> + >> pr_debug("initialization complete\n"); >> >> return 0; >> -- >> 2.33.0 >
On 21-01-25, 09:45, zhenglifeng (A) wrote: > On 2025/1/20 17:01, Viresh Kumar wrote: > > On 17-01-25, 18:14, Lifeng Zheng wrote: > >> + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > >> + if (policy->boost_enabled != cpufreq_boost_enabled()) { > >> + policy->boost_enabled = cpufreq_boost_enabled(); > >> + ret = cpufreq_driver->set_boost(policy, policy->boost_enabled); > > > > I though you agreed to do some optimization here ? > > Sorry. Do I miss something here? https://lore.kernel.org/all/17c7ed77-21f1-4093-91fc-f3eaa863d312@huawei.com/
On 2025/1/21 12:20, Viresh Kumar wrote: > On 21-01-25, 09:45, zhenglifeng (A) wrote: >> On 2025/1/20 17:01, Viresh Kumar wrote: >>> On 17-01-25, 18:14, Lifeng Zheng wrote: >>>> + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ >>>> + if (policy->boost_enabled != cpufreq_boost_enabled()) { >>>> + policy->boost_enabled = cpufreq_boost_enabled(); >>>> + ret = cpufreq_driver->set_boost(policy, policy->boost_enabled); >>> >>> I though you agreed to do some optimization here ? >> >> Sorry. Do I miss something here? > > https://lore.kernel.org/all/17c7ed77-21f1-4093-91fc-f3eaa863d312@huawei.com/ > I think I already done that, isn't it?
On 21-01-25, 14:22, zhenglifeng (A) wrote: > On 2025/1/21 12:20, Viresh Kumar wrote: > > > On 21-01-25, 09:45, zhenglifeng (A) wrote: > >> On 2025/1/20 17:01, Viresh Kumar wrote: > >>> On 17-01-25, 18:14, Lifeng Zheng wrote: > >>>> + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ > >>>> + if (policy->boost_enabled != cpufreq_boost_enabled()) { > >>>> + policy->boost_enabled = cpufreq_boost_enabled(); > >>>> + ret = cpufreq_driver->set_boost(policy, policy->boost_enabled); > >>> > >>> I though you agreed to do some optimization here ? > >> > >> Sorry. Do I miss something here? > > > > https://lore.kernel.org/all/17c7ed77-21f1-4093-91fc-f3eaa863d312@huawei.com/ > > > > I think I already done that, isn't it? And I misread /facepalm .
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5882d7f5e3c1..5a3566c2eb8d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1409,10 +1409,6 @@ static int cpufreq_online(unsigned int cpu) goto out_free_policy; } - /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ - if (cpufreq_boost_enabled() && policy_has_boost_freq(policy)) - policy->boost_enabled = true; - /* * The initialization has succeeded and the policy is online. * If there is a problem with its frequency table, take it @@ -1573,6 +1569,18 @@ static int cpufreq_online(unsigned int cpu) if (new_policy && cpufreq_thermal_control_enabled(cpufreq_driver)) policy->cdev = of_cpufreq_cooling_register(policy); + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ + if (policy->boost_enabled != cpufreq_boost_enabled()) { + policy->boost_enabled = cpufreq_boost_enabled(); + ret = cpufreq_driver->set_boost(policy, policy->boost_enabled); + if (ret) { + /* If the set_boost fails, the online operation is not affected */ + pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__, policy->cpu, + policy->boost_enabled ? "enable" : "disable"); + policy->boost_enabled = !policy->boost_enabled; + } + } + pr_debug("initialization complete\n"); return 0;
In cpufreq_online() of cpufreq.c, the per-policy boost flag is already set to mirror the cpufreq_driver boost during init but using freq_table to judge if the policy has boost frequency. There are two drawbacks to this approach: 1. It doesn't work for the cpufreq drivers that do not use a frequency table. For now, acpi-cpufreq and amd-pstate have to enable boost in policy initialization. And cppc_cpufreq never set policy to boost when going online no matter what the cpufreq_driver boost flag is. 2. If the cpu goes offline when cpufreq_driver boost enabled and then goes online when cpufreq_driver boost disabled, the per-policy boost flag will unreasonably remain true. Running set_boost at the end of the online process is a more generic way for all cpufreq drivers. Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com> --- drivers/cpufreq/cpufreq.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)