All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Peter Zijlstra <peterz@infradead.org>,
	Linux PM list <linux-pm@vger.kernel.org>
Cc: kernel test robot <ying.huang@linux.intel.com>,
	Steve Muckle <steve.muckle@linaro.org>,
	lkp@01.org, linux-kernel@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Patrick Bellasi <patrick.bellasi@arm.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>,
	Mike Galbraith <efault@gmx.de>,
	Michael Turquette <mturquette@baylibre.com>,
	Juri Lelli <Juri.Lelli@arm.com>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steve Muckle <smuckle@linaro.org>, Ingo Molnar <mingo@kernel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Subject: [PATCH] intel_pstate: Fix intel_pstate_get()
Date: Wed, 04 May 2016 14:01:10 +0200	[thread overview]
Message-ID: <54177738.QIcPrzf0Y7@vostro.rjw.lan> (raw)
In-Reply-To: <20160503083231.GG3430@twins.programming.kicks-ass.net>

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 8fa520af5081 "intel_pstate: Remove freq calculation from
intel_pstate_calc_busy()" intel_pstate_get() calls get_avg_frequency()
to compute the average frequency, which is problematic for two reasons.

First, intel_pstate_get() may be invoked before the driver reads the
CPU feedback registers for the first time and if that happens,
get_avg_frequency() will attempt to divide by zero.

Second, the get_avg_frequency() call in intel_pstate_get() is racy
with respect to intel_pstate_sample() and it may end up returning
completely meaningless values for this reason.

Moreover, after commit 7349ec0470b6 "intel_pstate: Move
intel_pstate_calc_busy() into get_target_pstate_use_performance()"
sample.core_pct_busy is never computed on Atom, but it is used in
intel_pstate_adjust_busy_pstate() in that case too.

To address those problems notice that if sample.core_pct_busy
was used in the average frequency computation carried out by
get_avg_frequency(), both the divide by zero problem and the
race with respect to intel_pstate_sample() would be avoided.

Accordingly, move the invocation of intel_pstate_calc_busy() from
get_target_pstate_use_performance() to intel_pstate_update_util(),
which also will take care of the uninitialized sample.core_pct_busy
on Atom, and modify get_avg_frequency() to use sample.core_pct_busy
as per the above.

Reported-by: kernel test robot <ying.huang@linux.intel.com>
Link: http://marc.info/?l=linux-kernel&m=146226437623173&w=4
Fixes: 8fa520af5081 "intel_pstate: Remove freq calculation from intel_pstate_calc_busy()"
Fixes: 7349ec0470b6 "intel_pstate: Move intel_pstate_calc_busy() into get_target_pstate_use_performance()"
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -928,8 +928,9 @@ static inline bool intel_pstate_sample(s
 
 static inline int32_t get_avg_frequency(struct cpudata *cpu)
 {
-	return div64_u64(cpu->pstate.max_pstate_physical * cpu->sample.aperf *
-		cpu->pstate.scaling, cpu->sample.mperf);
+	return fp_toint(mul_fp(cpu->sample.core_pct_busy,
+			       int_tofp(cpu->pstate.max_pstate_physical *
+						cpu->pstate.scaling / 100)));
 }
 
 static inline int32_t get_target_pstate_use_cpu_load(struct cpudata *cpu)
@@ -972,8 +973,6 @@ static inline int32_t get_target_pstate_
 	int32_t core_busy, max_pstate, current_pstate, sample_ratio;
 	u64 duration_ns;
 
-	intel_pstate_calc_busy(cpu);
-
 	/*
 	 * core_busy is the ratio of actual performance to max
 	 * max_pstate is the max non turbo pstate available
@@ -1056,8 +1055,11 @@ static void intel_pstate_update_util(str
 	if ((s64)delta_ns >= pid_params.sample_rate_ns) {
 		bool sample_taken = intel_pstate_sample(cpu, time);
 
-		if (sample_taken && !hwp_active)
-			intel_pstate_adjust_busy_pstate(cpu);
+		if (sample_taken) {
+			intel_pstate_calc_busy(cpu);
+			if (!hwp_active)
+				intel_pstate_adjust_busy_pstate(cpu);
+		}
 	}
 }
 

WARNING: multiple messages have this Message-ID (diff)
From: Rafael J. Wysocki <rjw@rjwysocki.net>
To: lkp@lists.01.org
Subject: [PATCH] intel_pstate: Fix intel_pstate_get()
Date: Wed, 04 May 2016 14:01:10 +0200	[thread overview]
Message-ID: <54177738.QIcPrzf0Y7@vostro.rjw.lan> (raw)
In-Reply-To: <20160503083231.GG3430@twins.programming.kicks-ass.net>

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

After commit 8fa520af5081 "intel_pstate: Remove freq calculation from
intel_pstate_calc_busy()" intel_pstate_get() calls get_avg_frequency()
to compute the average frequency, which is problematic for two reasons.

First, intel_pstate_get() may be invoked before the driver reads the
CPU feedback registers for the first time and if that happens,
get_avg_frequency() will attempt to divide by zero.

Second, the get_avg_frequency() call in intel_pstate_get() is racy
with respect to intel_pstate_sample() and it may end up returning
completely meaningless values for this reason.

Moreover, after commit 7349ec0470b6 "intel_pstate: Move
intel_pstate_calc_busy() into get_target_pstate_use_performance()"
sample.core_pct_busy is never computed on Atom, but it is used in
intel_pstate_adjust_busy_pstate() in that case too.

To address those problems notice that if sample.core_pct_busy
was used in the average frequency computation carried out by
get_avg_frequency(), both the divide by zero problem and the
race with respect to intel_pstate_sample() would be avoided.

Accordingly, move the invocation of intel_pstate_calc_busy() from
get_target_pstate_use_performance() to intel_pstate_update_util(),
which also will take care of the uninitialized sample.core_pct_busy
on Atom, and modify get_avg_frequency() to use sample.core_pct_busy
as per the above.

Reported-by: kernel test robot <ying.huang@linux.intel.com>
Link: http://marc.info/?l=linux-kernel&m=146226437623173&w=4
Fixes: 8fa520af5081 "intel_pstate: Remove freq calculation from intel_pstate_calc_busy()"
Fixes: 7349ec0470b6 "intel_pstate: Move intel_pstate_calc_busy() into get_target_pstate_use_performance()"
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -928,8 +928,9 @@ static inline bool intel_pstate_sample(s
 
 static inline int32_t get_avg_frequency(struct cpudata *cpu)
 {
-	return div64_u64(cpu->pstate.max_pstate_physical * cpu->sample.aperf *
-		cpu->pstate.scaling, cpu->sample.mperf);
+	return fp_toint(mul_fp(cpu->sample.core_pct_busy,
+			       int_tofp(cpu->pstate.max_pstate_physical *
+						cpu->pstate.scaling / 100)));
 }
 
 static inline int32_t get_target_pstate_use_cpu_load(struct cpudata *cpu)
@@ -972,8 +973,6 @@ static inline int32_t get_target_pstate_
 	int32_t core_busy, max_pstate, current_pstate, sample_ratio;
 	u64 duration_ns;
 
-	intel_pstate_calc_busy(cpu);
-
 	/*
 	 * core_busy is the ratio of actual performance to max
 	 * max_pstate is the max non turbo pstate available
@@ -1056,8 +1055,11 @@ static void intel_pstate_update_util(str
 	if ((s64)delta_ns >= pid_params.sample_rate_ns) {
 		bool sample_taken = intel_pstate_sample(cpu, time);
 
-		if (sample_taken && !hwp_active)
-			intel_pstate_adjust_busy_pstate(cpu);
+		if (sample_taken) {
+			intel_pstate_calc_busy(cpu);
+			if (!hwp_active)
+				intel_pstate_adjust_busy_pstate(cpu);
+		}
 	}
 }
 


  parent reply	other threads:[~2016-05-04 11:58 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-03  1:10 [sched/fair] 41e0d37f7a: divide error: 0000 [#1] SMP kernel test robot
2016-05-03  8:32 ` [lkp] " Peter Zijlstra
2016-05-03  8:32   ` Peter Zijlstra
2016-05-03  9:19   ` [lkp] " Wanpeng Li
2016-05-03  9:25     ` Wanpeng Li
2016-05-03 13:33       ` Rafael J. Wysocki
2016-05-03 13:33         ` Rafael J. Wysocki
2016-05-04  0:53         ` [lkp] " Wanpeng Li
2016-05-04 11:41           ` Rafael J. Wysocki
2016-05-04 11:41             ` Rafael J. Wysocki
2016-05-04 11:46             ` [lkp] " Wanpeng Li
2016-05-03 12:15   ` Rafael J. Wysocki
2016-05-03 12:15     ` Rafael J. Wysocki
2016-05-03 12:54     ` [lkp] " Rafael J. Wysocki
2016-05-03 12:54       ` Rafael J. Wysocki
2016-05-03 12:58       ` [lkp] " Rafael J. Wysocki
2016-05-03 12:58         ` Rafael J. Wysocki
2016-05-03 13:22         ` [lkp] " Rafael J. Wysocki
2016-05-03 13:22           ` Rafael J. Wysocki
2016-05-03 13:53           ` [lkp] " Rafael J. Wysocki
2016-05-03 13:53             ` Rafael J. Wysocki
2016-05-03 15:10             ` [lkp] " Rafael J. Wysocki
2016-05-03 15:10               ` Rafael J. Wysocki
2016-05-05  5:05               ` [lkp] " Wanpeng Li
2016-05-05 13:46                 ` Rafael J. Wysocki
2016-05-05 13:46                   ` Rafael J. Wysocki
2016-05-06  7:06                   ` [lkp] " Wanpeng Li
2016-05-06 12:29                     ` Rafael J. Wysocki
2016-05-06 12:29                       ` Rafael J. Wysocki
2016-05-04  0:58     ` [lkp] " Wanpeng Li
2016-05-04 11:44       ` Rafael J. Wysocki
2016-05-04 11:44         ` Rafael J. Wysocki
2016-05-04 11:51         ` [lkp] " Wanpeng Li
2016-05-04 11:56           ` Rafael J. Wysocki
2016-05-04 11:56             ` Rafael J. Wysocki
2016-05-04 12:04             ` [lkp] " Wanpeng Li
2016-05-04 12:01   ` Rafael J. Wysocki [this message]
2016-05-04 12:01     ` [PATCH] intel_pstate: Fix intel_pstate_get() Rafael J. Wysocki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54177738.QIcPrzf0Y7@vostro.rjw.lan \
    --to=rjw@rjwysocki.net \
    --cc=Juri.Lelli@arm.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=efault@gmx.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lkp@01.org \
    --cc=mingo@kernel.org \
    --cc=morten.rasmussen@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=patrick.bellasi@arm.com \
    --cc=peterz@infradead.org \
    --cc=rafael@kernel.org \
    --cc=smuckle@linaro.org \
    --cc=srinivas.pandruvada@linux.intel.com \
    --cc=steve.muckle@linaro.org \
    --cc=tglx@linutronix.de \
    --cc=vincent.guittot@linaro.org \
    --cc=ying.huang@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.