* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-22 20:40 ` [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula Giovanni Gherdovich
@ 2021-01-25 10:04 ` Peter Zijlstra
2021-01-26 9:28 ` Giovanni Gherdovich
2021-02-02 18:40 ` Rafael J. Wysocki
2021-01-25 10:06 ` Peter Zijlstra
` (2 subsequent siblings)
3 siblings, 2 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-01-25 10:04 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Borislav Petkov, Ingo Molnar, Rafael J . Wysocki, Viresh Kumar,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> This workload is constant in time, so instead of using the PELT sum we can
> pretend that scale invariance is obtained with
>
> util_inv = util_raw * freq_curr / freq_max1 [formula-1]
>
> where util_raw is the PELT util from v5.10 (which is to say, not invariant),
> and util_inv is the PELT util from v5.11-rc4. freq_max1 comes from
> commit 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for
> frequency invariance on AMD EPYC") and is (P0+max_boost)/2 = (2.25+3.4)/2 =
> 2.825 GHz. Then we have the schedutil formula
>
> freq_next = 1.25 * freq_max2 * util_inv [formula-2]
>
> Here v5.11-rc4 uses freq_max2 = P0 = 2.25 GHz (and this patch changes it to
> 3.4 GHz).
>
> Since all cores are busy, there is no boost available. Let's be generous and say
> the tasks initially get P0, i.e. freq_curr = 2.25 GHz. Combining the formulas
> above and taking util_raw = 825/1024 = 0.8, freq_next is:
>
> freq_next = 1.25 * 2.25 * 0.8 * 2.25 / 2.825 = 1.79 GHz
Right, so here's a 'problem' between schedutil and cpufreq, they don't
use the same f_max at all times.
And this is also an inconsistency between acpi_cpufreq and intel_pstate
(passive). IIRC the intel_pstate cpufreq drivers uses 4C/1C/P0 resp,
while ACPI seems to stick to P0 f_max.
Rafael; should ACPI change that behaviour rather than adding yet another
magic variable?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-25 10:04 ` Peter Zijlstra
@ 2021-01-26 9:28 ` Giovanni Gherdovich
2021-01-26 10:02 ` Peter Zijlstra
2021-02-02 18:45 ` Rafael J. Wysocki
2021-02-02 18:40 ` Rafael J. Wysocki
1 sibling, 2 replies; 26+ messages in thread
From: Giovanni Gherdovich @ 2021-01-26 9:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Borislav Petkov, Ingo Molnar, Rafael J . Wysocki, Viresh Kumar,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
On Mon, 2021-01-25 at 11:04 +0100, Peter Zijlstra wrote:
> On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > This workload is constant in time, so instead of using the PELT sum we can
> > pretend that scale invariance is obtained with
> >
> > util_inv = util_raw * freq_curr / freq_max1 [formula-1]
> >
> > where util_raw is the PELT util from v5.10 (which is to say, not invariant),
> > and util_inv is the PELT util from v5.11-rc4. freq_max1 comes from
> > commit 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for
> > frequency invariance on AMD EPYC") and is (P0+max_boost)/2 = (2.25+3.4)/2 =
> > 2.825 GHz. Then we have the schedutil formula
> >
> > freq_next = 1.25 * freq_max2 * util_inv [formula-2]
> >
> > Here v5.11-rc4 uses freq_max2 = P0 = 2.25 GHz (and this patch changes it to
> > 3.4 GHz).
> >
> > Since all cores are busy, there is no boost available. Let's be generous and say
> > the tasks initially get P0, i.e. freq_curr = 2.25 GHz. Combining the formulas
> > above and taking util_raw = 825/1024 = 0.8, freq_next is:
> >
> > freq_next = 1.25 * 2.25 * 0.8 * 2.25 / 2.825 = 1.79 GHz
>
> Right, so here's a 'problem' between schedutil and cpufreq, they don't
> use the same f_max at all times.
>
> And this is also an inconsistency between acpi_cpufreq and intel_pstate
> (passive). IIRC the intel_pstate cpufreq drivers uses 4C/1C/P0 resp,
> while ACPI seems to stick to P0 f_max.
That's correct. A different f_max is used depending on the occasion. Let me
rephrase with:
cpufreq core asks the driver what's the f_max. What's the answer?
intel_pstate says: 1C
acpi_cpufreq says: P0
scheduler asks the freq-invariance machinery what's f_max, because it needs to
compute f_curr/f_max. What's the answer?
Intel CPUs: 4C in most cases, 1C on Atom, something else on Xeon Phi.
AMD CPUs: (P0 + 1C) / 2.
Legend:
1C = 1-core boost
4C = 4-cores boost
P0 = max non-boost P-States
>
> Rafael; should ACPI change that behaviour rather than adding yet another
> magic variable?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-26 9:28 ` Giovanni Gherdovich
@ 2021-01-26 10:02 ` Peter Zijlstra
2021-02-02 18:45 ` Rafael J. Wysocki
1 sibling, 0 replies; 26+ messages in thread
From: Peter Zijlstra @ 2021-01-26 10:02 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Borislav Petkov, Ingo Molnar, Rafael J . Wysocki, Viresh Kumar,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
On Tue, Jan 26, 2021 at 10:28:30AM +0100, Giovanni Gherdovich wrote:
> On Mon, 2021-01-25 at 11:04 +0100, Peter Zijlstra wrote:
> > On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > > This workload is constant in time, so instead of using the PELT sum we can
> > > pretend that scale invariance is obtained with
> > >
> > > util_inv = util_raw * freq_curr / freq_max1 [formula-1]
> > >
> > > where util_raw is the PELT util from v5.10 (which is to say, not invariant),
> > > and util_inv is the PELT util from v5.11-rc4. freq_max1 comes from
> > > commit 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for
> > > frequency invariance on AMD EPYC") and is (P0+max_boost)/2 = (2.25+3.4)/2 =
> > > 2.825 GHz. Then we have the schedutil formula
> > >
> > > freq_next = 1.25 * freq_max2 * util_inv [formula-2]
> > >
> > > Here v5.11-rc4 uses freq_max2 = P0 = 2.25 GHz (and this patch changes it to
> > > 3.4 GHz).
> > >
> > > Since all cores are busy, there is no boost available. Let's be generous and say
> > > the tasks initially get P0, i.e. freq_curr = 2.25 GHz. Combining the formulas
> > > above and taking util_raw = 825/1024 = 0.8, freq_next is:
> > >
> > > freq_next = 1.25 * 2.25 * 0.8 * 2.25 / 2.825 = 1.79 GHz
> >
> > Right, so here's a 'problem' between schedutil and cpufreq, they don't
> > use the same f_max at all times.
> >
> > And this is also an inconsistency between acpi_cpufreq and intel_pstate
> > (passive). IIRC the intel_pstate cpufreq drivers uses 4C/1C/P0 resp,
> > while ACPI seems to stick to P0 f_max.
>
> That's correct. A different f_max is used depending on the occasion. Let me
> rephrase with:
>
> cpufreq core asks the driver what's the f_max. What's the answer?
>
> intel_pstate says: 1C
Oh indeed it does...
> acpi_cpufreq says: P0
>
> scheduler asks the freq-invariance machinery what's f_max, because it needs to
> compute f_curr/f_max. What's the answer?
>
> Intel CPUs: 4C in most cases, 1C on Atom, something else on Xeon Phi.
> AMD CPUs: (P0 + 1C) / 2.
Right, and thwn freq-invariance uses larger f_max than cpufreq uses for
frequency selection, we under select exactly like you found.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-26 9:28 ` Giovanni Gherdovich
2021-01-26 10:02 ` Peter Zijlstra
@ 2021-02-02 18:45 ` Rafael J. Wysocki
2021-02-02 19:11 ` Rafael J. Wysocki
1 sibling, 1 reply; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-02-02 18:45 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Peter Zijlstra, Borislav Petkov, Ingo Molnar, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Mel Gorman, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
On Tue, Jan 26, 2021 at 5:19 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
>
> On Mon, 2021-01-25 at 11:04 +0100, Peter Zijlstra wrote:
> > On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > > This workload is constant in time, so instead of using the PELT sum we can
> > > pretend that scale invariance is obtained with
> > >
> > > util_inv = util_raw * freq_curr / freq_max1 [formula-1]
> > >
> > > where util_raw is the PELT util from v5.10 (which is to say, not invariant),
> > > and util_inv is the PELT util from v5.11-rc4. freq_max1 comes from
> > > commit 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for
> > > frequency invariance on AMD EPYC") and is (P0+max_boost)/2 = (2.25+3.4)/2 =
> > > 2.825 GHz. Then we have the schedutil formula
> > >
> > > freq_next = 1.25 * freq_max2 * util_inv [formula-2]
> > >
> > > Here v5.11-rc4 uses freq_max2 = P0 = 2.25 GHz (and this patch changes it to
> > > 3.4 GHz).
> > >
> > > Since all cores are busy, there is no boost available. Let's be generous and say
> > > the tasks initially get P0, i.e. freq_curr = 2.25 GHz. Combining the formulas
> > > above and taking util_raw = 825/1024 = 0.8, freq_next is:
> > >
> > > freq_next = 1.25 * 2.25 * 0.8 * 2.25 / 2.825 = 1.79 GHz
> >
> > Right, so here's a 'problem' between schedutil and cpufreq, they don't
> > use the same f_max at all times.
> >
> > And this is also an inconsistency between acpi_cpufreq and intel_pstate
> > (passive). IIRC the intel_pstate cpufreq drivers uses 4C/1C/P0 resp,
> > while ACPI seems to stick to P0 f_max.
>
> That's correct. A different f_max is used depending on the occasion. Let me
> rephrase with:
OK, I confused the terminology, sorry about that.
> cpufreq core asks the driver what's the f_max. What's the answer?
>
> intel_pstate says: 1C
Yes, unless turbo is disabled, in which case it is P0.
> acpi_cpufreq says: P0
This is P0+1, isn't it?
> scheduler asks the freq-invariance machinery what's f_max, because it needs to
> compute f_curr/f_max. What's the answer?
>
> Intel CPUs: 4C in most cases, 1C on Atom, something else on Xeon Phi.
> AMD CPUs: (P0 + 1C) / 2.
>
>
> Legend:
> 1C = 1-core boost
> 4C = 4-cores boost
> P0 = max non-boost P-States
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-02-02 18:45 ` Rafael J. Wysocki
@ 2021-02-02 19:11 ` Rafael J. Wysocki
2021-02-03 9:56 ` Giovanni Gherdovich
0 siblings, 1 reply; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-02-02 19:11 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Peter Zijlstra, Borislav Petkov, Ingo Molnar, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Mel Gorman, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
On Tue, Feb 2, 2021 at 7:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jan 26, 2021 at 5:19 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> >
> > On Mon, 2021-01-25 at 11:04 +0100, Peter Zijlstra wrote:
> > > On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > > > This workload is constant in time, so instead of using the PELT sum we can
> > > > pretend that scale invariance is obtained with
> > > >
> > > > util_inv = util_raw * freq_curr / freq_max1 [formula-1]
> > > >
> > > > where util_raw is the PELT util from v5.10 (which is to say, not invariant),
> > > > and util_inv is the PELT util from v5.11-rc4. freq_max1 comes from
> > > > commit 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for
> > > > frequency invariance on AMD EPYC") and is (P0+max_boost)/2 = (2.25+3.4)/2 =
> > > > 2.825 GHz. Then we have the schedutil formula
> > > >
> > > > freq_next = 1.25 * freq_max2 * util_inv [formula-2]
> > > >
> > > > Here v5.11-rc4 uses freq_max2 = P0 = 2.25 GHz (and this patch changes it to
> > > > 3.4 GHz).
> > > >
> > > > Since all cores are busy, there is no boost available. Let's be generous and say
> > > > the tasks initially get P0, i.e. freq_curr = 2.25 GHz. Combining the formulas
> > > > above and taking util_raw = 825/1024 = 0.8, freq_next is:
> > > >
> > > > freq_next = 1.25 * 2.25 * 0.8 * 2.25 / 2.825 = 1.79 GHz
> > >
> > > Right, so here's a 'problem' between schedutil and cpufreq, they don't
> > > use the same f_max at all times.
> > >
> > > And this is also an inconsistency between acpi_cpufreq and intel_pstate
> > > (passive). IIRC the intel_pstate cpufreq drivers uses 4C/1C/P0 resp,
> > > while ACPI seems to stick to P0 f_max.
> >
> > That's correct. A different f_max is used depending on the occasion. Let me
> > rephrase with:
>
> OK, I confused the terminology, sorry about that.
>
> > cpufreq core asks the driver what's the f_max. What's the answer?
> >
> > intel_pstate says: 1C
>
> Yes, unless turbo is disabled, in which case it is P0.
BTW, and that actually is quite important, the max_freq reported by
intel_pstate doesn't matter for schedutil after the new ->adjust_perf
callback has been added, because that doesn't even use the frequency.
So, as a long-term remedy, it may just be better to implement
->adjust_perf in acpi_cpufreq().
Again, I'm terribly sorry for missing this thread and the patch.
> > acpi_cpufreq says: P0
>
> This is P0+1, isn't it?
>
> > scheduler asks the freq-invariance machinery what's f_max, because it needs to
> > compute f_curr/f_max. What's the answer?
> >
> > Intel CPUs: 4C in most cases, 1C on Atom, something else on Xeon Phi.
> > AMD CPUs: (P0 + 1C) / 2.
> >
> >
> > Legend:
> > 1C = 1-core boost
> > 4C = 4-cores boost
> > P0 = max non-boost P-States
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-02-02 19:11 ` Rafael J. Wysocki
@ 2021-02-03 9:56 ` Giovanni Gherdovich
0 siblings, 0 replies; 26+ messages in thread
From: Giovanni Gherdovich @ 2021-02-03 9:56 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Peter Zijlstra, Borislav Petkov, Ingo Molnar, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Mel Gorman, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
On Tue, 2021-02-02 at 20:11 +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2021 at 7:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Jan 26, 2021 at 5:19 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> > >
> > >
> > > cpufreq core asks the driver what's the f_max. What's the answer?
> > >
> > > intel_pstate says: 1C
> >
> > Yes, unless turbo is disabled, in which case it is P0.
>
> BTW, and that actually is quite important, the max_freq reported by
> intel_pstate doesn't matter for schedutil after the new ->adjust_perf
> callback has been added, because that doesn't even use the frequency.
>
> So, as a long-term remedy, it may just be better to implement
> ->adjust_perf in acpi_cpufreq().
Thanks for pointing this out.
I agree that in the long term adding ->adjust_perf to acpi_cpufreq is
the best solution.
Yet for this submission, considering it's late in the 5.11 cycle,
the patch I propose is a reasonable candidate: the footprint is small and
it's gone through a fair amount of testing.
Thanks,
Giovanni
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-25 10:04 ` Peter Zijlstra
2021-01-26 9:28 ` Giovanni Gherdovich
@ 2021-02-02 18:40 ` Rafael J. Wysocki
1 sibling, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-02-02 18:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Giovanni Gherdovich, Borislav Petkov, Ingo Molnar,
Rafael J . Wysocki, Viresh Kumar, Jon Grimm, Nathan Fontenot,
Yazen Ghannam, Thomas Lendacky, Suthikulpanit Suravee,
Mel Gorman, Pu Wen, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Michael Larabel, the arch/x86 maintainers,
Linux PM, Linux Kernel Mailing List, ACPI Devel Maling List
On Mon, Jan 25, 2021 at 11:11 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > This workload is constant in time, so instead of using the PELT sum we can
> > pretend that scale invariance is obtained with
> >
> > util_inv = util_raw * freq_curr / freq_max1 [formula-1]
> >
> > where util_raw is the PELT util from v5.10 (which is to say, not invariant),
> > and util_inv is the PELT util from v5.11-rc4. freq_max1 comes from
> > commit 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for
> > frequency invariance on AMD EPYC") and is (P0+max_boost)/2 = (2.25+3.4)/2 =
> > 2.825 GHz. Then we have the schedutil formula
> >
> > freq_next = 1.25 * freq_max2 * util_inv [formula-2]
> >
> > Here v5.11-rc4 uses freq_max2 = P0 = 2.25 GHz (and this patch changes it to
> > 3.4 GHz).
> >
> > Since all cores are busy, there is no boost available. Let's be generous and say
> > the tasks initially get P0, i.e. freq_curr = 2.25 GHz. Combining the formulas
> > above and taking util_raw = 825/1024 = 0.8, freq_next is:
> >
> > freq_next = 1.25 * 2.25 * 0.8 * 2.25 / 2.825 = 1.79 GHz
>
> Right, so here's a 'problem' between schedutil and cpufreq, they don't
> use the same f_max at all times.
>
> And this is also an inconsistency between acpi_cpufreq and intel_pstate
> (passive). IIRC the intel_pstate cpufreq drivers uses 4C/1C/P0 resp,
> while ACPI seems to stick to P0 f_max.
The only place where 4C is used is the scale invariance code AFAICS.
intel_pstate uses P0 as the f_max unless turbo is disabled.
The difference between intel_pstate and acpi_cpufreq is that (a) the
latter uses a frequency table and the former doesn't and (b) the
latter uses the P0 entry of the frequency table to represent the
entire turbo range,
> Rafael; should ACPI change that behaviour rather than adding yet another
> magic variable?
I'm not sure. That may change the behavior from what is expected by some users.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-22 20:40 ` [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula Giovanni Gherdovich
2021-01-25 10:04 ` Peter Zijlstra
@ 2021-01-25 10:06 ` Peter Zijlstra
2021-01-26 9:09 ` Giovanni Gherdovich
2021-02-02 18:59 ` Rafael J. Wysocki
2021-02-03 6:04 ` Viresh Kumar
3 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-01-25 10:06 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Borislav Petkov, Ingo Molnar, Rafael J . Wysocki, Viresh Kumar,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> 1. PROBLEM DESCRIPTION (over-utilization and schedutil)
>
> The problem happens on CPU-bound workloads spanning a large number of cores.
> In this case schedutil won't select the maximum P-State. Actually, it's
> likely that it will select the minimum one.
>
> A CPU-bound workload puts the machine in a state generally called
> "over-utilization": an increase in CPU speed doesn't result in an increase of
> capacity. The fraction of time tasks spend on CPU becomes constant regardless
> of clock frequency (the tasks eat whatever we throw at them), and the PELT
> invariant util goes up and down with the frequency (i.e. it's not invariant
> anymore).
> v5.10 v5.11-rc4
> ~~~~~~~~~~~~~~~~~~~~~~~~
> CPU activity (mpstat) 80-90% 80-90%
> schedutil requests (tracepoint) always P0 mostly P2
> CPU frequency (HW feedback) ~2.2 GHz ~1.5 GHz
> PELT root rq util (tracepoint) ~825 ~450
>
> mpstat shows that the workload is CPU-bound and usage doesn't change with
So I'm having trouble with calling a 80%-90% workload CPU bound, because
clearly there's a ton of idle time.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-25 10:06 ` Peter Zijlstra
@ 2021-01-26 9:09 ` Giovanni Gherdovich
2021-01-26 9:31 ` Mel Gorman
0 siblings, 1 reply; 26+ messages in thread
From: Giovanni Gherdovich @ 2021-01-26 9:09 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Borislav Petkov, Ingo Molnar, Rafael J . Wysocki, Viresh Kumar,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
On Mon, 2021-01-25 at 11:06 +0100, Peter Zijlstra wrote:
> On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > 1. PROBLEM DESCRIPTION (over-utilization and schedutil)
> >
> > The problem happens on CPU-bound workloads spanning a large number of cores.
> > In this case schedutil won't select the maximum P-State. Actually, it's
> > likely that it will select the minimum one.
> >
> > A CPU-bound workload puts the machine in a state generally called
> > "over-utilization": an increase in CPU speed doesn't result in an increase of
> > capacity. The fraction of time tasks spend on CPU becomes constant regardless
> > of clock frequency (the tasks eat whatever we throw at them), and the PELT
> > invariant util goes up and down with the frequency (i.e. it's not invariant
> > anymore).
> > v5.10 v5.11-rc4
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> > CPU activity (mpstat) 80-90% 80-90%
> > schedutil requests (tracepoint) always P0 mostly P2
> > CPU frequency (HW feedback) ~2.2 GHz ~1.5 GHz
> > PELT root rq util (tracepoint) ~825 ~450
> >
> > mpstat shows that the workload is CPU-bound and usage doesn't change with
>
> So I'm having trouble with calling a 80%-90% workload CPU bound, because
> clearly there's a ton of idle time.
Yes you're right. There is considerable idle time and calling it CPU-bound is
a bit of a stretch.
Yet I don't think I'm completely off the mark. The busy time is the same with
the machine running at 1.5 GHz and at 2.2 GHz (it just takes longer to
finish). To me it seems like the CPU is the bottleneck, with some overhead on
top.
I will confirm what causes the idle time.
Giovanni
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-26 9:09 ` Giovanni Gherdovich
@ 2021-01-26 9:31 ` Mel Gorman
2021-01-26 10:05 ` Peter Zijlstra
0 siblings, 1 reply; 26+ messages in thread
From: Mel Gorman @ 2021-01-26 9:31 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Peter Zijlstra, Borislav Petkov, Ingo Molnar, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
On Tue, Jan 26, 2021 at 10:09:27AM +0100, Giovanni Gherdovich wrote:
> On Mon, 2021-01-25 at 11:06 +0100, Peter Zijlstra wrote:
> > On Fri, Jan 22, 2021 at 09:40:38PM +0100, Giovanni Gherdovich wrote:
> > > 1. PROBLEM DESCRIPTION (over-utilization and schedutil)
> > >
> > > The problem happens on CPU-bound workloads spanning a large number of cores.
> > > In this case schedutil won't select the maximum P-State. Actually, it's
> > > likely that it will select the minimum one.
> > >
> > > A CPU-bound workload puts the machine in a state generally called
> > > "over-utilization": an increase in CPU speed doesn't result in an increase of
> > > capacity. The fraction of time tasks spend on CPU becomes constant regardless
> > > of clock frequency (the tasks eat whatever we throw at them), and the PELT
> > > invariant util goes up and down with the frequency (i.e. it's not invariant
> > > anymore).
> > > v5.10 v5.11-rc4
> > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > > CPU activity (mpstat) 80-90% 80-90%
> > > schedutil requests (tracepoint) always P0 mostly P2
> > > CPU frequency (HW feedback) ~2.2 GHz ~1.5 GHz
> > > PELT root rq util (tracepoint) ~825 ~450
> > >
> > > mpstat shows that the workload is CPU-bound and usage doesn't change with
> >
> > So I'm having trouble with calling a 80%-90% workload CPU bound, because
> > clearly there's a ton of idle time.
>
> Yes you're right. There is considerable idle time and calling it CPU-bound is
> a bit of a stretch.
>
> Yet I don't think I'm completely off the mark. The busy time is the same with
> the machine running at 1.5 GHz and at 2.2 GHz (it just takes longer to
> finish). To me it seems like the CPU is the bottleneck, with some overhead on
> top.
>
I think this is an important observation because while the load may not
be fully CPU-bound, it's still at the point where race-to-idle by running
at a higher frequency is relevant. During the busy time, the results
(and Michael's testing) indicate that the higher frequency may still be
justified. I agree that there is a "a 'problem' between schedutil and
cpufreq, they don't use the same f_max at all times", fixing that mid
-rc may not be appropriate because it's a big change in an rc window.
So, should this patch be merged for 5.11 as a stopgap, fix up
schedutil/cpufreq and then test both AMD and Intel chips reporting the
correct max non-turbo and max-turbo frequencies? That would give time to
give some testing in linux-next before merging to reduce the chance
something else falls out.
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-26 9:31 ` Mel Gorman
@ 2021-01-26 10:05 ` Peter Zijlstra
[not found] ` <1611933781.15858.48.camel@suse.cz>
0 siblings, 1 reply; 26+ messages in thread
From: Peter Zijlstra @ 2021-01-26 10:05 UTC (permalink / raw)
To: Mel Gorman
Cc: Giovanni Gherdovich, Borislav Petkov, Ingo Molnar,
Rafael J . Wysocki, Viresh Kumar, Jon Grimm, Nathan Fontenot,
Yazen Ghannam, Thomas Lendacky, Suthikulpanit Suravee, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
x86, linux-pm, linux-kernel, linux-acpi
On Tue, Jan 26, 2021 at 09:31:40AM +0000, Mel Gorman wrote:
> So, should this patch be merged for 5.11 as a stopgap, fix up
> schedutil/cpufreq and then test both AMD and Intel chips reporting the
> correct max non-turbo and max-turbo frequencies? That would give time to
> give some testing in linux-next before merging to reduce the chance
> something else falls out.
Yeah, we should probably do this now. Rafael, you want this or should I
take it?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-22 20:40 ` [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula Giovanni Gherdovich
2021-01-25 10:04 ` Peter Zijlstra
2021-01-25 10:06 ` Peter Zijlstra
@ 2021-02-02 18:59 ` Rafael J. Wysocki
2021-02-02 19:26 ` Rafael J. Wysocki
2021-02-03 9:12 ` Giovanni Gherdovich
2021-02-03 6:04 ` Viresh Kumar
3 siblings, 2 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-02-02 18:59 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Borislav Petkov, Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Mel Gorman, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
On Fri, Jan 22, 2021 at 9:47 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
>
[cut]
>
> Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems")
> Fixes: 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for frequency invariance on AMD EPYC")
> Reported-by: Michael Larabel <Michael@phoronix.com>
> Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> ---
> drivers/cpufreq/acpi-cpufreq.c | 64 +++++++++++++++++++++++++++++++-
> drivers/cpufreq/cpufreq.c | 3 ++
> include/linux/cpufreq.h | 5 +++
> kernel/sched/cpufreq_schedutil.c | 8 +++-
> 4 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 1e4fbb002a31..2378bc1bf2c4 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -27,6 +27,10 @@
>
> #include <acpi/processor.h>
>
> +#ifdef CONFIG_ACPI_CPPC_LIB
Why is the #ifdef needed here?
> +#include <acpi/cppc_acpi.h>
> +#endif
> +
> #include <asm/msr.h>
> #include <asm/processor.h>
> #include <asm/cpufeature.h>
> @@ -628,11 +632,57 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
> }
> #endif
>
> +#ifdef CONFIG_ACPI_CPPC_LIB
> +static bool amd_max_boost(unsigned int max_freq,
> + unsigned int *max_boost)
> +{
> + struct cppc_perf_caps perf_caps;
> + u64 highest_perf, nominal_perf, perf_ratio;
> + int ret;
> +
> + ret = cppc_get_perf_caps(0, &perf_caps);
> + if (ret) {
> + pr_debug("Could not retrieve perf counters (%d)\n", ret);
> + return false;
> + }
> +
> + highest_perf = perf_caps.highest_perf;
> + nominal_perf = perf_caps.nominal_perf;
> +
> + if (!highest_perf || !nominal_perf) {
> + pr_debug("Could not retrieve highest or nominal performance\n");
> + return false;
> + }
> +
> + perf_ratio = div_u64(highest_perf * SCHED_CAPACITY_SCALE, nominal_perf);
> + if (perf_ratio <= SCHED_CAPACITY_SCALE) {
> + pr_debug("Either perf_ratio is 0, or nominal >= highest performance\n");
> + return false;
> + }
> +
> + *max_boost = max_freq * perf_ratio >> SCHED_CAPACITY_SHIFT;
> + if (!*max_boost) {
> + pr_debug("max_boost seems to be zero\n");
> + return false;
> + }
> +
> + return true;
> +}
> +#else
> +static bool amd_max_boost(unsigned int max_freq,
> + unsigned int *max_boost)
> +{
> + return false;
> +}
> +#endif
> +
> static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> {
> unsigned int i;
> unsigned int valid_states = 0;
> unsigned int cpu = policy->cpu;
> + unsigned int freq, max_freq = 0;
> + unsigned int max_boost;
> struct acpi_cpufreq_data *data;
> unsigned int result = 0;
> struct cpuinfo_x86 *c = &cpu_data(policy->cpu);
> @@ -779,15 +829,25 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> freq_table[valid_states-1].frequency / 1000)
> continue;
>
> + freq = perf->states[i].core_frequency * 1000;
> freq_table[valid_states].driver_data = i;
> - freq_table[valid_states].frequency =
> - perf->states[i].core_frequency * 1000;
> + freq_table[valid_states].frequency = freq;
> +
> + if (freq > max_freq)
> + max_freq = freq;
> +
> valid_states++;
> }
> freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
> policy->freq_table = freq_table;
> perf->state = 0;
>
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> + amd_max_boost(max_freq, &max_boost)) {
> + policy->cpuinfo.max_boost = max_boost;
Why not to set max_freq to max_boost instead?
This value is set once at the init time anyway and schedutil would use
max_boost instead of max_freq anyway.
Also notice that the static branch is global and the max_boost value
for different CPUs may be different, at least in theory.
> + static_branch_enable(&cpufreq_amd_max_boost);
> + }
> +
> switch (perf->control_register.space_id) {
> case ACPI_ADR_SPACE_SYSTEM_IO:
> /*
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d0a3525ce27f..b96677f6b57e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2721,6 +2721,9 @@ int cpufreq_boost_enabled(void)
> }
> EXPORT_SYMBOL_GPL(cpufreq_boost_enabled);
>
> +DEFINE_STATIC_KEY_FALSE(cpufreq_amd_max_boost);
> +EXPORT_SYMBOL_GPL(cpufreq_amd_max_boost);
> +
> /*********************************************************************
> * REGISTER / UNREGISTER CPUFREQ DRIVER *
> *********************************************************************/
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9c8b7437b6cd..341cac76d254 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -40,9 +40,14 @@ enum cpufreq_table_sorting {
> CPUFREQ_TABLE_SORTED_DESCENDING
> };
>
> +DECLARE_STATIC_KEY_FALSE(cpufreq_amd_max_boost);
> +
> +#define cpufreq_driver_has_max_boost() static_branch_unlikely(&cpufreq_amd_max_boost)
> +
> struct cpufreq_cpuinfo {
> unsigned int max_freq;
> unsigned int min_freq;
> + unsigned int max_boost;
>
> /* in 10^(-9) s = nanoseconds */
> unsigned int transition_latency;
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 6931f0cdeb80..541f3db3f576 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -159,8 +159,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> unsigned long util, unsigned long max)
> {
> struct cpufreq_policy *policy = sg_policy->policy;
> - unsigned int freq = arch_scale_freq_invariant() ?
> - policy->cpuinfo.max_freq : policy->cur;
> + unsigned int freq, max_freq;
> +
> + max_freq = cpufreq_driver_has_max_boost() ?
> + policy->cpuinfo.max_boost : policy->cpuinfo.max_freq;
> +
> + freq = arch_scale_freq_invariant() ? max_freq : policy->cur;
>
> freq = map_util_freq(util, freq, max);
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-02-02 18:59 ` Rafael J. Wysocki
@ 2021-02-02 19:26 ` Rafael J. Wysocki
2021-02-03 8:39 ` Giovanni Gherdovich
2021-02-03 9:12 ` Giovanni Gherdovich
1 sibling, 1 reply; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-02-02 19:26 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Borislav Petkov, Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Mel Gorman, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
On Tue, Feb 2, 2021 at 7:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Jan 22, 2021 at 9:47 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> >
>
> [cut]
>
> >
> > Fixes: 41ea667227ba ("x86, sched: Calculate frequency invariance for AMD systems")
> > Fixes: 976df7e5730e ("x86, sched: Use midpoint of max_boost and max_P for frequency invariance on AMD EPYC")
> > Reported-by: Michael Larabel <Michael@phoronix.com>
> > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> > ---
> > drivers/cpufreq/acpi-cpufreq.c | 64 +++++++++++++++++++++++++++++++-
> > drivers/cpufreq/cpufreq.c | 3 ++
> > include/linux/cpufreq.h | 5 +++
> > kernel/sched/cpufreq_schedutil.c | 8 +++-
> > 4 files changed, 76 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > index 1e4fbb002a31..2378bc1bf2c4 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -27,6 +27,10 @@
> >
> > #include <acpi/processor.h>
> >
> > +#ifdef CONFIG_ACPI_CPPC_LIB
>
> Why is the #ifdef needed here?
>
> > +#include <acpi/cppc_acpi.h>
> > +#endif
> > +
> > #include <asm/msr.h>
> > #include <asm/processor.h>
> > #include <asm/cpufeature.h>
> > @@ -628,11 +632,57 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
> > }
> > #endif
> >
> > +#ifdef CONFIG_ACPI_CPPC_LIB
> > +static bool amd_max_boost(unsigned int max_freq,
> > + unsigned int *max_boost)
> > +{
> > + struct cppc_perf_caps perf_caps;
> > + u64 highest_perf, nominal_perf, perf_ratio;
> > + int ret;
> > +
> > + ret = cppc_get_perf_caps(0, &perf_caps);
> > + if (ret) {
> > + pr_debug("Could not retrieve perf counters (%d)\n", ret);
> > + return false;
> > + }
> > +
> > + highest_perf = perf_caps.highest_perf;
> > + nominal_perf = perf_caps.nominal_perf;
> > +
> > + if (!highest_perf || !nominal_perf) {
> > + pr_debug("Could not retrieve highest or nominal performance\n");
> > + return false;
> > + }
> > +
> > + perf_ratio = div_u64(highest_perf * SCHED_CAPACITY_SCALE, nominal_perf);
> > + if (perf_ratio <= SCHED_CAPACITY_SCALE) {
> > + pr_debug("Either perf_ratio is 0, or nominal >= highest performance\n");
> > + return false;
> > + }
> > +
> > + *max_boost = max_freq * perf_ratio >> SCHED_CAPACITY_SHIFT;
> > + if (!*max_boost) {
> > + pr_debug("max_boost seems to be zero\n");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +#else
> > +static bool amd_max_boost(unsigned int max_freq,
> > + unsigned int *max_boost)
> > +{
> > + return false;
> > +}
> > +#endif
> > +
> > static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > {
> > unsigned int i;
> > unsigned int valid_states = 0;
> > unsigned int cpu = policy->cpu;
> > + unsigned int freq, max_freq = 0;
> > + unsigned int max_boost;
> > struct acpi_cpufreq_data *data;
> > unsigned int result = 0;
> > struct cpuinfo_x86 *c = &cpu_data(policy->cpu);
> > @@ -779,15 +829,25 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > freq_table[valid_states-1].frequency / 1000)
> > continue;
> >
> > + freq = perf->states[i].core_frequency * 1000;
> > freq_table[valid_states].driver_data = i;
> > - freq_table[valid_states].frequency =
> > - perf->states[i].core_frequency * 1000;
> > + freq_table[valid_states].frequency = freq;
> > +
> > + if (freq > max_freq)
> > + max_freq = freq;
> > +
> > valid_states++;
> > }
> > freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
> > policy->freq_table = freq_table;
> > perf->state = 0;
> >
> > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> > + amd_max_boost(max_freq, &max_boost)) {
> > + policy->cpuinfo.max_boost = max_boost;
>
> Why not to set max_freq to max_boost instead?
I mean, would setting the frequency in the last table entry to max_boost work?
Alternatively, one more (artificial) entry with the frequency equal to
max_boost could be added.
> This value is set once at the init time anyway and schedutil would use
> max_boost instead of max_freq anyway.
>
> Also notice that the static branch is global and the max_boost value
> for different CPUs may be different, at least in theory.
>
> > + static_branch_enable(&cpufreq_amd_max_boost);
> > + }
> > +
> > switch (perf->control_register.space_id) {
> > case ACPI_ADR_SPACE_SYSTEM_IO:
> > /*
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index d0a3525ce27f..b96677f6b57e 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2721,6 +2721,9 @@ int cpufreq_boost_enabled(void)
> > }
> > EXPORT_SYMBOL_GPL(cpufreq_boost_enabled);
> >
> > +DEFINE_STATIC_KEY_FALSE(cpufreq_amd_max_boost);
> > +EXPORT_SYMBOL_GPL(cpufreq_amd_max_boost);
> > +
> > /*********************************************************************
> > * REGISTER / UNREGISTER CPUFREQ DRIVER *
> > *********************************************************************/
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> > index 9c8b7437b6cd..341cac76d254 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -40,9 +40,14 @@ enum cpufreq_table_sorting {
> > CPUFREQ_TABLE_SORTED_DESCENDING
> > };
> >
> > +DECLARE_STATIC_KEY_FALSE(cpufreq_amd_max_boost);
> > +
> > +#define cpufreq_driver_has_max_boost() static_branch_unlikely(&cpufreq_amd_max_boost)
> > +
> > struct cpufreq_cpuinfo {
> > unsigned int max_freq;
> > unsigned int min_freq;
> > + unsigned int max_boost;
> >
> > /* in 10^(-9) s = nanoseconds */
> > unsigned int transition_latency;
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 6931f0cdeb80..541f3db3f576 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -159,8 +159,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> > unsigned long util, unsigned long max)
> > {
> > struct cpufreq_policy *policy = sg_policy->policy;
> > - unsigned int freq = arch_scale_freq_invariant() ?
> > - policy->cpuinfo.max_freq : policy->cur;
> > + unsigned int freq, max_freq;
> > +
> > + max_freq = cpufreq_driver_has_max_boost() ?
> > + policy->cpuinfo.max_boost : policy->cpuinfo.max_freq;
> > +
> > + freq = arch_scale_freq_invariant() ? max_freq : policy->cur;
> >
> > freq = map_util_freq(util, freq, max);
> >
> > --
> > 2.26.2
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-02-02 19:26 ` Rafael J. Wysocki
@ 2021-02-03 8:39 ` Giovanni Gherdovich
2021-02-03 13:40 ` Rafael J. Wysocki
0 siblings, 1 reply; 26+ messages in thread
From: Giovanni Gherdovich @ 2021-02-03 8:39 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar
Cc: Borislav Petkov, Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
Hello,
both Rafael and Viresh make a similar remark: why adding a new "max_boost"
variable, since "max_freq" is already available and could be used instead.
Replying here to both.
On Tue, 2021-02-02 at 20:26 +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 2, 2021 at 7:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, Jan 22, 2021 at 9:47 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> >
> > [cut]
> > > @@ -779,15 +829,25 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > > freq_table[valid_states-1].frequency / 1000)
> > > continue;
> > >
> > > + freq = perf->states[i].core_frequency * 1000;
> > > freq_table[valid_states].driver_data = i;
> > > - freq_table[valid_states].frequency =
> > > - perf->states[i].core_frequency * 1000;
> > > + freq_table[valid_states].frequency = freq;
> > > +
> > > + if (freq > max_freq)
> > > + max_freq = freq;
> > > +
> > > valid_states++;
> > > }
> > > freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
> > > policy->freq_table = freq_table;
> > > perf->state = 0;
> > >
> > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> > > + amd_max_boost(max_freq, &max_boost)) {
> > > + policy->cpuinfo.max_boost = max_boost;
> >
> > Why not to set max_freq to max_boost instead?
>
> I mean, would setting the frequency in the last table entry to max_boost work?
>
> Alternatively, one more (artificial) entry with the frequency equal to
> max_boost could be added.
On Wed, 2021-02-03 at 11:34 +0530, Viresh Kumar wrote:
> [cut]
>
> On 22-01-21, 21:40, Giovanni Gherdovich wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 6931f0cdeb80..541f3db3f576 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -159,8 +159,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> > unsigned long util, unsigned long max)
> > {
> > struct cpufreq_policy *policy = sg_policy->policy;
> > - unsigned int freq = arch_scale_freq_invariant() ?
> > - policy->cpuinfo.max_freq : policy->cur;
> > + unsigned int freq, max_freq;
> > +
> > + max_freq = cpufreq_driver_has_max_boost() ?
> > + policy->cpuinfo.max_boost : policy->cpuinfo.max_freq;
>
> Also, can't we update max_freq itself from the cpufreq driver? What
> troubles will it cost ?
I could add the max boost frequency to the frequency table (and
policy->cpuinfo.max_freq would follow), yes, but that would trigger the
following warning from acpi-cpufreq.c:
static void acpi_cpufreq_cpu_ready(struct cpufreq_policy *policy)
{
struct acpi_processor_performance *perf = per_cpu_ptr(acpi_perf_data,
policy->cpu);
if (perf->states[0].core_frequency * 1000 != policy->cpuinfo.max_freq)
pr_warn(FW_WARN "P-state 0 is not max freq\n");
}
so I thought that to stay out of troubles I'd supply a different variable,
max_boost, to be used only in the schedutil formula. After schedutil
figures out a desired next_freq then the usual comparison with the
firmware-supplied frequency table could take place.
Altering the frequency table seemed more invasive because once a freq value is
in there, it's going to be actually requested by the driver to the platform. I
only want my max_boost to stretch the range of schedutil's next_freq.
On Tue, 2021-02-02 at 19:59 +0100, Rafael J. Wysocki wrote:
>
> [cut]
> Also notice that the static branch is global and the max_boost value
> for different CPUs may be different, at least in theory.
In theory yes, but I'm guarding the code with two conditions:
* vendor is X86_VENDOR_AMD
* cppc_get_perf_caps() returns success
this identifies AMD EPYC cpus with model 7xx2 and later, where max_boost is
the same on all cores. I may have added synchronization so that only one cpu
sets the value, but I didn't in the interest of simplicity for an -rc patch
(I'd have to consider hotplug, the maxcpus= command line param, ecc).
Giovanni
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-02-03 8:39 ` Giovanni Gherdovich
@ 2021-02-03 13:40 ` Rafael J. Wysocki
0 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-02-03 13:40 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Rafael J. Wysocki, Viresh Kumar, Borislav Petkov, Ingo Molnar,
Peter Zijlstra, Rafael J . Wysocki, Jon Grimm, Nathan Fontenot,
Yazen Ghannam, Thomas Lendacky, Suthikulpanit Suravee,
Mel Gorman, Pu Wen, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Michael Larabel, the arch/x86 maintainers,
Linux PM, Linux Kernel Mailing List, ACPI Devel Maling List
On Wed, Feb 3, 2021 at 9:39 AM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
>
> Hello,
>
> both Rafael and Viresh make a similar remark: why adding a new "max_boost"
> variable, since "max_freq" is already available and could be used instead.
>
> Replying here to both.
>
> On Tue, 2021-02-02 at 20:26 +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 2, 2021 at 7:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Fri, Jan 22, 2021 at 9:47 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> > >
> > > [cut]
> > > > @@ -779,15 +829,25 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > > > freq_table[valid_states-1].frequency / 1000)
> > > > continue;
> > > >
> > > > + freq = perf->states[i].core_frequency * 1000;
> > > > freq_table[valid_states].driver_data = i;
> > > > - freq_table[valid_states].frequency =
> > > > - perf->states[i].core_frequency * 1000;
> > > > + freq_table[valid_states].frequency = freq;
> > > > +
> > > > + if (freq > max_freq)
> > > > + max_freq = freq;
> > > > +
> > > > valid_states++;
> > > > }
> > > > freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
> > > > policy->freq_table = freq_table;
> > > > perf->state = 0;
> > > >
> > > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> > > > + amd_max_boost(max_freq, &max_boost)) {
> > > > + policy->cpuinfo.max_boost = max_boost;
> > >
> > > Why not to set max_freq to max_boost instead?
> >
> > I mean, would setting the frequency in the last table entry to max_boost work?
> >
> > Alternatively, one more (artificial) entry with the frequency equal to
> > max_boost could be added.
>
> On Wed, 2021-02-03 at 11:34 +0530, Viresh Kumar wrote:
> > [cut]
> >
> > On 22-01-21, 21:40, Giovanni Gherdovich wrote:
> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > > index 6931f0cdeb80..541f3db3f576 100644
> > > --- a/kernel/sched/cpufreq_schedutil.c
> > > +++ b/kernel/sched/cpufreq_schedutil.c
> > > @@ -159,8 +159,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> > > unsigned long util, unsigned long max)
> > > {
> > > struct cpufreq_policy *policy = sg_policy->policy;
> > > - unsigned int freq = arch_scale_freq_invariant() ?
> > > - policy->cpuinfo.max_freq : policy->cur;
> > > + unsigned int freq, max_freq;
> > > +
> > > + max_freq = cpufreq_driver_has_max_boost() ?
> > > + policy->cpuinfo.max_boost : policy->cpuinfo.max_freq;
> >
> > Also, can't we update max_freq itself from the cpufreq driver? What
> > troubles will it cost ?
>
> I could add the max boost frequency to the frequency table (and
> policy->cpuinfo.max_freq would follow), yes, but that would trigger the
> following warning from acpi-cpufreq.c:
>
> static void acpi_cpufreq_cpu_ready(struct cpufreq_policy *policy)
> {
> struct acpi_processor_performance *perf = per_cpu_ptr(acpi_perf_data,
> policy->cpu);
>
> if (perf->states[0].core_frequency * 1000 != policy->cpuinfo.max_freq)
> pr_warn(FW_WARN "P-state 0 is not max freq\n");
> }
This check can be changed, though.
> so I thought that to stay out of troubles I'd supply a different variable,
> max_boost, to be used only in the schedutil formula.
Which is not necessary and the extra static branch is not necessary.
Moreover, there is no reason whatsoever to believe that EPYC is the
only affected processor model. If I'm not mistaken, the regression
will be visible on every CPU where the scale invariance algorithm uses
the max frequency greater than the max frequency used acpi_cpufreq.
Also, AFAICS, it should be sufficient to modify acpi_cpufreq to remedy
this for all of the affected CPUs, not just EPYC.
> After schedutil figures out a desired next_freq then the usual comparison with the
> firmware-supplied frequency table could take place.
>
> Altering the frequency table seemed more invasive because once a freq value is
> in there, it's going to be actually requested by the driver to the platform.
This need not be the case if the control structure for the new entry
is copied from an existing one.
> I only want my max_boost to stretch the range of schedutil's next_freq.
Right, but that can be done in a different way which would be cleaner too IMO.
I'm going to send an alternative patch to fix this problem.
> On Tue, 2021-02-02 at 19:59 +0100, Rafael J. Wysocki wrote:
> >
> > [cut]
> > Also notice that the static branch is global and the max_boost value
> > for different CPUs may be different, at least in theory.
>
> In theory yes, but I'm guarding the code with two conditions:
>
> * vendor is X86_VENDOR_AMD
> * cppc_get_perf_caps() returns success
>
> this identifies AMD EPYC cpus with model 7xx2 and later, where max_boost is
> the same on all cores. I may have added synchronization so that only one cpu
> sets the value, but I didn't in the interest of simplicity for an -rc patch
> (I'd have to consider hotplug, the maxcpus= command line param, ecc).
And what about the other potentially affected processors?
I wouldn't worry about the -rc time frame too much. If we can do a
better fix now, let's do it.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-02-02 18:59 ` Rafael J. Wysocki
2021-02-02 19:26 ` Rafael J. Wysocki
@ 2021-02-03 9:12 ` Giovanni Gherdovich
1 sibling, 0 replies; 26+ messages in thread
From: Giovanni Gherdovich @ 2021-02-03 9:12 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Borislav Petkov, Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki,
Viresh Kumar, Jon Grimm, Nathan Fontenot, Yazen Ghannam,
Thomas Lendacky, Suthikulpanit Suravee, Mel Gorman, Pu Wen,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Michael Larabel,
the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List,
ACPI Devel Maling List
On Tue, 2021-02-02 at 19:59 +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 22, 2021 at 9:47 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote:
> >
>
> [cut]
>
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> > index 1e4fbb002a31..2378bc1bf2c4 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -27,6 +27,10 @@
> >
> > #include <acpi/processor.h>
> >
> > +#ifdef CONFIG_ACPI_CPPC_LIB
>
> Why is the #ifdef needed here?
>
Right, it isn't needed. The guard is necessary only later when the function
cppc_get_perf_caps() is used. I'll send a v3 with this correction.
Giovanni
> > +#include <acpi/cppc_acpi.h>
> > +#endif
> > +
> > #include <asm/msr.h>
> > #include <asm/processor.h>
> > #include <asm/cpufeature.h>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula
2021-01-22 20:40 ` [PATCH v2 1/1] x86,sched: On AMD EPYC set freq_max = max_boost in schedutil invariant formula Giovanni Gherdovich
` (2 preceding siblings ...)
2021-02-02 18:59 ` Rafael J. Wysocki
@ 2021-02-03 6:04 ` Viresh Kumar
3 siblings, 0 replies; 26+ messages in thread
From: Viresh Kumar @ 2021-02-03 6:04 UTC (permalink / raw)
To: Giovanni Gherdovich
Cc: Borislav Petkov, Ingo Molnar, Peter Zijlstra, Rafael J . Wysocki,
Jon Grimm, Nathan Fontenot, Yazen Ghannam, Thomas Lendacky,
Suthikulpanit Suravee, Mel Gorman, Pu Wen, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Michael Larabel, x86,
linux-pm, linux-kernel, linux-acpi
I am sorry but I wasn't able to get the full picture (not your fault,
it is me), but ...
On 22-01-21, 21:40, Giovanni Gherdovich wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d0a3525ce27f..b96677f6b57e 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2721,6 +2721,9 @@ int cpufreq_boost_enabled(void)
> }
> EXPORT_SYMBOL_GPL(cpufreq_boost_enabled);
>
> +DEFINE_STATIC_KEY_FALSE(cpufreq_amd_max_boost);
> +EXPORT_SYMBOL_GPL(cpufreq_amd_max_boost);
> +
> /*********************************************************************
> * REGISTER / UNREGISTER CPUFREQ DRIVER *
> *********************************************************************/
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9c8b7437b6cd..341cac76d254 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -40,9 +40,14 @@ enum cpufreq_table_sorting {
> CPUFREQ_TABLE_SORTED_DESCENDING
> };
>
> +DECLARE_STATIC_KEY_FALSE(cpufreq_amd_max_boost);
> +
> +#define cpufreq_driver_has_max_boost() static_branch_unlikely(&cpufreq_amd_max_boost)
> +
I am not happy with AMD specific code/changes in common parts..
> struct cpufreq_cpuinfo {
> unsigned int max_freq;
> unsigned int min_freq;
> + unsigned int max_boost;
>
> /* in 10^(-9) s = nanoseconds */
> unsigned int transition_latency;
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 6931f0cdeb80..541f3db3f576 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -159,8 +159,12 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> unsigned long util, unsigned long max)
> {
> struct cpufreq_policy *policy = sg_policy->policy;
> - unsigned int freq = arch_scale_freq_invariant() ?
> - policy->cpuinfo.max_freq : policy->cur;
> + unsigned int freq, max_freq;
> +
> + max_freq = cpufreq_driver_has_max_boost() ?
> + policy->cpuinfo.max_boost : policy->cpuinfo.max_freq;
Also, can't we update max_freq itself from the cpufreq driver? What
troubles will it cost ?
> +
> + freq = arch_scale_freq_invariant() ? max_freq : policy->cur;
>
> freq = map_util_freq(util, freq, max);
>
> --
> 2.26.2
--
viresh
^ permalink raw reply [flat|nested] 26+ messages in thread