* [PATCH 0/2] More frequency invariance fixes for x86
@ 2020-04-28 13:24 Giovanni Gherdovich
2020-04-28 13:24 ` [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting Giovanni Gherdovich
2020-04-28 13:24 ` [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Giovanni Gherdovich
0 siblings, 2 replies; 13+ messages in thread
From: Giovanni Gherdovich @ 2020-04-28 13:24 UTC (permalink / raw)
To: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar,
Peter Zijlstra, Borislav Petkov, Len Brown, Rafael J . Wysocki
Cc: x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds,
Giovanni Gherdovich
Patch 1/2 prevents a division by zero in case the product
"delta_MPERF * arch_max_freq_ratio" overflows u64, as suggested by Linus at [1].
This patch supersedes the version at [2], as it also disables frequency
invariance when that overflow happens.
Patch 2/2 implements the recommendation by Ricardo Neri to check for an all
zero MSR_TURBO_RATIO_LIMIT and disable freq invariance in that case too.
[1] https://lore.kernel.org/lkml/CAHk-=wiX+NT2yxtdPszH9U_S96MCNQA56GJFXY45mZc47yG5KQ@mail.gmail.com/
[2] https://lore.kernel.org/lkml/20200422144055.18171-1-ggherdovich@suse.cz/
[3] https://lore.kernel.org/lkml/20200424013222.GA26355@ranerica-svr.sc.intel.com/
Giovanni Gherdovich (2):
x86, sched: Prevent divisions by zero in frequency invariant
accounting
x86, sched: Bail out of frequency invariance if turbo frequency is
unknown
arch/x86/kernel/smpboot.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
--
2.16.4
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting 2020-04-28 13:24 [PATCH 0/2] More frequency invariance fixes for x86 Giovanni Gherdovich @ 2020-04-28 13:24 ` Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki 2020-05-01 13:30 ` Peter Zijlstra 2020-04-28 13:24 ` [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Giovanni Gherdovich 1 sibling, 2 replies; 13+ messages in thread From: Giovanni Gherdovich @ 2020-04-28 13:24 UTC (permalink / raw) To: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Borislav Petkov, Len Brown, Rafael J . Wysocki Cc: x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds, Giovanni Gherdovich The product mcnt * arch_max_freq_ratio could be zero if it overflows u64. For context, a large value for arch_max_freq_ratio would be 5000, corresponding to a turbo_freq/base_freq ratio of 5 (normally it's more like 1500-2000). A large increment frequency for the MPERF counter would be 5GHz (the base clock of all CPUs on the market today is less than that). With these figures, a CPU would need to go without a scheduler tick for around 8 days for the u64 overflow to happen. It is unlikely, but the check is warranted. In that case it's also appropriate to disable frequency invariant accounting: the feature relies on measures of the clock frequency done at every scheduler tick, which need to be "fresh" to be at all meaningful. Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") --- arch/x86/kernel/smpboot.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 8c89e4d9ad28..4718f29a3065 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -2039,6 +2039,14 @@ static void init_freq_invariance(bool secondary) } } +static void disable_freq_invariance_workfn(struct work_struct *work) +{ + static_branch_disable(&arch_scale_freq_key); +} + +static DECLARE_WORK(disable_freq_invariance_work, + disable_freq_invariance_workfn); + DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE; void arch_scale_freq_tick(void) @@ -2055,14 +2063,18 @@ void arch_scale_freq_tick(void) acnt = aperf - this_cpu_read(arch_prev_aperf); mcnt = mperf - this_cpu_read(arch_prev_mperf); - if (!mcnt) - return; this_cpu_write(arch_prev_aperf, aperf); this_cpu_write(arch_prev_mperf, mperf); acnt <<= 2*SCHED_CAPACITY_SHIFT; mcnt *= arch_max_freq_ratio; + if (!mcnt) { + pr_warn("Scheduler tick missing for long time, disabling scale-invariant accounting.\n"); + /* static_branch_disable() acquires a lock and may sleep */ + schedule_work(&disable_freq_invariance_work); + return; + } freq_scale = div64_u64(acnt, mcnt); -- 2.16.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting 2020-04-28 13:24 ` [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting Giovanni Gherdovich @ 2020-04-29 11:30 ` Rafael J. Wysocki 2020-05-01 13:30 ` Peter Zijlstra 1 sibling, 0 replies; 13+ messages in thread From: Rafael J. Wysocki @ 2020-04-29 11:30 UTC (permalink / raw) To: Giovanni Gherdovich Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Borislav Petkov, Len Brown, Rafael J . Wysocki, the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List, Ricardo Neri, Linus Torvalds On Tue, Apr 28, 2020 at 3:25 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote: > > The product mcnt * arch_max_freq_ratio could be zero if it overflows u64. > > For context, a large value for arch_max_freq_ratio would be 5000, > corresponding to a turbo_freq/base_freq ratio of 5 (normally it's more like > 1500-2000). A large increment frequency for the MPERF counter would be 5GHz > (the base clock of all CPUs on the market today is less than that). With > these figures, a CPU would need to go without a scheduler tick for around 8 > days for the u64 overflow to happen. It is unlikely, but the check is > warranted. > > In that case it's also appropriate to disable frequency invariant > accounting: the feature relies on measures of the clock frequency done at > every scheduler tick, which need to be "fresh" to be at all meaningful. > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > arch/x86/kernel/smpboot.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 8c89e4d9ad28..4718f29a3065 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -2039,6 +2039,14 @@ static void init_freq_invariance(bool secondary) > } > } > > +static void disable_freq_invariance_workfn(struct work_struct *work) > +{ > + static_branch_disable(&arch_scale_freq_key); > +} > + > +static DECLARE_WORK(disable_freq_invariance_work, > + disable_freq_invariance_workfn); > + > DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE; > > void arch_scale_freq_tick(void) > @@ -2055,14 +2063,18 @@ void arch_scale_freq_tick(void) > > acnt = aperf - this_cpu_read(arch_prev_aperf); > mcnt = mperf - this_cpu_read(arch_prev_mperf); > - if (!mcnt) > - return; > > this_cpu_write(arch_prev_aperf, aperf); > this_cpu_write(arch_prev_mperf, mperf); > > acnt <<= 2*SCHED_CAPACITY_SHIFT; > mcnt *= arch_max_freq_ratio; > + if (!mcnt) { > + pr_warn("Scheduler tick missing for long time, disabling scale-invariant accounting.\n"); > + /* static_branch_disable() acquires a lock and may sleep */ > + schedule_work(&disable_freq_invariance_work); > + return; > + } > > freq_scale = div64_u64(acnt, mcnt); > > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting 2020-04-28 13:24 ` [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki @ 2020-05-01 13:30 ` Peter Zijlstra 2020-05-02 14:25 ` Giovanni Gherdovich 1 sibling, 1 reply; 13+ messages in thread From: Peter Zijlstra @ 2020-05-01 13:30 UTC (permalink / raw) To: Giovanni Gherdovich Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds On Tue, Apr 28, 2020 at 03:24:49PM +0200, Giovanni Gherdovich wrote: > The product mcnt * arch_max_freq_ratio could be zero if it overflows u64. > > For context, a large value for arch_max_freq_ratio would be 5000, > corresponding to a turbo_freq/base_freq ratio of 5 (normally it's more like > 1500-2000). A large increment frequency for the MPERF counter would be 5GHz > (the base clock of all CPUs on the market today is less than that). With > these figures, a CPU would need to go without a scheduler tick for around 8 > days for the u64 overflow to happen. It is unlikely, but the check is > warranted. > > In that case it's also appropriate to disable frequency invariant > accounting: the feature relies on measures of the clock frequency done at > every scheduler tick, which need to be "fresh" to be at all meaningful. > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") > acnt <<= 2*SCHED_CAPACITY_SHIFT; > mcnt *= arch_max_freq_ratio; > + if (!mcnt) { The problem is; this doesn't do what you claim it does. > + pr_warn("Scheduler tick missing for long time, disabling scale-invariant accounting.\n"); > + /* static_branch_disable() acquires a lock and may sleep */ > + schedule_work(&disable_freq_invariance_work); > + return; > + } > > freq_scale = div64_u64(acnt, mcnt); I've changed the patch like so.. OK? (ok, perhaps I went a little overboard with the paranoia ;-) --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -55,6 +55,7 @@ #include <linux/gfp.h> #include <linux/cpuidle.h> #include <linux/numa.h> +#include <linux/overflow.h> #include <asm/acpi.h> #include <asm/desc.h> @@ -2057,11 +2058,19 @@ static void init_freq_invariance(bool se } } +static void disable_freq_invariance_workfn(struct work_struct *work) +{ + static_branch_disable(&arch_scale_freq_key); +} + +static DECLARE_WORK(disable_freq_invariance_work, + disable_freq_invariance_workfn); + DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE; void arch_scale_freq_tick(void) { - u64 freq_scale; + u64 freq_scale = SCHED_CAPACITY_SCALE; u64 aperf, mperf; u64 acnt, mcnt; @@ -2073,19 +2082,27 @@ void arch_scale_freq_tick(void) acnt = aperf - this_cpu_read(arch_prev_aperf); mcnt = mperf - this_cpu_read(arch_prev_mperf); - if (!mcnt) - return; this_cpu_write(arch_prev_aperf, aperf); this_cpu_write(arch_prev_mperf, mperf); - acnt <<= 2*SCHED_CAPACITY_SHIFT; - mcnt *= arch_max_freq_ratio; + if (check_shl_overflow(acnt, 2*SCHED_CAPACITY_SHIFT, &acnt)) + goto error; + + if (check_mul_overflow(mcnt, arch_max_freq_ratio, &mcnt) || !mcnt) + goto error; freq_scale = div64_u64(acnt, mcnt); + if (!freq_scale) + goto error; if (freq_scale > SCHED_CAPACITY_SCALE) freq_scale = SCHED_CAPACITY_SCALE; this_cpu_write(arch_freq_scale, freq_scale); + return; + +error: + pr_warn("Scheduler frequency invariance went wobbly, disabling!\n"); + schedule_work(&disable_freq_invariance_work); } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting 2020-05-01 13:30 ` Peter Zijlstra @ 2020-05-02 14:25 ` Giovanni Gherdovich 2020-05-18 22:20 ` Ricardo Neri 0 siblings, 1 reply; 13+ messages in thread From: Giovanni Gherdovich @ 2020-05-02 14:25 UTC (permalink / raw) To: Peter Zijlstra Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds On Fri, 2020-05-01 at 15:30 +0200, Peter Zijlstra wrote: > On Tue, Apr 28, 2020 at 03:24:49PM +0200, Giovanni Gherdovich wrote: > > The product mcnt * arch_max_freq_ratio could be zero if it overflows u64. > > > > For context, a large value for arch_max_freq_ratio would be 5000, > > corresponding to a turbo_freq/base_freq ratio of 5 (normally it's more like > > 1500-2000). A large increment frequency for the MPERF counter would be 5GHz > > (the base clock of all CPUs on the market today is less than that). With > > these figures, a CPU would need to go without a scheduler tick for around 8 > > days for the u64 overflow to happen. It is unlikely, but the check is > > warranted. > > > > In that case it's also appropriate to disable frequency invariant > > accounting: the feature relies on measures of the clock frequency done at > > every scheduler tick, which need to be "fresh" to be at all meaningful. > > > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") > > acnt <<= 2*SCHED_CAPACITY_SHIFT; > > mcnt *= arch_max_freq_ratio; > > + if (!mcnt) { > > The problem is; this doesn't do what you claim it does. > > > + pr_warn("Scheduler tick missing for long time, disabling scale-invariant accounting.\n"); > > + /* static_branch_disable() acquires a lock and may sleep */ > > + schedule_work(&disable_freq_invariance_work); > > + return; > > + } > > > > freq_scale = div64_u64(acnt, mcnt); > > I've changed the patch like so.. OK? > > (ok, perhaps I went a little overboard with the paranoia ;-) Right, I wasn't really checking for overflow, only for when the product "mcnt * arch_max_freq_ratio" becomes zero. Thanks for your edit (I took note of the macros check_*_overflow, didn't know them). I fully subscribe to the paranoid approach. I understand you've already edited the patches in your tree, so I am not resending, just confirming my Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -55,6 +55,7 @@ > #include <linux/gfp.h> > #include <linux/cpuidle.h> > #include <linux/numa.h> > +#include <linux/overflow.h> > > #include <asm/acpi.h> > #include <asm/desc.h> > @@ -2057,11 +2058,19 @@ static void init_freq_invariance(bool se > } > } > > +static void disable_freq_invariance_workfn(struct work_struct *work) > +{ > + static_branch_disable(&arch_scale_freq_key); > +} > + > +static DECLARE_WORK(disable_freq_invariance_work, > + disable_freq_invariance_workfn); > + > DEFINE_PER_CPU(unsigned long, arch_freq_scale) = SCHED_CAPACITY_SCALE; > > void arch_scale_freq_tick(void) > { > - u64 freq_scale; > + u64 freq_scale = SCHED_CAPACITY_SCALE; > u64 aperf, mperf; > u64 acnt, mcnt; > > @@ -2073,19 +2082,27 @@ void arch_scale_freq_tick(void) > > acnt = aperf - this_cpu_read(arch_prev_aperf); > mcnt = mperf - this_cpu_read(arch_prev_mperf); > - if (!mcnt) > - return; > > this_cpu_write(arch_prev_aperf, aperf); > this_cpu_write(arch_prev_mperf, mperf); > > - acnt <<= 2*SCHED_CAPACITY_SHIFT; > - mcnt *= arch_max_freq_ratio; > + if (check_shl_overflow(acnt, 2*SCHED_CAPACITY_SHIFT, &acnt)) > + goto error; > + > + if (check_mul_overflow(mcnt, arch_max_freq_ratio, &mcnt) || !mcnt) > + goto error; > > freq_scale = div64_u64(acnt, mcnt); > + if (!freq_scale) > + goto error; > > if (freq_scale > SCHED_CAPACITY_SCALE) > freq_scale = SCHED_CAPACITY_SCALE; > > this_cpu_write(arch_freq_scale, freq_scale); > + return; > + > +error: > + pr_warn("Scheduler frequency invariance went wobbly, disabling!\n"); > + schedule_work(&disable_freq_invariance_work); > } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting 2020-05-02 14:25 ` Giovanni Gherdovich @ 2020-05-18 22:20 ` Ricardo Neri 2020-05-19 16:46 ` Giovanni Gherdovich 0 siblings, 1 reply; 13+ messages in thread From: Ricardo Neri @ 2020-05-18 22:20 UTC (permalink / raw) To: Giovanni Gherdovich Cc: Peter Zijlstra, Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Linus Torvalds On Sat, May 02, 2020 at 04:25:00PM +0200, Giovanni Gherdovich wrote: > > > > I've changed the patch like so.. OK? > > > > (ok, perhaps I went a little overboard with the paranoia ;-) > > Right, I wasn't really checking for overflow, only for when the product > "mcnt * arch_max_freq_ratio" becomes zero. > > Thanks for your edit (I took note of the macros check_*_overflow, didn't know > them). I fully subscribe to the paranoid approach. > > I understand you've already edited the patches in your tree, so I am not > resending, just confirming my > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> Hi, have these changes been merged? I still don't see them in the tip or Linus' tree. Thanks and BR, Ricardo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting 2020-05-18 22:20 ` Ricardo Neri @ 2020-05-19 16:46 ` Giovanni Gherdovich 0 siblings, 0 replies; 13+ messages in thread From: Giovanni Gherdovich @ 2020-05-19 16:46 UTC (permalink / raw) To: Ricardo Neri Cc: Peter Zijlstra, Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Linus Torvalds On Mon, 2020-05-18 at 15:20 -0700, Ricardo Neri wrote: > On Sat, May 02, 2020 at 04:25:00PM +0200, Giovanni Gherdovich wrote: > > > > > > I've changed the patch like so.. OK? > > > > > > (ok, perhaps I went a little overboard with the paranoia ;-) > > > > Right, I wasn't really checking for overflow, only for when the product > > "mcnt * arch_max_freq_ratio" becomes zero. > > > > Thanks for your edit (I took note of the macros check_*_overflow, didn't know > > them). I fully subscribe to the paranoid approach. > > > > I understand you've already edited the patches in your tree, so I am not > > resending, just confirming my > > > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > > Hi, have these changes been merged? I still don't see them in the tip or > Linus' tree. > Hi Ricardo, the kbuild bot found an error in this patch, the macro check_mul_overflow doesn't build on x86 32bit, so Peter Zijlstra hasn't merged it yet. This is the error: https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/7GDIBOMNVDG5W2XZD4EICE2TUZR3THBN/ I'm writing a patch to avoid doing frequency invariance entirely on i386. I doubt those machines have APERFMPERF anyways. This will fix the build error. Cheers, Giovanni ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown 2020-04-28 13:24 [PATCH 0/2] More frequency invariance fixes for x86 Giovanni Gherdovich 2020-04-28 13:24 ` [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting Giovanni Gherdovich @ 2020-04-28 13:24 ` Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki ` (2 more replies) 1 sibling, 3 replies; 13+ messages in thread From: Giovanni Gherdovich @ 2020-04-28 13:24 UTC (permalink / raw) To: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Borislav Petkov, Len Brown, Rafael J . Wysocki Cc: x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds, Giovanni Gherdovich There may be CPUs that support turbo boost but don't declare any turbo ratio, i.e. their MSR_TURBO_RATIO_LIMIT is all zeroes. In that condition scale-invariant calculations can't be performed. Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") --- arch/x86/kernel/smpboot.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 4718f29a3065..ab2a0df7d1fb 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1991,9 +1991,11 @@ static bool intel_set_max_freq_ratio(void) /* * Some hypervisors advertise X86_FEATURE_APERFMPERF * but then fill all MSR's with zeroes. + * Some CPUs have turbo boost but don't declare any turbo ratio + * in MSR_TURBO_RATIO_LIMIT. */ - if (!base_freq) { - pr_debug("Couldn't determine cpu base frequency, necessary for scale-invariant accounting.\n"); + if (!base_freq || !turbo_freq) { + pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n"); return false; } -- 2.16.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown 2020-04-28 13:24 ` [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Giovanni Gherdovich @ 2020-04-29 11:30 ` Rafael J. Wysocki 2020-05-01 13:04 ` Peter Zijlstra 2020-05-02 0:04 ` Ricardo Neri 2 siblings, 0 replies; 13+ messages in thread From: Rafael J. Wysocki @ 2020-04-29 11:30 UTC (permalink / raw) To: Giovanni Gherdovich Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Borislav Petkov, Len Brown, Rafael J . Wysocki, the arch/x86 maintainers, Linux PM, Linux Kernel Mailing List, Ricardo Neri, Linus Torvalds On Tue, Apr 28, 2020 at 3:25 PM Giovanni Gherdovich <ggherdovich@suse.cz> wrote: > > There may be CPUs that support turbo boost but don't declare any turbo > ratio, i.e. their MSR_TURBO_RATIO_LIMIT is all zeroes. In that condition > scale-invariant calculations can't be performed. > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > arch/x86/kernel/smpboot.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 4718f29a3065..ab2a0df7d1fb 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1991,9 +1991,11 @@ static bool intel_set_max_freq_ratio(void) > /* > * Some hypervisors advertise X86_FEATURE_APERFMPERF > * but then fill all MSR's with zeroes. > + * Some CPUs have turbo boost but don't declare any turbo ratio > + * in MSR_TURBO_RATIO_LIMIT. > */ > - if (!base_freq) { > - pr_debug("Couldn't determine cpu base frequency, necessary for scale-invariant accounting.\n"); > + if (!base_freq || !turbo_freq) { > + pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n"); > return false; > } > > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown 2020-04-28 13:24 ` [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki @ 2020-05-01 13:04 ` Peter Zijlstra 2020-05-02 0:06 ` Ricardo Neri 2020-05-02 14:26 ` Giovanni Gherdovich 2020-05-02 0:04 ` Ricardo Neri 2 siblings, 2 replies; 13+ messages in thread From: Peter Zijlstra @ 2020-05-01 13:04 UTC (permalink / raw) To: Giovanni Gherdovich Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds On Tue, Apr 28, 2020 at 03:24:50PM +0200, Giovanni Gherdovich wrote: > There may be CPUs that support turbo boost but don't declare any turbo > ratio, i.e. their MSR_TURBO_RATIO_LIMIT is all zeroes. In that condition > scale-invariant calculations can't be performed. > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") > --- > arch/x86/kernel/smpboot.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 4718f29a3065..ab2a0df7d1fb 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1991,9 +1991,11 @@ static bool intel_set_max_freq_ratio(void) > /* > * Some hypervisors advertise X86_FEATURE_APERFMPERF > * but then fill all MSR's with zeroes. > + * Some CPUs have turbo boost but don't declare any turbo ratio > + * in MSR_TURBO_RATIO_LIMIT. > */ > - if (!base_freq) { > - pr_debug("Couldn't determine cpu base frequency, necessary for scale-invariant accounting.\n"); > + if (!base_freq || !turbo_freq) { > + pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n"); > return false; > } I've added the below, imagine base_freq > turbo_freq * SCHED_CAPACITY_SCALE. --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1975,6 +1975,7 @@ static bool core_set_max_freq_ratio(u64 static bool intel_set_max_freq_ratio(void) { u64 base_freq, turbo_freq; + u64 turbo_ratio; if (slv_set_max_freq_ratio(&base_freq, &turbo_freq)) goto out; @@ -2008,9 +2009,15 @@ static bool intel_set_max_freq_ratio(voi return false; } - arch_turbo_freq_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, - base_freq); + turbo_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, base_freq); + if (!turbo_ratio) { + pr_debug("Non-zero turbo and base frequencies led to a 0 ratio.\n"); + return false; + } + + arch_turbo_freq_ratio = turbo_ratio; arch_set_max_freq_ratio(turbo_disabled()); + return true; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown 2020-05-01 13:04 ` Peter Zijlstra @ 2020-05-02 0:06 ` Ricardo Neri 2020-05-02 14:26 ` Giovanni Gherdovich 1 sibling, 0 replies; 13+ messages in thread From: Ricardo Neri @ 2020-05-02 0:06 UTC (permalink / raw) To: Peter Zijlstra Cc: Giovanni Gherdovich, Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Linus Torvalds On Fri, May 01, 2020 at 03:04:27PM +0200, Peter Zijlstra wrote: > On Tue, Apr 28, 2020 at 03:24:50PM +0200, Giovanni Gherdovich wrote: > > There may be CPUs that support turbo boost but don't declare any turbo > > ratio, i.e. their MSR_TURBO_RATIO_LIMIT is all zeroes. In that condition > > scale-invariant calculations can't be performed. > > > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > > Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") > > --- > > arch/x86/kernel/smpboot.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > > index 4718f29a3065..ab2a0df7d1fb 100644 > > --- a/arch/x86/kernel/smpboot.c > > +++ b/arch/x86/kernel/smpboot.c > > @@ -1991,9 +1991,11 @@ static bool intel_set_max_freq_ratio(void) > > /* > > * Some hypervisors advertise X86_FEATURE_APERFMPERF > > * but then fill all MSR's with zeroes. > > + * Some CPUs have turbo boost but don't declare any turbo ratio > > + * in MSR_TURBO_RATIO_LIMIT. > > */ > > - if (!base_freq) { > > - pr_debug("Couldn't determine cpu base frequency, necessary for scale-invariant accounting.\n"); > > + if (!base_freq || !turbo_freq) { > > + pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n"); > > return false; > > } > > I've added the below, imagine base_freq > turbo_freq * > SCHED_CAPACITY_SCALE. > > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1975,6 +1975,7 @@ static bool core_set_max_freq_ratio(u64 > static bool intel_set_max_freq_ratio(void) > { > u64 base_freq, turbo_freq; > + u64 turbo_ratio; > > if (slv_set_max_freq_ratio(&base_freq, &turbo_freq)) > goto out; > @@ -2008,9 +2009,15 @@ static bool intel_set_max_freq_ratio(voi > return false; > } > > - arch_turbo_freq_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, > - base_freq); > + turbo_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, base_freq); > + if (!turbo_ratio) { > + pr_debug("Non-zero turbo and base frequencies led to a 0 ratio.\n"); > + return false; > + } > + > + arch_turbo_freq_ratio = turbo_ratio; I guess this covers more cases in which turbo_ratio can be zero. Also, FWIW Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Thanks and BR, Ricardo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown 2020-05-01 13:04 ` Peter Zijlstra 2020-05-02 0:06 ` Ricardo Neri @ 2020-05-02 14:26 ` Giovanni Gherdovich 1 sibling, 0 replies; 13+ messages in thread From: Giovanni Gherdovich @ 2020-05-02 14:26 UTC (permalink / raw) To: Peter Zijlstra Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Ricardo Neri, Linus Torvalds On Fri, 2020-05-01 at 15:04 +0200, Peter Zijlstra wrote: > On Tue, Apr 28, 2020 at 03:24:50PM +0200, Giovanni Gherdovich wrote: > > There may be CPUs that support turbo boost but don't declare any turbo > > ratio, i.e. their MSR_TURBO_RATIO_LIMIT is all zeroes. In that condition > > scale-invariant calculations can't be performed. > > > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > > Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > Fixes: 1567c3e3467c ("x86, sched: Add support for frequency invariance") > > --- > > arch/x86/kernel/smpboot.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > > index 4718f29a3065..ab2a0df7d1fb 100644 > > --- a/arch/x86/kernel/smpboot.c > > +++ b/arch/x86/kernel/smpboot.c > > @@ -1991,9 +1991,11 @@ static bool intel_set_max_freq_ratio(void) > > /* > > * Some hypervisors advertise X86_FEATURE_APERFMPERF > > * but then fill all MSR's with zeroes. > > + * Some CPUs have turbo boost but don't declare any turbo ratio > > + * in MSR_TURBO_RATIO_LIMIT. > > */ > > - if (!base_freq) { > > - pr_debug("Couldn't determine cpu base frequency, necessary for scale-invariant accounting.\n"); > > + if (!base_freq || !turbo_freq) { > > + pr_debug("Couldn't determine cpu base or turbo frequency, necessary for scale-invariant accounting.\n"); > > return false; > > } > > I've added the below, imagine base_freq > turbo_freq * > SCHED_CAPACITY_SCALE. Right, I didn't consider that case. It doesn't hurt to be defensive. I understand you've already edited the patches in your tree, so I am not resending, just confirming my Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1975,6 +1975,7 @@ static bool core_set_max_freq_ratio(u64 > static bool intel_set_max_freq_ratio(void) > { > u64 base_freq, turbo_freq; > + u64 turbo_ratio; > > if (slv_set_max_freq_ratio(&base_freq, &turbo_freq)) > goto out; > @@ -2008,9 +2009,15 @@ static bool intel_set_max_freq_ratio(voi > return false; > } > > - arch_turbo_freq_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, > - base_freq); > + turbo_ratio = div_u64(turbo_freq * SCHED_CAPACITY_SCALE, base_freq); > + if (!turbo_ratio) { > + pr_debug("Non-zero turbo and base frequencies led to a 0 ratio.\n"); > + return false; > + } > + > + arch_turbo_freq_ratio = turbo_ratio; > arch_set_max_freq_ratio(turbo_disabled()); > + > return true; > } > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown 2020-04-28 13:24 ` [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki 2020-05-01 13:04 ` Peter Zijlstra @ 2020-05-02 0:04 ` Ricardo Neri 2 siblings, 0 replies; 13+ messages in thread From: Ricardo Neri @ 2020-05-02 0:04 UTC (permalink / raw) To: Giovanni Gherdovich Cc: Srinivas Pandruvada, Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Borislav Petkov, Len Brown, Rafael J . Wysocki, x86, linux-pm, linux-kernel, Linus Torvalds On Tue, Apr 28, 2020 at 03:24:50PM +0200, Giovanni Gherdovich wrote: > There may be CPUs that support turbo boost but don't declare any turbo > ratio, i.e. their MSR_TURBO_RATIO_LIMIT is all zeroes. In that condition > scale-invariant calculations can't be performed. > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz> > Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Thanks for implementing this, Giovanni! Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-19 16:46 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-28 13:24 [PATCH 0/2] More frequency invariance fixes for x86 Giovanni Gherdovich 2020-04-28 13:24 ` [PATCH 1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki 2020-05-01 13:30 ` Peter Zijlstra 2020-05-02 14:25 ` Giovanni Gherdovich 2020-05-18 22:20 ` Ricardo Neri 2020-05-19 16:46 ` Giovanni Gherdovich 2020-04-28 13:24 ` [PATCH 2/2] x86, sched: Bail out of frequency invariance if turbo frequency is unknown Giovanni Gherdovich 2020-04-29 11:30 ` Rafael J. Wysocki 2020-05-01 13:04 ` Peter Zijlstra 2020-05-02 0:06 ` Ricardo Neri 2020-05-02 14:26 ` Giovanni Gherdovich 2020-05-02 0:04 ` Ricardo Neri
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).