[PATCH v2] cpufreq: Fix per-policy boost behavior after CPU hotplug

[PATCH v2] cpufreq: Fix per-policy boost behavior after CPU hotplug

[PoShao Chen]

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(+)

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()) {
-- 
2.18.0

Re: [PATCH v2] cpufreq: Fix per-policy boost behavior after CPU hotplug

[Viresh Kumar]

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) {

-- 
viresh

Re: [PATCH v2] cpufreq: Fix per-policy boost behavior after CPU hotplug

[PoShao Chen]

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.

-- 
poshao

Re: [PATCH v2] cpufreq: Fix per-policy boost behavior after CPU hotplug

[Viresh Kumar]

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.

-- 
viresh