Re: [PATCH v6 1/4] cpufreq: Remove per-CPU QoS constraint

[Viresh Kumar]

On 19-03-26, 10:30, Pierre Gondois wrote:
> 
> On 3/18/26 12:13, Viresh Kumar wrote:
> > On 17-03-26, 11:17, Pierre Gondois wrote:
> > > policy->max_freq_req represents the maximum allowed frequency as
> > > requested by the policyX/scaling_max_freq sysfs file. This request
> > > applies to all CPUs of the policy. It is not possible to request
> > > a per-CPU maximum frequency.
> > > 
> > > Thus, the interaction between the policy boost and scaling_max_freq
> > > settings should be handled by adding a boost specific QoS constraint.
> > > This will be handled in the following patches.
> > I don't think the above is required anymore. This patch is removing stale code
> > now which isn't useful anymore. It has nothing to do with a boost specific QOS
> > constraint.
> Yes ok
> > And it would be better to know for sure why this isn't required anymore and
> > which patch exactly fixed this issue.
> > 
> On a kernel based on 1608f0230510~, and replicating the
> process described in the commit message of
> 
> commit 1608f0230510 ("cpufreq: Fix re-boost issue after hotplugging
> a CPU")
> 
> I could not see any issue regarding the values of:
> 
> - policy1/cpuinfo_max_freq
> - policy1/scaling_max_freq

The commit message (of 1608f0230510) is confusing. The issue was discussed
properly in the following thread.

https://lore.kernel.org/all/20250120082723.am7rxujmdvzz4eky@vireshk-i7/

The problem is that policy->max and policy->cpuinfo_max_freq are incorrect after
the sequence mentioned in the commit, while max_freq_req is correct.

I think another commit has fixed that (incorrectly and unintentionally):
commit 6db0f533d320 ("cpufreq: preserve freq_table_sorted across suspend/hibernate")

@@ -1421,9 +1421,12 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
                 * If there is a problem with its frequency table, take it
                 * offline and drop it.
                 */
-               ret = cpufreq_table_validate_and_sort(policy);
-               if (ret)
-                       goto out_offline_policy;
+               if (policy->freq_table_sorted != CPUFREQ_TABLE_SORTED_ASCENDING &&
+                   policy->freq_table_sorted != CPUFREQ_TABLE_SORTED_DESCENDING) {
+                       ret = cpufreq_table_validate_and_sort(policy);
+                       if (ret)
+                               goto out_offline_policy;
+               }

This skipped calling cpufreq_table_validate_and_sort() completely on online and
so max/cpuinfo_max_freq, max_freq_req are all in sync.

That change should be fixed with:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 277884d91913..1f794524a1d9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1427,12 +1427,9 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
                 * If there is a problem with its frequency table, take it
                 * offline and drop it.
                 */
-               if (policy->freq_table_sorted != CPUFREQ_TABLE_SORTED_ASCENDING &&
-                   policy->freq_table_sorted != CPUFREQ_TABLE_SORTED_DESCENDING) {
-                       ret = cpufreq_table_validate_and_sort(policy);
-                       if (ret)
-                               goto out_offline_policy;
-               }
+               ret = cpufreq_table_validate_and_sort(policy);
+               if (ret)
+                       goto out_offline_policy;

                /* related_cpus should at least include policy->cpus. */
                cpumask_copy(policy->related_cpus, policy->cpus);
diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
index 7f251daf03ce..5b364d8da4f9 100644
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -360,6 +360,10 @@ int cpufreq_table_validate_and_sort(struct cpufreq_policy *policy)
        if (policy_has_boost_freq(policy))
                policy->boost_supported = true;

+       if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING ||
+           policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_DESCENDING)
+               return 0;
+
        return set_freq_table_sorted(policy);
 }

-- 
viresh