When turning on turbo, if frequency configuration takes effect slowly,
the updated policy->cur may be equal to the frequency configured in
governor->limits(), performance governor will not adjust the frequency,
configured frequency will remain at turbo-freq.
Simplified call stack looks as follows:
cpufreq_register_driver(&cppc_cpufreq_driver)
...
cppc_cpufreq_cpu_init()
cppc_get_perf_caps()
policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
cppc_set_perf(highest_perf) // set highest_perf
policy->cur = cpufreq_driver->get() // if cur == policy->max
cpufreq_init_policy()
...
cpufreq_start_governor() // governor: performance
new_freq = cpufreq_driver->get() // if new_freq == policy->max
if (policy->cur != new_freq)
cpufreq_out_of_sync(policy, new_freq)
...
policy->cur = new_freq
...
policy->governor->limits()
__cpufreq_driver_target(policy->max)
if (policy->cur==target)
// generate error, keep set highest_perf
ret
cppc_set_perf(target)
Fix this by changing highest_perf to nominal_perf in cppc_cpufreq_cpu_init().
Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC")
Signed-off-by: liwei <[email protected]>
---
drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 64420d9cfd1e..db04a82b8a97 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
if (caps->highest_perf > caps->nominal_perf)
boost_supported = true;
- /* Set policy->cur to max now. The governors will adjust later. */
- policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
- cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
+ /* Set policy->cur to norm now. */
+ policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
+ cpu_data->perf_ctrls.desired_perf = caps->nominal_perf;
ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
if (ret) {
pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
- caps->highest_perf, cpu, ret);
+ caps->nominal_perf, cpu, ret);
goto out;
}
--
2.25.1
CC'ing few folks who are working with the driver.
On 28-04-24, 17:28, liwei wrote:
> When turning on turbo, if frequency configuration takes effect slowly,
> the updated policy->cur may be equal to the frequency configured in
> governor->limits(), performance governor will not adjust the frequency,
> configured frequency will remain at turbo-freq.
>
> Simplified call stack looks as follows:
> cpufreq_register_driver(&cppc_cpufreq_driver)
> ...
> cppc_cpufreq_cpu_init()
> cppc_get_perf_caps()
> policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
> cppc_set_perf(highest_perf) // set highest_perf
> policy->cur = cpufreq_driver->get() // if cur == policy->max
> cpufreq_init_policy()
> ...
> cpufreq_start_governor() // governor: performance
> new_freq = cpufreq_driver->get() // if new_freq == policy->max
> if (policy->cur != new_freq)
> cpufreq_out_of_sync(policy, new_freq)
> ...
> policy->cur = new_freq
> ...
> policy->governor->limits()
> __cpufreq_driver_target(policy->max)
> if (policy->cur==target)
> // generate error, keep set highest_perf
> ret
> cppc_set_perf(target)
>
> Fix this by changing highest_perf to nominal_perf in cppc_cpufreq_cpu_init().
>
> Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC")
> Signed-off-by: liwei <[email protected]>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 64420d9cfd1e..db04a82b8a97 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> if (caps->highest_perf > caps->nominal_perf)
> boost_supported = true;
>
> - /* Set policy->cur to max now. The governors will adjust later. */
> - policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> - cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
> + /* Set policy->cur to norm now. */
> + policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
> + cpu_data->perf_ctrls.desired_perf = caps->nominal_perf;
>
> ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
> if (ret) {
> pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
> - caps->highest_perf, cpu, ret);
> + caps->nominal_perf, cpu, ret);
> goto out;
> }
>
> --
> 2.25.1
--
viresh
On Mon, Apr 29, 2024 at 04:19:45PM +0530, Viresh Kumar wrote:
>CC'ing few folks who are working with the driver.
>
>On 28-04-24, 17:28, liwei wrote:
>> When turning on turbo, if frequency configuration takes effect slowly,
>> the updated policy->cur may be equal to the frequency configured in
>> governor->limits(), performance governor will not adjust the frequency,
>> configured frequency will remain at turbo-freq.
>>
>> Simplified call stack looks as follows:
>> cpufreq_register_driver(&cppc_cpufreq_driver)
>> ...
>> cppc_cpufreq_cpu_init()
>> cppc_get_perf_caps()
>> policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
>> cppc_set_perf(highest_perf) // set highest_perf
>> policy->cur = cpufreq_driver->get() // if cur == policy->max
>> cpufreq_init_policy()
>> ...
>> cpufreq_start_governor() // governor: performance
>> new_freq = cpufreq_driver->get() // if new_freq == policy->max
>> if (policy->cur != new_freq)
>> cpufreq_out_of_sync(policy, new_freq)
>> ...
>> policy->cur = new_freq
>> ...
>> policy->governor->limits()
>> __cpufreq_driver_target(policy->max)
>> if (policy->cur==target)
>> // generate error, keep set highest_perf
>> ret
>> cppc_set_perf(target)
>>
>> Fix this by changing highest_perf to nominal_perf in cppc_cpufreq_cpu_init().
>>
>> Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC")
>> Signed-off-by: liwei <[email protected]>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 64420d9cfd1e..db04a82b8a97 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> if (caps->highest_perf > caps->nominal_perf)
>> boost_supported = true;
>>
>> - /* Set policy->cur to max now. The governors will adjust later. */
>> - policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
>> - cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
>> + /* Set policy->cur to norm now. */
>> + policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
>> + cpu_data->perf_ctrls.desired_perf = caps->nominal_perf;
>>
This seems like the safer thing to do.
Reviewed-by: Vanshidhar Konda <[email protected]>
>> ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>> if (ret) {
>> pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>> - caps->highest_perf, cpu, ret);
>> + caps->nominal_perf, cpu, ret);
>> goto out;
>> }
>>
>> --
>> 2.25.1
>
>--
>viresh
Hello Liwei,
The change itself seems ok, but I'm not sure I understand what the
issue is exactly.
On 4/29/24 12:49, Viresh Kumar wrote:
> CC'ing few folks who are working with the driver.
>
> On 28-04-24, 17:28, liwei wrote:
>> When turning on turbo, if frequency configuration takes effect slowly,
>> the updated policy->cur may be equal to the frequency configured in
>> governor->limits(), performance governor will not adjust the frequency,
>> configured frequency will remain at turbo-freq.
>>
>> Simplified call stack looks as follows:
>> cpufreq_register_driver(&cppc_cpufreq_driver)
>> ...
>> cppc_cpufreq_cpu_init()
>> cppc_get_perf_caps()
>> policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
>> cppc_set_perf(highest_perf) // set highest_perf
>> policy->cur = cpufreq_driver->get() // if cur == policy->max
During the driver initialization, we have:
cppc_cpufreq_cpu_init()
\-policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
\-policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
\-cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
\-cppc_set_perf(cpu, &cpu_data->perf_ctrls); // set freq to highest_perf
so here:
policy->max = nominal_freq
policy->cur = highest_freq
And then for the cpufreq framework:
cpufreq_online()
// IIUC there is some delay here, so policy->cur = nominal_freq ?
// i.e. the freq. was requested to change to the highest_freq,
// but the change is not effective yet ?
\-policy->cur = cpufreq_driver->get(policy->cpu);
\-cpufreq_init_policy()
\-cpufreq_set_policy()
\-cpufreq_start_governor()
\-cpufreq_verify_current_freq()
\-new_freq = cpufreq_driver->get(policy->cpu); // new_freq = nominal_freq ?
\-if (policy->cur != new_freq)
\- cpufreq_out_of_sync()
\- policy->cur = new_freq;
\-cpufreq_start_governor()
\-cpufreq_gov_performance_limits()
\-__cpufreq_driver_target(target_freq=policy->max) // with policy->max = nominal_freq ?
\-if (target_freq == policy->cur)
\- // do nothing
I am not sure I understand when you are turning the turbo on with:
# echo 1 > /sys/devices/system/cpu/cpufreq/boost
Or do you mean that turbo is available but not turned on ?
Regards,
Pierre
>> cpufreq_init_policy()
>> ...
>> cpufreq_start_governor() // governor: performance
>> new_freq = cpufreq_driver->get() // if new_freq == policy->max
>> if (policy->cur != new_freq)
>> cpufreq_out_of_sync(policy, new_freq)
>> ...
>> policy->cur = new_freq
>> ...
>> policy->governor->limits()
>> __cpufreq_driver_target(policy->max)
>> if (policy->cur==target)
>> // generate error, keep set highest_perf
>> ret
>> cppc_set_perf(target)
>>
>> Fix this by changing highest_perf to nominal_perf in cppc_cpufreq_cpu_init().
>>
>> Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC")
>> Signed-off-by: liwei <[email protected]>
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 64420d9cfd1e..db04a82b8a97 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> if (caps->highest_perf > caps->nominal_perf)
>> boost_supported = true;
>>
>> - /* Set policy->cur to max now. The governors will adjust later. */
>> - policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
>> - cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
>> + /* Set policy->cur to norm now. */
>> + policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
>> + cpu_data->perf_ctrls.desired_perf = caps->nominal_perf;
>>
>> ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>> if (ret) {
>> pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>> - caps->highest_perf, cpu, ret);
>> + caps->nominal_perf, cpu, ret);
>> goto out;
>> }
>>
>> --
>> 2.25.1
>
在 2024/5/3 22:19, Pierre Gondois 写道:
> Hello Liwei,
> The change itself seems ok, but I'm not sure I understand what the
> issue is exactly.
>
> On 4/29/24 12:49, Viresh Kumar wrote:
>> CC'ing few folks who are working with the driver.
>>
>> On 28-04-24, 17:28, liwei wrote:
>>> When turning on turbo, if frequency configuration takes effect slowly,
>>> the updated policy->cur may be equal to the frequency configured in
>>> governor->limits(), performance governor will not adjust the frequency,
>>> configured frequency will remain at turbo-freq.
>>>
>>> Simplified call stack looks as follows:
>>> cpufreq_register_driver(&cppc_cpufreq_driver)
>>> ...
>>> cppc_cpufreq_cpu_init()
>>> cppc_get_perf_caps()
>>> policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
>>> cppc_set_perf(highest_perf) // set highest_perf
>>> policy->cur = cpufreq_driver->get() // if cur == policy->max
>
> During the driver initialization, we have:
> cppc_cpufreq_cpu_init()
> \-policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
> \-policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> \-cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
> \-cppc_set_perf(cpu, &cpu_data->perf_ctrls); // set freq to highest_perf
> so here:
> policy->max = nominal_freq
> policy->cur = highest_freq
>
>
> And then for the cpufreq framework:
> cpufreq_online()
> // IIUC there is some delay here, so policy->cur = nominal_freq ?
> // i.e. the freq. was requested to change to the highest_freq,
> // but the change is not effective yet ?
> \-policy->cur = cpufreq_driver->get(policy->cpu);
> \-cpufreq_init_policy()
> \-cpufreq_set_policy()
> \-cpufreq_start_governor()
> \-cpufreq_verify_current_freq()
> \-new_freq = cpufreq_driver->get(policy->cpu); // new_freq =
> nominal_freq ?
> \-if (policy->cur != new_freq)
> \- cpufreq_out_of_sync()
> \- policy->cur = new_freq;
> \-cpufreq_start_governor()
> \-cpufreq_gov_performance_limits()
> \-__cpufreq_driver_target(target_freq=policy->max) // with
> policy->max = nominal_freq ?
> \-if (target_freq == policy->cur)
> \- // do nothing
>
> I am not sure I understand when you are turning the turbo on with:
> # echo 1 > /sys/devices/system/cpu/cpufreq/boost
>
> Or do you mean that turbo is available but not turned on ?
>
Sorry, my description is not clear enough. The scenario described above
is during the kernel initialization process, turbo is available but
boost is not turned on.
I found this problem is to read
/sys/devices/system/cpu/cpufreq/policyX/cpuinfo_cur_freq directly after
OS startup, and found that some frequencies are in turbo state and
/sys/devices/system/cpu/cpufreq/boost has not been modified, its value
is still 0.
LiWei
>
> Regards,
> Pierre
>
>>> cpufreq_init_policy()
>>> ...
>>> cpufreq_start_governor() // governor: performance
>>> new_freq = cpufreq_driver->get() // if new_freq ==
>>> policy->max
>>> if (policy->cur != new_freq)
>>> cpufreq_out_of_sync(policy, new_freq)
>>> ...
>>> policy->cur = new_freq
>>> ...
>>> policy->governor->limits()
>>> __cpufreq_driver_target(policy->max)
>>> if (policy->cur==target)
>>> // generate error, keep set highest_perf
>>> ret
>>> cppc_set_perf(target)
>>>
>>> Fix this by changing highest_perf to nominal_perf in
>>> cppc_cpufreq_cpu_init().
>>>
>>> Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with
>>> CPPC")
>>> Signed-off-by: liwei <[email protected]>
>>> ---
>>> drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c
>>> b/drivers/cpufreq/cppc_cpufreq.c
>>> index 64420d9cfd1e..db04a82b8a97 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct
>>> cpufreq_policy *policy)
>>> if (caps->highest_perf > caps->nominal_perf)
>>> boost_supported = true;
>>> - /* Set policy->cur to max now. The governors will adjust later. */
>>> - policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
>>> - cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
>>> + /* Set policy->cur to norm now. */
>>> + policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
>>> + cpu_data->perf_ctrls.desired_perf = caps->nominal_perf;
>>> ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>>> if (ret) {
>>> pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>>> - caps->highest_perf, cpu, ret);
>>> + caps->nominal_perf, cpu, ret);
>>> goto out;
>>> }
>>> --
>>> 2.25.1
>>
Hi,
Thanks for adding me to this.
On Monday 29 Apr 2024 at 16:19:45 (+0530), Viresh Kumar wrote:
> CC'ing few folks who are working with the driver.
>
> On 28-04-24, 17:28, liwei wrote:
> > When turning on turbo, if frequency configuration takes effect slowly,
> > the updated policy->cur may be equal to the frequency configured in
> > governor->limits(), performance governor will not adjust the frequency,
> > configured frequency will remain at turbo-freq.
> >
> > Simplified call stack looks as follows:
> > cpufreq_register_driver(&cppc_cpufreq_driver)
> > ...
> > cppc_cpufreq_cpu_init()
> > cppc_get_perf_caps()
> > policy->max = cppc_perf_to_khz(caps, caps->nominal_perf)
> > cppc_set_perf(highest_perf) // set highest_perf
> > policy->cur = cpufreq_driver->get() // if cur == policy->max
> > cpufreq_init_policy()
> > ...
> > cpufreq_start_governor() // governor: performance
> > new_freq = cpufreq_driver->get() // if new_freq == policy->max
> > if (policy->cur != new_freq)
> > cpufreq_out_of_sync(policy, new_freq)
> > ...
> > policy->cur = new_freq
I believe the problem is here ^^^^^^^^^^^^^^^^^^^^^^.
cpufreq_verify_current_freq() should not update policy->cur unless a
request to change frequency has actually reached the driver. I believe
policy->cur should always reflect the request, not the actual current
frequency of the CPU.
Given that new_freq is the current (hardware) frequency of the CPU,
obtained via .get(), it can be the nominal frequency, as it is in your
case, or any frequency, if there is any firmware/hardware capping in
place.
This causes the issue in your scenario, in which __cpufreq_driver_target()
filters the request from the governor as it finds it equal to policy->cur,
and it believes it's already set by hardware.
This causes another issue in which scaling_cur_freq, which for some
systems returns policy->cur, ends up returning the hardware frequency of
the CPUs, and not the last frequency request, as it should:
"scaling_cur_freq
Current frequency of all of the CPUs belonging to this policy (in kHz).
In the majority of cases, this is the frequency of the last P-state
requested by the scaling driver from the hardware using the scaling
interface provided by it, which may or may not reflect the frequency
the CPU is actually running at (due to hardware design and other
limitations)." [1]
Therefore policy->cur gets polluted with the hardware frequency of the
CPU sampled at that one time, and this affects governor decisions, as
in your case, and scaling_cur_freq feedback as well. This bad value will
not change until there's another .target() or cpufreq_out_of_sync()
call, which will never happen for fixed frequency governors like the
performance governor.
Thanks,
Ionela.
[1] https://docs.kernel.org/admin-guide/pm/cpufreq.html
> > ...
> > policy->governor->limits()
> > __cpufreq_driver_target(policy->max)
> > if (policy->cur==target)
> > // generate error, keep set highest_perf
> > ret
> > cppc_set_perf(target)
> >
> > Fix this by changing highest_perf to nominal_perf in cppc_cpufreq_cpu_init().
> >
> > Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC")
> > Signed-off-by: liwei <[email protected]>
> > ---
> > drivers/cpufreq/cppc_cpufreq.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index 64420d9cfd1e..db04a82b8a97 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -669,14 +669,14 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > if (caps->highest_perf > caps->nominal_perf)
> > boost_supported = true;
> >
> > - /* Set policy->cur to max now. The governors will adjust later. */
> > - policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> > - cpu_data->perf_ctrls.desired_perf = caps->highest_perf;
> > + /* Set policy->cur to norm now. */
> > + policy->cur = cppc_perf_to_khz(caps, caps->nominal_perf);
> > + cpu_data->perf_ctrls.desired_perf = caps->nominal_perf;
> >
> > ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
> > if (ret) {
> > pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
> > - caps->highest_perf, cpu, ret);
> > + caps->nominal_perf, cpu, ret);
> > goto out;
> > }
> >
> > --
> > 2.25.1
>
> --
> viresh