Re: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

Re: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

[Linux regression tracking #adding (Thorsten Leemhuis)]

On 09.02.24 22:38, Doug Smythies wrote:
> 
> I noticed a regression in the 6.8rc series kernels. Bisecting the kernel pointed to:
> 
> # first bad commit: [9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d]
> sched/cpufreq: Rework schedutil governor performance estimation
> 
> There was previous bisection and suggestion of reversion,
> but I guess it wasn't done in the end. [1]
> 
> The regression: reduced maximum CPU frequency is ignored.
> 

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d
#regzbot title sched/cpufreq: reduced maximum CPU frequency is ignored.
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

[Linux regression tracking #update (Thorsten Leemhuis)]

On 14.02.24 14:42, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> On 09.02.24 22:38, Doug Smythies wrote:
>>
>> I noticed a regression in the 6.8rc series kernels. Bisecting the kernel pointed to:
>>
>> # first bad commit: [9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d]
>> sched/cpufreq: Rework schedutil governor performance estimation
>>
>> There was previous bisection and suggestion of reversion,
>> but I guess it wasn't done in the end. [1]
>>
>> The regression: reduced maximum CPU frequency is ignored.
>>
> 
> Thanks for the report. To be sure the issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
> tracking bot:
> 
> #regzbot ^introduced 9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d
> #regzbot title sched/cpufreq: reduced maximum CPU frequency is ignored.
> #regzbot ignore-activity

#regzbot fix: cpufreq: intel_pstate: fix pstate limits enforcement for
adjust_perf call back
#regzbot monitor:
https://lore.kernel.org/all/20240217213010.2466-1-dsmythies@telus.net/
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

[Linux regression tracking (Thorsten Leemhuis)]

On 16.02.24 14:17, Vincent Guittot wrote:
> On Thu, 15 Feb 2024 at 23:53, Doug Smythies <dsmythies@telus.net> wrote:
>>
>> This email thread appears as if it might be moving away from a regression
>> caused by your commit towards a conclusion that your commit exposed
>> a pre-existing bug in the intel_psate.c code.
> Ok

Well, even in that case it's a regression that must be fixed -- ideally
before 6.8. Did anything happen towards that?

I noticed that Doug send the fix "cpufreq: intel_pstate: fix pstate
limits enforcement for adjust_perf call back":
https://lore.kernel.org/all/20240217213010.2466-1-dsmythies@telus.net/

Is that supposed to fix the problem? Looks a bit like it, but I'm not
totally sure. In that case I'd say it likely should be applied to 6.8,
but Rafael apparently applied it to 6.9.

I'd also say that a Fixes: would be good as well (to ensure that fix is
also backported in case anyone backports 9c0b4bb7f630), but I know that
subsystems handle this differently.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

>> Therefore, I have moved Rafael from the C.C. line to the "to" line and
>> added Srinivas.
>>
>> On 2024.02.14 07:38 Vincent wrote:
>>> On Tue, 13 Feb 2024 at 19:07, Doug Smythies <dsmythies@telus.net> wrote:
>>>> On 2024.02.13 03:27 Vincent wrote:
>>>>> On Sun, 11 Feb 2024 at 17:43, Doug Smythies <dsmythies@telus.net> wrote:
>>>>>> On 2024.02.11 05:36 Vincent wrote:
>>>>>>> On Sat, 10 Feb 2024 at 00:16, Doug Smythies <dsmythies@telus.net> wrote:
>>>>>>>> On 2024.02.09.14:11 Vincent wrote:
>>>>>>>>> On Fri, 9 Feb 2024 at 22:38, Doug Smythies <dsmythies@telus.net> wrote:
>>>>>>>>>>
>>>>>>>>>> I noticed a regression in the 6.8rc series kernels. Bisecting the kernel pointed to:
>>>>>>>>>>
>>>>>>>>>> # first bad commit: [9c0b4bb7f6303c9c4e2e34984c46f5a86478f84d]
>>>>>>>>>> sched/cpufreq: Rework schedutil governor performance estimation
>>>>>>>>>>
>>>>>>>>>> There was previous bisection and suggestion of reversion,
>>>>>>>>>> but I guess it wasn't done in the end. [1]
>>>>>>>>>
>>>>>>>>> This has been fixed with
>>>>>>>>> https://lore.kernel.org/all/170539970061.398.16662091173685476681.tip-bot2@tip-bot2/
>>>>>>>>
>>>>>>>> Okay, thanks. I didn't find that one.
>>>>>>>>
>>>>>>>>>> The regression: reduced maximum CPU frequency is ignored.
>>>>>>
>>>>>> Perhaps I should have said "sometimes ignored".
>>>>>> With a maximum CPU frequency for all CPUs set to 2.4 GHz and
>>>>>> a 100% load on CPU 5, its frequency was sampled 1000 times:
>>>>>> 28.6% of samples were 2.4 GHz.
>>>>>> 71.4% of samples were 4.8 GHz (the max turbo frequency)
>>>>>> The results are highly non-repeatable, for example another sample:
>>>>>> 32.8% of samples were 2.4 GHz.
>>>>>> 76.2% of samples were 4.8 GHz
>>>>>>
>>>>>> Another interesting side note: If load is added to the other CPUs,
>>>>>> the set maximum CPU frequency is enforced.
>>>>>
>>>>> Could you trace cpufreq and pstate ? I'd like to understand how
>>>>> policy->cur can be changed
>>>>> whereas there is this comment in intel_pstate_set_policy():
>>>>>        /*
>>>>>         * policy->cur is never updated with the intel_pstate driver, but it
>>>>>         * is used as a stale frequency value. So, keep it within limits.
>>>>>         */
>>>>>
>>>>> but cpufreq_driver_fast_switch() updates it with the freq returned by
>>>>> intel_cpufreq_fast_switch()
>>>>
>>>> Perhaps I should submit a patch clarifying that comment.
>>>> It is true for the "intel_pstate" CPU frequency scaling driver but not for the
>>>> "intel_cpufreq" CPU frequency scaling driver, also known as the intel_pstate
>>>> driver in passive mode. Sorry for any confusion.
>>>>
>>>> I ran the intel_pstate_tracer.py during the test and do observe many, but
>>>> not all, CPUs requesting pstate 48 when the max is set to 24.
>>>> The calling request seems to always be via "fast_switch" path.
>>>> The root issue here appears to be a limit clamping problem for that path.
>>>
>>> Yes, I came to a similar conclusion as well. Whatever does schedutil
>>> ask for, it should be clamped by  cpu->max|min_perf_ratio.
>>
>> Agreed. And it is not clamping properly under specific conditions.
>>
>>> Do you know if you use fast_switch or adjust_perf call back ?
>>
>> I am not certain, but I think it uses "adjust_perf" call back.
>> I do know for certain that it never takes the
>> "intel_cpufreq_update_pstate" path
>> and always takes the
>> "intel_cpu_freq_adjust_perf" path.
> 
> intel_cpu_freq_adjust_perf is registered as the callback for
> cpufreq->adjust_perf
> 
>>
>> The problem seems to occur when that function is called with:
>> min_perf = 1024
>> target_perf = 1024
>> capacity = 1024
>>
>> Even though cpu->max_perf_ratio is 24, the related HWP MSR,
>> 0x774: IA32_HWP_REQUEST, ends up as 48, 48, 48 for min, max, des.
>>
>> This patch appears to fix the issue (still has my debug code and
>> includes a question):
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index ca94e60e705a..8f88a04a494b 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -2987,12 +2987,22 @@ static void intel_cpufreq_adjust_perf(unsigned int cpunum,
>>         if (min_pstate < cpu->min_perf_ratio)
>>                 min_pstate = cpu->min_perf_ratio;
>>
>> +//     if (min_pstate > cpu->pstate.max_pstate)   /* needed? I don't know */
>> +//             min_pstate = cpu->pstate.max_pstate;
>> +
>> +       if (min_pstate > cpu->max_perf_ratio)
>> +               min_pstate = cpu->max_perf_ratio;
>> +
>>         max_pstate = min(cap_pstate, cpu->max_perf_ratio);
>>         if (max_pstate < min_pstate)
>>                 max_pstate = min_pstate;
>>
>>         target_pstate = clamp_t(int, target_pstate, min_pstate, max_pstate);
>>
>> +       if((max_pstate > 40) || (max_pstate < 7) || (min_pstate < 7) || min_pstate > 40 || target_pstate > 40){
>> +               pr_debug("Doug: t: %d : min %d : max %d : minp %d : maxp %d : mnperf %lu : tgperf %lu : capacity %lu\n", target_pstate, min_pstate, max_pstate, cpu->min_perf_ratio, cpu->max_perf_ratio, min_perf, target_perf, capacity);
>> +       }
>> +
>>         intel_cpufreq_hwp_update(cpu, min_pstate, max_pstate, target_pstate, true);
>>
>>         cpu->pstate.current_pstate = target_pstate;
>>
>> With the patch, I never hit the debug statement if the max CPU frequency is limited to 2.4 GHz,
>> whereas it used to get triggered often.
>> More importantly, the system seems to now behave properly and obey set CPU frequency limits.
>>
>>

Re: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

[Rafael J. Wysocki]

On Sat, Feb 24, 2024 at 2:44 PM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
> On 16.02.24 14:17, Vincent Guittot wrote:
> > On Thu, 15 Feb 2024 at 23:53, Doug Smythies <dsmythies@telus.net> wrote:
> >>
> >> This email thread appears as if it might be moving away from a regression
> >> caused by your commit towards a conclusion that your commit exposed
> >> a pre-existing bug in the intel_psate.c code.
> > Ok
>
> Well, even in that case it's a regression that must be fixed -- ideally
> before 6.8. Did anything happen towards that?
>
> I noticed that Doug send the fix "cpufreq: intel_pstate: fix pstate
> limits enforcement for adjust_perf call back":
> https://lore.kernel.org/all/20240217213010.2466-1-dsmythies@telus.net/
>
> Is that supposed to fix the problem? Looks a bit like it, but I'm not
> totally sure. In that case I'd say it likely should be applied to 6.8,
> but Rafael apparently applied it to 6.9.

This hasn't reached linux-next yet, so I rebased it on top of -rc5 in
order to push it as a 6.8 fix.

> I'd also say that a Fixes: would be good as well (to ensure that fix is
> also backported in case anyone backports 9c0b4bb7f630), but I know that
> subsystems handle this differently.

So I added a Fixes: tag to it, but it points to the original change
that missed the check.

Thanks!

Re: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

[Linux regression tracking (Thorsten Leemhuis)]

On 24.02.24 15:11, Rafael J. Wysocki wrote:
> On Sat, Feb 24, 2024 at 2:44 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
>>
>> On 16.02.24 14:17, Vincent Guittot wrote:
>>> On Thu, 15 Feb 2024 at 23:53, Doug Smythies <dsmythies@telus.net> wrote:
>>>>
>>>> This email thread appears as if it might be moving away from a regression
>>>> caused by your commit towards a conclusion that your commit exposed
>>>> a pre-existing bug in the intel_psate.c code.
>>> Ok
>>
>> Well, even in that case it's a regression that must be fixed -- ideally
>> before 6.8. Did anything happen towards that?
>>
>> I noticed that Doug send the fix "cpufreq: intel_pstate: fix pstate
>> limits enforcement for adjust_perf call back":
>> https://lore.kernel.org/all/20240217213010.2466-1-dsmythies@telus.net/
>>
>> Is that supposed to fix the problem? Looks a bit like it, but I'm not
>> totally sure. In that case I'd say it likely should be applied to 6.8,
>> but Rafael apparently applied it to 6.9.
> 
> This hasn't reached linux-next yet, so I rebased it on top of -rc5 in
> order to push it as a 6.8 fix.

Ahh, great, many thx!

>> I'd also say that a Fixes: would be good as well (to ensure that fix is
>> also backported in case anyone backports 9c0b4bb7f630), but I know that
>> subsystems handle this differently.
> 
> So I added a Fixes: tag to it, but it points to the original change
> that missed the check.

Yeah, that totally works for me as well. Again: many thx!

Ciao, Thorsten

RE: sched/cpufreq: Rework schedutil governor performance estimation - Regression bisected

[Doug Smythies]

On 2024.02.24 06:31 Thorsten wrote:
> On 24.02.24 15:11, Rafael J. Wysocki wrote:
>> On Sat, Feb 24, 2024 at 2:44 PM Linux regression tracking (Thorsten
>>> Leemhuis) <regressions@leemhuis.info> wrote:
>>> On 16.02.24 14:17, Vincent Guittot wrote:
>>>> On Thu, 15 Feb 2024 at 23:53, Doug Smythies <dsmythies@telus.net> wrote:
>>>>>
>>>>> This email thread appears as if it might be moving away from a regression
>>>>> caused by your commit towards a conclusion that your commit exposed
>>>>> a pre-existing bug in the intel_psate.c code.
>>>> Ok
>>>
>>> Well, even in that case it's a regression that must be fixed -- ideally
>>> before 6.8. Did anything happen towards that?
>>>
>>> I noticed that Doug send the fix "cpufreq: intel_pstate: fix pstate
>>> limits enforcement for adjust_perf call back":
>>> https://lore.kernel.org/all/20240217213010.2466-1-dsmythies@telus.net/
>>>
>>> Is that supposed to fix the problem? 

Yes it fixes the preexisting issue exposed by 9c0b4bb7f630.

>>>Looks a bit like it, but I'm not
>>> totally sure. In that case I'd say it likely should be applied to 6.8,
>>> but Rafael apparently applied it to 6.9.
>> 
>> This hasn't reached linux-next yet, so I rebased it on top of -rc5 in
>> order to push it as a 6.8 fix.
>
> Ahh, great, many thx!

Yes, thanks.

>>> I'd also say that a Fixes: would be good as well (to ensure that fix is
>>> also backported in case anyone backports 9c0b4bb7f630), but I know that
>>> subsystems handle this differently.

I left the fixes tag off on purpose, because there was never anything wrong
with 9c0b4bb7f630. Apologies to Vincent for wasting his time, but thanks
for the help finding the actual issue.
 
>> So I added a Fixes: tag to it, but it points to the original change
>> that missed the check.
> Yeah, that totally works for me as well. Again: many thx!

Yes, thanks. A fix tag pointing to the original commit makes sense.

... Doug