| Message ID | 20240612033112.29343-1-poshao.chen@mediatek.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2] cpufreq: Fix per-policy boost behavior after CPU hotplug | expand |
On 12-06-24, 11:31, PoShao Chen wrote: > This patch fixes the behavior of the cpufreq boost when the > global boost flag is toggled during CPU hotplug offline. This action > previously led to incorrect scaling_max_freq values when the CPU was > brought back online. The issue also manifested as incorrect > scaling_cur_freq under the performance governor. > > For example, after the following operations, even if the global boost > is disabled, the resulting scaling_max_freq and scaling_cur_freq > will still reflect the settings of an enabled boost. > > $ echo performance > /sys/devices/system/cpu/cpufreq/policy7/scaling_governor > $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_cur_freq > 3200000 > $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_max_freq > 3200000 > > $ echo 1 > /sys/devices/system/cpu/cpufreq/boost > $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_cur_freq > 3250000 > $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_max_freq > 3250000 > > $ echo 0 > /sys/devices/system/cpu/cpu7/online > $ echo 0 > /sys/devices/system/cpu/cpufreq/boost > $ echo 1 > /sys/devices/system/cpu/cpu7/online > $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_cur_freq > 3250000 > $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_max_freq > 3250000 Please try this instead: diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 7c6879efe9ef..bd9fe2b0f032 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -43,6 +43,9 @@ static LIST_HEAD(cpufreq_policy_list); #define for_each_inactive_policy(__policy) \ for_each_suitable_policy(__policy, false) +#define for_each_policy(__policy) \ + list_for_each_entry(__policy, &cpufreq_policy_list, policy_list) + /* Iterate over governors */ static LIST_HEAD(cpufreq_governor_list); #define for_each_governor(__governor) \ @@ -2815,7 +2818,7 @@ int cpufreq_boost_trigger_state(int state) write_unlock_irqrestore(&cpufreq_driver_lock, flags); cpus_read_lock(); - for_each_active_policy(policy) { + for_each_policy(policy) { policy->boost_enabled = state; ret = cpufreq_driver->set_boost(policy, state); if (ret) {
On Thu, 2024-06-13 at 14:50 +0530, Viresh Kumar wrote: > Please try this instead: > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 7c6879efe9ef..bd9fe2b0f032 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -43,6 +43,9 @@ static LIST_HEAD(cpufreq_policy_list); > #define for_each_inactive_policy(__policy) \ > for_each_suitable_policy(__policy, false) > > +#define for_each_policy(__policy) \ > + list_for_each_entry(__policy, &cpufreq_policy_list, > policy_list) > + > /* Iterate over governors */ > static LIST_HEAD(cpufreq_governor_list); > #define for_each_governor(__governor) \ > @@ -2815,7 +2818,7 @@ int cpufreq_boost_trigger_state(int state) > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > cpus_read_lock(); > - for_each_active_policy(policy) { > + for_each_policy(policy) { > policy->boost_enabled = state; > ret = cpufreq_driver->set_boost(policy, state); > if (ret) { > > -- > viresh Thank you for the suggestion. However, calling ->set_boost when the policy is inactive will fail in two ways: 1. policy->freq_table will be NULL. 2. freq_qos_update_request will fail to refresh the frequency limit.
On 17-06-24, 13:48, PoShao Chen wrote: > On Thu, 2024-06-13 at 14:50 +0530, Viresh Kumar wrote: > > Please try this instead: > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 7c6879efe9ef..bd9fe2b0f032 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -43,6 +43,9 @@ static LIST_HEAD(cpufreq_policy_list); > > #define for_each_inactive_policy(__policy) \ > > for_each_suitable_policy(__policy, false) > > > > +#define for_each_policy(__policy) \ > > + list_for_each_entry(__policy, &cpufreq_policy_list, > > policy_list) > > + > > /* Iterate over governors */ > > static LIST_HEAD(cpufreq_governor_list); > > #define for_each_governor(__governor) \ > > @@ -2815,7 +2818,7 @@ int cpufreq_boost_trigger_state(int state) > > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > > > cpus_read_lock(); > > - for_each_active_policy(policy) { > > + for_each_policy(policy) { > > policy->boost_enabled = state; > > ret = cpufreq_driver->set_boost(policy, state); > > if (ret) { > > Thank you for the suggestion. However, calling ->set_boost when > the policy is inactive will fail in two ways: > > 1. policy->freq_table will be NULL. > 2. freq_qos_update_request will fail to refresh the frequency limit. What about just this then ? diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a45aac17c20f..8e92ba7dda4b 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1430,9 +1430,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 */ - policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); - /* * The initialization has succeeded and the policy is online. * If there is a problem with its frequency table, take it @@ -1446,6 +1443,9 @@ static int cpufreq_online(unsigned int cpu) cpumask_copy(policy->related_cpus, policy->cpus); } + /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */ + policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy); + /* * affected cpus must always be the one, which are online. We aren't * managing offline cpus here. So the cpufreq core sets the policy boost to whatever the global boost is at the time CPU comes online. This won't call cppc_cpufreq_set_boost() (I think that's the driver you are using ?) though. The freq_qos_update_request() thing we do there is driver specific and the driver itself needs to take care of this, perhaps in the online() callback.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a45aac17c20f..faadae05bc8a 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1495,6 +1495,35 @@ static int cpufreq_online(unsigned int cpu) blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_CREATE_POLICY, policy); + } else { + /* + * Call freq_qos_update_request() for the per-policy boost flag mirror + * the cpufreq_driver boost during hotplug online. + * Register an online callback if the default mirroring of the global + * boost setting is not intended. + */ + if (!cpufreq_driver->online) { + ret = freq_qos_update_request(policy->max_freq_req, policy->max); + if (ret) + pr_err("%s: freq qos update failed\n", __func__); + } else { + /* + * Let the per-policy boost flag mirror the cpufreq_driver + * boost if an illegal state occurs after hotplug + */ + if (policy->boost_enabled && !cpufreq_driver->boost_enabled) { + pr_info("%s: local boost flag mirror the global boost\n", + __func__); + policy->boost_enabled = cpufreq_driver->boost_enabled; + ret = cpufreq_driver->set_boost(policy, + cpufreq_driver->boost_enabled); + if (ret) { + policy->boost_enabled = !policy->boost_enabled; + pr_err("%s: Failed to mirror the global boost flag\n", + __func__); + } + } + } } if (cpufreq_driver->get && has_target()) {
This patch fixes the behavior of the cpufreq boost when the global boost flag is toggled during CPU hotplug offline. This action previously led to incorrect scaling_max_freq values when the CPU was brought back online. The issue also manifested as incorrect scaling_cur_freq under the performance governor. For example, after the following operations, even if the global boost is disabled, the resulting scaling_max_freq and scaling_cur_freq will still reflect the settings of an enabled boost. $ echo performance > /sys/devices/system/cpu/cpufreq/policy7/scaling_governor $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_cur_freq 3200000 $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_max_freq 3200000 $ echo 1 > /sys/devices/system/cpu/cpufreq/boost $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_cur_freq 3250000 $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_max_freq 3250000 $ echo 0 > /sys/devices/system/cpu/cpu7/online $ echo 0 > /sys/devices/system/cpu/cpufreq/boost $ echo 1 > /sys/devices/system/cpu/cpu7/online $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_cur_freq 3250000 $ cat /sys/devices/system/cpu/cpufreq/policy7/scaling_max_freq 3250000 Signed-off-by: PoShao Chen <poshao.chen@mediatek.com> --- V1 -> V2: Adjusted log messages and fixed build error. Link: https://lore.kernel.org/all/20240611115920.28665-1-poshao.chen@mediatek.com/ --- drivers/cpufreq/cpufreq.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)