* [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
* [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 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 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 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 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
* 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 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 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 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
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).