[1/2] x86, sched: Prevent divisions by zero in frequency invariant accounting
diff mbox series

Message ID 20200428132450.24901-2-ggherdovich@suse.cz
State New, archived
Headers show
Series
  • More frequency invariance fixes for x86
Related show

Commit Message

Giovanni Gherdovich April 28, 2020, 1:24 p.m. UTC
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(-)

Comments

Rafael J. Wysocki April 29, 2020, 11:30 a.m. UTC | #1
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
>
Peter Zijlstra May 1, 2020, 1:30 p.m. UTC | #2
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);
 }
Giovanni Gherdovich May 2, 2020, 2:25 p.m. UTC | #3
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);
>  }
Ricardo Neri May 18, 2020, 10:20 p.m. UTC | #4
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
Giovanni Gherdovich May 19, 2020, 4:46 p.m. UTC | #5
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

Patch
diff mbox series

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);