diff mbox series

[v2] cpufreq: intel_pstate: set stale CPU frequency to minimum

Message ID 001d01d9d3a7$71736f50$545a4df0$@telus.net
State New
Headers show
Series [v2] cpufreq: intel_pstate: set stale CPU frequency to minimum | expand

Commit Message

Doug Smythies Aug. 20, 2023, 8:46 p.m. UTC 
The intel_pstate CPU frequency scaling driver does not
use policy->cur and it is 0.
When the CPU frequency is outdated arch_freq_get_on_cpu()
will default to the nominal clock frequency when its call to
cpufreq_quick_getpolicy_cur returns the never updated 0.
Thus, the listed frequency might be outside of currently
set limits. Some users are complaining about the high
reported frequency, albeit stale, when their system is
idle and/or it is above the reduced maximum they have set.

This patch will maintain policy_cur for the intel_pstate
driver at the current minimum CPU frequency.

Reported-by: Yang Jie <yang.jie@linux.intel.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
Signed-off-by: Doug Smythies <dsmythies@telus.net>
---

v1 -> v2:
   * v1 was a completely different approach, programming around
     the issue rather than fixing it at the source.
     reference:
     https://patchwork.kernel.org/project/linux-pm/patch/006901d9be8c$f4439930$dccacb90$@telus.net/
   * v2 does not fix an issue with the intel_cpufreq CPU scaling
     driver (A.K.A. the intel_pstate driver in passive mode) and
     the schedutil CPU frequency scaling governor when HWP is enabled
     where limit changes are not reflected in the stale listed frequencies.
     A fix for that will be some future patch.

---
 drivers/cpufreq/intel_pstate.c | 5 +++++
 1 file changed, 5 insertions(+)

--
2.25.1

Comments

Keyon Jie Aug. 28, 2023, 11:35 p.m. UTC | #1 
On 8/22/23 04:46, Rafael J. Wysocki wrote:
> On Sun, Aug 20, 2023 at 10:46 PM Doug Smythies <dsmythies@telus.net> wrote:
>>
>> The intel_pstate CPU frequency scaling driver does not
>> use policy->cur and it is 0.
>> When the CPU frequency is outdated arch_freq_get_on_cpu()
>> will default to the nominal clock frequency when its call to
>> cpufreq_quick_getpolicy_cur returns the never updated 0.
>> Thus, the listed frequency might be outside of currently
>> set limits. Some users are complaining about the high
>> reported frequency, albeit stale, when their system is
>> idle and/or it is above the reduced maximum they have set.
>>
>> This patch will maintain policy_cur for the intel_pstate
>> driver at the current minimum CPU frequency.
>>
>> Reported-by: Yang Jie <yang.jie@linux.intel.com>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
>> Signed-off-by: Doug Smythies <dsmythies@telus.net>
>> ---
>>
>> v1 -> v2:
>>     * v1 was a completely different approach, programming around
>>       the issue rather than fixing it at the source.
>>       reference:
>>       https://patchwork.kernel.org/project/linux-pm/patch/006901d9be8c$f4439930$dccacb90$@telus.net/
>>     * v2 does not fix an issue with the intel_cpufreq CPU scaling
>>       driver (A.K.A. the intel_pstate driver in passive mode) and
>>       the schedutil CPU frequency scaling governor when HWP is enabled
>>       where limit changes are not reflected in the stale listed frequencies.
>>       A fix for that will be some future patch.
>>
>> ---
>>   drivers/cpufreq/intel_pstate.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 8ca2bce4341a..08284dee583a 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -2609,6 +2609,11 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>                          intel_pstate_clear_update_util_hook(policy->cpu);
>>                  intel_pstate_hwp_set(policy->cpu);
>>          }
>> +       /* policy current is never updated with the intel_pstate driver
>> +        * but it is used as a stale frequency value. So, keep it within
>> +        * limits.
>> +        */
>> +       policy->cur = policy->min;
>>
>>          mutex_unlock(&intel_pstate_limits_lock);
>>
>> --
> 
> Applied as 6.6 material, with some mailer-induced white space damage
> fixed and the new comment adjusted to the kernel coding style.
> 
> Thanks!

Hi Doug and Rafael,

Thank you for making the fix happen.

Hi Greg,

Will this be picked to the stable linux-6.1.y and linux-6.4.y kernel, it 
could benefit to users there.

Thanks,
~Keyon
Greg KH Aug. 29, 2023, 8:57 a.m. UTC | #2 
On Mon, Aug 28, 2023 at 04:35:13PM -0700, Keyon Jie wrote:
> 
> 
> On 8/22/23 04:46, Rafael J. Wysocki wrote:
> > On Sun, Aug 20, 2023 at 10:46 PM Doug Smythies <dsmythies@telus.net> wrote:
> > > 
> > > The intel_pstate CPU frequency scaling driver does not
> > > use policy->cur and it is 0.
> > > When the CPU frequency is outdated arch_freq_get_on_cpu()
> > > will default to the nominal clock frequency when its call to
> > > cpufreq_quick_getpolicy_cur returns the never updated 0.
> > > Thus, the listed frequency might be outside of currently
> > > set limits. Some users are complaining about the high
> > > reported frequency, albeit stale, when their system is
> > > idle and/or it is above the reduced maximum they have set.
> > > 
> > > This patch will maintain policy_cur for the intel_pstate
> > > driver at the current minimum CPU frequency.
> > > 
> > > Reported-by: Yang Jie <yang.jie@linux.intel.com>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
> > > Signed-off-by: Doug Smythies <dsmythies@telus.net>
> > > ---
> > > 
> > > v1 -> v2:
> > >     * v1 was a completely different approach, programming around
> > >       the issue rather than fixing it at the source.
> > >       reference:
> > >       https://patchwork.kernel.org/project/linux-pm/patch/006901d9be8c$f4439930$dccacb90$@telus.net/
> > >     * v2 does not fix an issue with the intel_cpufreq CPU scaling
> > >       driver (A.K.A. the intel_pstate driver in passive mode) and
> > >       the schedutil CPU frequency scaling governor when HWP is enabled
> > >       where limit changes are not reflected in the stale listed frequencies.
> > >       A fix for that will be some future patch.
> > > 
> > > ---
> > >   drivers/cpufreq/intel_pstate.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> > > index 8ca2bce4341a..08284dee583a 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -2609,6 +2609,11 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
> > >                          intel_pstate_clear_update_util_hook(policy->cpu);
> > >                  intel_pstate_hwp_set(policy->cpu);
> > >          }
> > > +       /* policy current is never updated with the intel_pstate driver
> > > +        * but it is used as a stale frequency value. So, keep it within
> > > +        * limits.
> > > +        */
> > > +       policy->cur = policy->min;
> > > 
> > >          mutex_unlock(&intel_pstate_limits_lock);
> > > 
> > > --
> > 
> > Applied as 6.6 material, with some mailer-induced white space damage
> > fixed and the new comment adjusted to the kernel coding style.
> > 
> > Thanks!
> 
> Hi Doug and Rafael,
> 
> Thank you for making the fix happen.
> 
> Hi Greg,
> 
> Will this be picked to the stable linux-6.1.y and linux-6.4.y kernel, it
> could benefit to users there.

Sure, when it hits Linus's tree, please follow the instructions in:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to get it merged.

thanks,

greg k-h
Keyon Jie Sept. 5, 2023, 6:17 p.m. UTC | #3 
On 8/29/23 01:57, Greg KH wrote:
> On Mon, Aug 28, 2023 at 04:35:13PM -0700, Keyon Jie wrote:
>>
>>
>> On 8/22/23 04:46, Rafael J. Wysocki wrote:
>>> On Sun, Aug 20, 2023 at 10:46 PM Doug Smythies <dsmythies@telus.net> wrote:
>>>>
>>>> The intel_pstate CPU frequency scaling driver does not
>>>> use policy->cur and it is 0.
>>>> When the CPU frequency is outdated arch_freq_get_on_cpu()
>>>> will default to the nominal clock frequency when its call to
>>>> cpufreq_quick_getpolicy_cur returns the never updated 0.
>>>> Thus, the listed frequency might be outside of currently
>>>> set limits. Some users are complaining about the high
>>>> reported frequency, albeit stale, when their system is
>>>> idle and/or it is above the reduced maximum they have set.
>>>>
>>>> This patch will maintain policy_cur for the intel_pstate
>>>> driver at the current minimum CPU frequency.
>>>>
>>>> Reported-by: Yang Jie <yang.jie@linux.intel.com>
>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
>>>> Signed-off-by: Doug Smythies <dsmythies@telus.net>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>      * v1 was a completely different approach, programming around
>>>>        the issue rather than fixing it at the source.
>>>>        reference:
>>>>        https://patchwork.kernel.org/project/linux-pm/patch/006901d9be8c$f4439930$dccacb90$@telus.net/
>>>>      * v2 does not fix an issue with the intel_cpufreq CPU scaling
>>>>        driver (A.K.A. the intel_pstate driver in passive mode) and
>>>>        the schedutil CPU frequency scaling governor when HWP is enabled
>>>>        where limit changes are not reflected in the stale listed frequencies.
>>>>        A fix for that will be some future patch.
>>>>
>>>> ---
>>>>    drivers/cpufreq/intel_pstate.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>>> index 8ca2bce4341a..08284dee583a 100644
>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>> @@ -2609,6 +2609,11 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>>>                           intel_pstate_clear_update_util_hook(policy->cpu);
>>>>                   intel_pstate_hwp_set(policy->cpu);
>>>>           }
>>>> +       /* policy current is never updated with the intel_pstate driver
>>>> +        * but it is used as a stale frequency value. So, keep it within
>>>> +        * limits.
>>>> +        */
>>>> +       policy->cur = policy->min;
>>>>
>>>>           mutex_unlock(&intel_pstate_limits_lock);
>>>>
>>>> --
>>>
>>> Applied as 6.6 material, with some mailer-induced white space damage
>>> fixed and the new comment adjusted to the kernel coding style.
>>>
>>> Thanks!
>>
>> Hi Doug and Rafael,
>>
>> Thank you for making the fix happen.
>>
>> Hi Greg,
>>
>> Will this be picked to the stable linux-6.1.y and linux-6.4.y kernel, it
>> could benefit to users there.
> 
> Sure, when it hits Linus's tree, please follow the instructions in:
>      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to get it merged.

Thank you Greg.

Hi Rafael,

As the issue happens from the 5.18 onward kernels, we need the fix to be 
picked for stable kernels after that, could you please help to add "Cc: 
stable@vger.kernel.org" in the sign-off area when you send it to the 
mainline?

Thank you,
~Keyon

> 
> thanks,
> 
> greg k-h
Rafael J. Wysocki Sept. 5, 2023, 6:30 p.m. UTC | #4 
On Tue, Sep 5, 2023 at 8:17 PM Keyon Jie <yang.jie@linux.intel.com> wrote:
>
>
>
> On 8/29/23 01:57, Greg KH wrote:
> > On Mon, Aug 28, 2023 at 04:35:13PM -0700, Keyon Jie wrote:
> >>
> >>
> >> On 8/22/23 04:46, Rafael J. Wysocki wrote:
> >>> On Sun, Aug 20, 2023 at 10:46 PM Doug Smythies <dsmythies@telus.net> wrote:
> >>>>
> >>>> The intel_pstate CPU frequency scaling driver does not
> >>>> use policy->cur and it is 0.
> >>>> When the CPU frequency is outdated arch_freq_get_on_cpu()
> >>>> will default to the nominal clock frequency when its call to
> >>>> cpufreq_quick_getpolicy_cur returns the never updated 0.
> >>>> Thus, the listed frequency might be outside of currently
> >>>> set limits. Some users are complaining about the high
> >>>> reported frequency, albeit stale, when their system is
> >>>> idle and/or it is above the reduced maximum they have set.
> >>>>
> >>>> This patch will maintain policy_cur for the intel_pstate
> >>>> driver at the current minimum CPU frequency.
> >>>>
> >>>> Reported-by: Yang Jie <yang.jie@linux.intel.com>
> >>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
> >>>> Signed-off-by: Doug Smythies <dsmythies@telus.net>
> >>>> ---
> >>>>
> >>>> v1 -> v2:
> >>>>      * v1 was a completely different approach, programming around
> >>>>        the issue rather than fixing it at the source.
> >>>>        reference:
> >>>>        https://patchwork.kernel.org/project/linux-pm/patch/006901d9be8c$f4439930$dccacb90$@telus.net/
> >>>>      * v2 does not fix an issue with the intel_cpufreq CPU scaling
> >>>>        driver (A.K.A. the intel_pstate driver in passive mode) and
> >>>>        the schedutil CPU frequency scaling governor when HWP is enabled
> >>>>        where limit changes are not reflected in the stale listed frequencies.
> >>>>        A fix for that will be some future patch.
> >>>>
> >>>> ---
> >>>>    drivers/cpufreq/intel_pstate.c | 5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> >>>> index 8ca2bce4341a..08284dee583a 100644
> >>>> --- a/drivers/cpufreq/intel_pstate.c
> >>>> +++ b/drivers/cpufreq/intel_pstate.c
> >>>> @@ -2609,6 +2609,11 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
> >>>>                           intel_pstate_clear_update_util_hook(policy->cpu);
> >>>>                   intel_pstate_hwp_set(policy->cpu);
> >>>>           }
> >>>> +       /* policy current is never updated with the intel_pstate driver
> >>>> +        * but it is used as a stale frequency value. So, keep it within
> >>>> +        * limits.
> >>>> +        */
> >>>> +       policy->cur = policy->min;
> >>>>
> >>>>           mutex_unlock(&intel_pstate_limits_lock);
> >>>>
> >>>> --
> >>>
> >>> Applied as 6.6 material, with some mailer-induced white space damage
> >>> fixed and the new comment adjusted to the kernel coding style.
> >>>
> >>> Thanks!
> >>
> >> Hi Doug and Rafael,
> >>
> >> Thank you for making the fix happen.
> >>
> >> Hi Greg,
> >>
> >> Will this be picked to the stable linux-6.1.y and linux-6.4.y kernel, it
> >> could benefit to users there.
> >
> > Sure, when it hits Linus's tree, please follow the instructions in:
> >      https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > for how to get it merged.
>
> Thank you Greg.
>
> Hi Rafael,
>
> As the issue happens from the 5.18 onward kernels, we need the fix to be
> picked for stable kernels after that, could you please help to add "Cc:
> stable@vger.kernel.org" in the sign-off area when you send it to the
> mainline?

It's already merged, as commit d51847acb018 ("cpufreq: intel_pstate:
set stale CPU frequency to minimum").

Please feel free to send an inclusion request for it to stable@vger.kernel.org

Thanks!
Keyon Jie Sept. 5, 2023, 10:23 p.m. UTC | #5 
On 9/5/23 11:30, Rafael J. Wysocki wrote:
> On Tue, Sep 5, 2023 at 8:17 PM Keyon Jie <yang.jie@linux.intel.com> wrote:
>>
>>
>>
>> On 8/29/23 01:57, Greg KH wrote:
>>> On Mon, Aug 28, 2023 at 04:35:13PM -0700, Keyon Jie wrote:
>>>>
>>>>
>>>> On 8/22/23 04:46, Rafael J. Wysocki wrote:
>>>>> On Sun, Aug 20, 2023 at 10:46 PM Doug Smythies <dsmythies@telus.net> wrote:
>>>>>>
>>>>>> The intel_pstate CPU frequency scaling driver does not
>>>>>> use policy->cur and it is 0.
>>>>>> When the CPU frequency is outdated arch_freq_get_on_cpu()
>>>>>> will default to the nominal clock frequency when its call to
>>>>>> cpufreq_quick_getpolicy_cur returns the never updated 0.
>>>>>> Thus, the listed frequency might be outside of currently
>>>>>> set limits. Some users are complaining about the high
>>>>>> reported frequency, albeit stale, when their system is
>>>>>> idle and/or it is above the reduced maximum they have set.
>>>>>>
>>>>>> This patch will maintain policy_cur for the intel_pstate
>>>>>> driver at the current minimum CPU frequency.
>>>>>>
>>>>>> Reported-by: Yang Jie <yang.jie@linux.intel.com>
>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217597
>>>>>> Signed-off-by: Doug Smythies <dsmythies@telus.net>
>>>>>> ---
>>>>>>
>>>>>> v1 -> v2:
>>>>>>       * v1 was a completely different approach, programming around
>>>>>>         the issue rather than fixing it at the source.
>>>>>>         reference:
>>>>>>         https://patchwork.kernel.org/project/linux-pm/patch/006901d9be8c$f4439930$dccacb90$@telus.net/
>>>>>>       * v2 does not fix an issue with the intel_cpufreq CPU scaling
>>>>>>         driver (A.K.A. the intel_pstate driver in passive mode) and
>>>>>>         the schedutil CPU frequency scaling governor when HWP is enabled
>>>>>>         where limit changes are not reflected in the stale listed frequencies.
>>>>>>         A fix for that will be some future patch.
>>>>>>
>>>>>> ---
>>>>>>     drivers/cpufreq/intel_pstate.c | 5 +++++
>>>>>>     1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>>>>> index 8ca2bce4341a..08284dee583a 100644
>>>>>> --- a/drivers/cpufreq/intel_pstate.c
>>>>>> +++ b/drivers/cpufreq/intel_pstate.c
>>>>>> @@ -2609,6 +2609,11 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>>>>>                            intel_pstate_clear_update_util_hook(policy->cpu);
>>>>>>                    intel_pstate_hwp_set(policy->cpu);
>>>>>>            }
>>>>>> +       /* policy current is never updated with the intel_pstate driver
>>>>>> +        * but it is used as a stale frequency value. So, keep it within
>>>>>> +        * limits.
>>>>>> +        */
>>>>>> +       policy->cur = policy->min;
>>>>>>
>>>>>>            mutex_unlock(&intel_pstate_limits_lock);
>>>>>>
>>>>>> --
>>>>>
>>>>> Applied as 6.6 material, with some mailer-induced white space damage
>>>>> fixed and the new comment adjusted to the kernel coding style.
>>>>>
>>>>> Thanks!
>>>>
>>>> Hi Doug and Rafael,
>>>>
>>>> Thank you for making the fix happen.
>>>>
>>>> Hi Greg,
>>>>
>>>> Will this be picked to the stable linux-6.1.y and linux-6.4.y kernel, it
>>>> could benefit to users there.
>>>
>>> Sure, when it hits Linus's tree, please follow the instructions in:
>>>       https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>> for how to get it merged.
>>
>> Thank you Greg.
>>
>> Hi Rafael,
>>
>> As the issue happens from the 5.18 onward kernels, we need the fix to be
>> picked for stable kernels after that, could you please help to add "Cc:
>> stable@vger.kernel.org" in the sign-off area when you send it to the
>> mainline?
> 
> It's already merged, as commit d51847acb018 ("cpufreq: intel_pstate:
> set stale CPU frequency to minimum").
> 
> Please feel free to send an inclusion request for it to stable@vger.kernel.org

Thank you Rafael, will do.

Thanks,
~Keyon

> 
> Thanks!
diff mbox series

Patch

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 8ca2bce4341a..08284dee583a 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2609,6 +2609,11 @@  static int intel_pstate_set_policy(struct cpufreq_policy *policy)
                        intel_pstate_clear_update_util_hook(policy->cpu);
                intel_pstate_hwp_set(policy->cpu);
        }
+       /* policy current is never updated with the intel_pstate driver
+        * but it is used as a stale frequency value. So, keep it within
+        * limits.
+        */
+       policy->cur = policy->min;

        mutex_unlock(&intel_pstate_limits_lock);