Re: [PATCH v5 2/6] cpufreq: Add boost_freq_req QoS request

[Rafael J. Wysocki]

On Fri, Mar 13, 2026 at 3:30 PM Pierre Gondois <pierre.gondois@xxxxxxx> wrote:
>
>
> On 3/11/26 14:57, Rafael J. Wysocki wrote:
> > On Wed, Mar 11, 2026 at 2:52 PM Rafael J. Wysocki<rafael@xxxxxxxxxx> wrote:
> >> On Wed, Feb 25, 2026 at 9:49 AM Pierre Gondois<pierre.gondois@xxxxxxx> wrote:
> >>> The Power Management Quality of Service (PM QoS) allows to
> >>> aggregate constraints from multiple entities. It is currently
> >>> used to manage the min/max frequency of a given policy.
> >>>
> >>> Frequency constraints can come for instance from:
> >>> - Thermal framework: acpi_thermal_cpufreq_init()
> >>> - Firmware: _PPC objects: acpi_processor_ppc_init()
> >>> - User: by setting policyX/scaling_[min|max]_freq
> >>> The minimum of the max frequency constraints is used to compute
> >>> the resulting maximum allowed frequency.
> >>>
> >>> When enabling boost frequencies, the same frequency request object
> >>> (policy->max_freq_req) as to handle requests from users is used.
> >>> As a result, when setting:
> >>> - scaling_max_freq
> >>> - boost
> >>> The last sysfs file used overwrites the request from the other
> >>> sysfs file.
> >>>
> >>> To avoid this, create a per-policy boost_freq_req to save the boost
> >>> constraints instead of overwriting the last scaling_max_freq
> >>> constraint.
> >>>
> >>> Signed-off-by: Pierre Gondois<pierre.gondois@xxxxxxx>
> >>> ---
> >>>   drivers/cpufreq/cpufreq.c | 37 ++++++++++++++++++++++++++++++++-----
> >>>   include/linux/cpufreq.h   |  1 +
> >>>   2 files changed, 33 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> >>> index db414c052658b..50467b938668a 100644
> >>> --- a/drivers/cpufreq/cpufreq.c
> >>> +++ b/drivers/cpufreq/cpufreq.c
> >>> @@ -1359,17 +1359,21 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> >>>          /* Cancel any pending policy->update work before freeing the policy. */
> >>>          cancel_work_sync(&policy->update);
> >>>
> >>> -       if (policy->max_freq_req) {
> >>> +       if ((policy->max_freq_req && !policy->boost_supported) ||
> >>> +           policy->boost_freq_req) {
> >> Are you sure?
> >>
> >>>                  /*
> >>> -                * Remove max_freq_req after sending CPUFREQ_REMOVE_POLICY
> >>> -                * notification, since CPUFREQ_CREATE_POLICY notification was
> >>> -                * sent after adding max_freq_req earlier.
> >>> +                * Remove max/boost _freq_req after sending CPUFREQ_REMOVE_POLICY
> >>> +                * notification, since CPUFREQ_CREATE_POLICY notification was sent
> >>> +                * after adding max/boost _freq_req earlier.
> >>>                   */
> >>>                  blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> >>>                                               CPUFREQ_REMOVE_POLICY, policy);
> >>> -               freq_qos_remove_request(policy->max_freq_req);
> >>>          }
> >>>
> >>> +       freq_qos_remove_request(policy->boost_freq_req);
> >>> +       kfree(policy->boost_freq_req);
> >>> +
> >>> +       freq_qos_remove_request(policy->max_freq_req);
> >>>          freq_qos_remove_request(policy->min_freq_req);
> >>>          kfree(policy->min_freq_req);
> >>>
> >>> @@ -1479,6 +1483,29 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
> >>>                          goto out_destroy_policy;
> >>>                  }
> >>>
> >>> +               if (policy->boost_supported) {
> >>> +                       policy->boost_freq_req = kzalloc(sizeof(*policy->boost_freq_req),
> >>> +                                                        GFP_KERNEL);
> >> Instead of doing this, why don't you add 1 to the policy->min_freq_req
> >> allocation size when boost is supported?  You'll destroy the policy if
> >> the allocation fails anyway.
>
>
> Yes right,
> this would make the error handling much simpler to follow.
>
> On the other hand Viresh seems to have acknowledged the logic,

But he has also said "+1" to this one. :-)

> so whatever seems is better to both of you.

I think we agree.