All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
@ 2009-09-15  3:42 Arjan van de Ven
  2009-09-15  3:54 ` Andrew Morton
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Arjan van de Ven @ 2009-09-15  3:42 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Arjan van de Ven, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

Reworked patch based on Andrew's review feedback and spelling fixes.
Rather than adding a new governor temporarily, this just puts the fixes
into the existing menu governor.

Andrew: this replaces cpuidle-a-new-variant-of-the-menu-governor-to-boost-io-performance.patch


I don't have a power meter in my hotel room so I've only been able to verify
that the patch functions as before with the timechart tool.
(no major changes were done though, only review feedback)


From: Arjan van de Ven <arjan@linux.intel.com>
Subject: cpuidle: Fix the menu governor to boost IO performance

Fix the menu idle governor which balances power savings, energy efficiency
and performance impact.

The reason for a reworked governor is that there have been serious
performance issues reported with the existing code on Nehalem server
systems.

To show this I'm sure Andrew wants to see benchmark results:
(benchmark is "fio", "no cstates" is using "idle=poll")

		no cstates	current linux	new algorithm
1 disk		107 Mb/s	85 Mb/s		105 Mb/s
2 disks		215 Mb/s	123 Mb/s	209 Mb/s
12 disks	590 Mb/s	320 Mb/s	585 Mb/s

In various power benchmark measurements, no degredation was found by our
measurement&diagnostics team.  Obviously a small percentage more power 
was used in the "fio" benchmark, due to the much higher performance.

While it would be a novel idea to describe the new algorithm in this
commit message, I cheaped out and described it in comments in the code
instead.

[changes since first post: spelling fixes from akpm, review feedback,
folded menu-tng into menu.c]

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Len Brown <lenb@kernel.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Yanmin Zhang <yanmin_zhang@linux.intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/cpuidle/governors/menu.c |  251 ++++++++++++++++++++++++++++++++------
 include/linux/sched.h            |    4 +
 kernel/sched.c                   |   15 +++
 3 files changed, 231 insertions(+), 39 deletions(-)

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index f1df59f..9f3d775 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -2,8 +2,12 @@
  * menu.c - the menu idle governor
  *
  * Copyright (C) 2006-2007 Adam Belay <abelay@novell.com>
+ * Copyright (C) 2009 Intel Corporation
+ * Author:
+ *        Arjan van de Ven <arjan@linux.intel.com>
  *
- * This code is licenced under the GPL.
+ * This code is licenced under the GPL version 2 as described
+ * in the COPYING file that acompanies the Linux Kernel.
  */
 
 #include <linux/kernel.h>
@@ -13,20 +17,153 @@
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
 #include <linux/tick.h>
+#include <linux/sched.h>
 
-#define BREAK_FUZZ	4	/* 4 us */
-#define PRED_HISTORY_PCT	50
+#define BUCKETS 12
+#define RESOLUTION 1024
+#define DECAY 4
+#define MAX_INTERESTING 50000
+
+/*
+ * Concepts and ideas behind the menu governor
+ *
+ * For the menu governor, there are 3 decision factors for picking a C
+ * state:
+ * 1) Energy break even point
+ * 2) Performance impact
+ * 3) Latency tolerance (from pmqos infrastructure)
+ * These these three factors are treated independently.
+ *
+ * Energy break even point
+ * -----------------------
+ * C state entry and exit have an energy cost, and a certain amount of time in
+ * the  C state is required to actually break even on this cost. CPUIDLE
+ * provides us this duration in the "target_residency" field. So all that we
+ * need is a good prediction of how long we'll be idle. Like the traditional
+ * menu governor, we start with the actual known "next timer event" time.
+ *
+ * Since there are other source of wakeups (interrupts for example) than
+ * the next timer event, this estimation is rather optimistic. To get a
+ * more realistic estimate, a correction factor is applied to the estimate,
+ * that is based on historic behavior. For example, if in the past the actual
+ * duration always was 50% of the next timer tick, the correction factor will
+ * be 0.5.
+ *
+ * menu uses a running average for this correction factor, however it uses a
+ * set of factors, not just a single factor. This stems from the realization
+ * that the ratio is dependent on the order of magnitude of the expected
+ * duration; if we expect 500 milliseconds of idle time the likelihood of
+ * getting an interrupt very early is much higher than if we expect 50 micro
+ * seconds of idle time. A second independent factor that has big impact on
+ * the actual factor is if there is (disk) IO outstanding or not.
+ * (as a special twist, we consider every sleep longer than 50 milliseconds
+ * as perfect; there are no power gains for sleeping longer than this)
+ *
+ * For these two reasons we keep an array of 12 independent factors, that gets
+ * indexed based on the magnitude of the expected duration as well as the
+ * "is IO outstanding" property.
+ *
+ * Limiting Performance Impact
+ * ---------------------------
+ * C states, especially those with large exit latencies, can have a real
+ * noticable impact on workloads, which is not acceptable for most sysadmins,
+ * and in addition, less performance has a power price of its own.
+ *
+ * As a general rule of thumb, menu assumes that the following heuristic
+ * holds:
+ *     The busier the system, the less impact of C states is acceptable
+ *
+ * This rule-of-thumb is implemented using a performance-multiplier:
+ * If the exit latency times the performance multiplier is longer than
+ * the predicted duration, the C state is not considered a candidate
+ * for selection due to a too high performance impact. So the higher
+ * this multiplier is, the longer we need to be idle to pick a deep C
+ * state, and thus the less likely a busy CPU will hit such a deep
+ * C state.
+ *
+ * Two factors are used in determing this multiplier:
+ * a value of 10 is added for each point of "per cpu load average" we have.
+ * a value of 5 points is added for each process that is waiting for
+ * IO on this CPU.
+ * (these values are experimentally determined)
+ *
+ * The load average factor gives a longer term (few seconds) input to the
+ * decision, while the iowait value gives a cpu local instantanious input.
+ * The iowait factor may look low, but realize that this is also already
+ * represented in the system load average.
+ *
+ */
 
 struct menu_device {
 	int		last_state_idx;
 
 	unsigned int	expected_us;
-	unsigned int	predicted_us;
-	unsigned int    current_predicted_us;
-	unsigned int	last_measured_us;
-	unsigned int	elapsed_us;
+	u64		predicted_us;
+	unsigned int	measured_us;
+	unsigned int	exit_us;
+	unsigned int	bucket;
+	u64		correction_factor[BUCKETS];
 };
 
+
+#define LOAD_INT(x) ((x) >> FSHIFT)
+#define LOAD_FRAC(x) LOAD_INT(((x) & (FIXED_1-1)) * 100)
+
+static int get_loadavg(void)
+{
+	unsigned long this = this_cpu_load();
+
+
+	return LOAD_INT(this) * 10 + LOAD_FRAC(this) / 10;
+}
+
+static inline int which_bucket(unsigned int duration)
+{
+	int bucket = 0;
+
+	/*
+	 * We keep two groups of stats; one with no
+	 * IO pending, one without.
+	 * This allows us to calculate
+	 * E(duration)|iowait
+	 */
+	if (nr_iowait_cpu())
+		bucket = BUCKETS/2;
+
+	if (duration < 10)
+		return bucket;
+	if (duration < 100)
+		return bucket + 1;
+	if (duration < 1000)
+		return bucket + 2;
+	if (duration < 10000)
+		return bucket + 3;
+	if (duration < 100000)
+		return bucket + 4;
+	return bucket + 5;
+}
+
+/*
+ * Return a multiplier for the exit latency that is intended
+ * to take performance requirements into account.
+ * The more performance critical we estimate the system
+ * to be, the higher this multiplier, and thus the higher
+ * the barrier to go to an expensive C state.
+ */
+static inline int performance_multiplier(void)
+{
+	int mult = 1;
+
+	/* for higher loadavg, we are more reluctant */
+
+	mult += 2 * get_loadavg();
+
+	/* for IO wait tasks (per cpu!) we add 5x each */
+	mult += 10 * nr_iowait_cpu();
+
+	return mult;
+}
+
 static DEFINE_PER_CPU(struct menu_device, menu_devices);
 
 /**
@@ -38,37 +175,59 @@ static int menu_select(struct cpuidle_device *dev)
 	struct menu_device *data = &__get_cpu_var(menu_devices);
 	int latency_req = pm_qos_requirement(PM_QOS_CPU_DMA_LATENCY);
 	int i;
+	int multiplier;
+
+	data->last_state_idx = 0;
+	data->exit_us = 0;
 
 	/* Special case when user has set very strict latency requirement */
-	if (unlikely(latency_req == 0)) {
-		data->last_state_idx = 0;
+	if (unlikely(latency_req == 0))
 		return 0;
-	}
 
-	/* determine the expected residency time */
+	/* determine the expected residency time, round up */
 	data->expected_us =
-		(u32) ktime_to_ns(tick_nohz_get_sleep_length()) / 1000;
+	    DIV_ROUND_UP((u32)ktime_to_ns(tick_nohz_get_sleep_length()), 1000);
+
+
+	data->bucket = which_bucket(data->expected_us);
+
+	multiplier = performance_multiplier();
+
+	/*
+	 * if the correction factor is 0 (eg first time init or cpu hotplug
+	 * etc), we actually want to start out with a unity factor.
+	 */
+	if (data->correction_factor[data->bucket] == 0)
+		data->correction_factor[data->bucket] = RESOLUTION * DECAY;
+
+	/* Make sure to round up for half microseconds */
+	data->predicted_us = DIV_ROUND_CLOSEST(
+		data->expected_us * data->correction_factor[data->bucket],
+		RESOLUTION * DECAY);
+
+	/*
+	 * We want to default to C1 (hlt), not to busy polling
+	 * unless the timer is happening really really soon.
+	 */
+	if (data->expected_us > 5)
+		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
 
-	/* Recalculate predicted_us based on prediction_history_pct */
-	data->predicted_us *= PRED_HISTORY_PCT;
-	data->predicted_us += (100 - PRED_HISTORY_PCT) *
-				data->current_predicted_us;
-	data->predicted_us /= 100;
 
 	/* find the deepest idle state that satisfies our constraints */
-	for (i = CPUIDLE_DRIVER_STATE_START + 1; i < dev->state_count; i++) {
+	for (i = CPUIDLE_DRIVER_STATE_START; i < dev->state_count; i++) {
 		struct cpuidle_state *s = &dev->states[i];
 
-		if (s->target_residency > data->expected_us)
-			break;
 		if (s->target_residency > data->predicted_us)
 			break;
 		if (s->exit_latency > latency_req)
 			break;
+		if (s->exit_latency * multiplier > data->predicted_us)
+			break;
+		data->exit_us = s->exit_latency;
+		data->last_state_idx = i;
 	}
 
-	data->last_state_idx = i - 1;
-	return i - 1;
+	return data->last_state_idx;
 }
 
 /**
@@ -85,35 +244,49 @@ static void menu_reflect(struct cpuidle_device *dev)
 	unsigned int last_idle_us = cpuidle_get_last_residency(dev);
 	struct cpuidle_state *target = &dev->states[last_idx];
 	unsigned int measured_us;
+	u64 new_factor;
 
 	/*
 	 * Ugh, this idle state doesn't support residency measurements, so we
 	 * are basically lost in the dark.  As a compromise, assume we slept
-	 * for one full standard timer tick.  However, be aware that this
-	 * could potentially result in a suboptimal state transition.
+	 * for the whole expected time.
 	 */
 	if (unlikely(!(target->flags & CPUIDLE_FLAG_TIME_VALID)))
-		last_idle_us = USEC_PER_SEC / HZ;
+		last_idle_us = data->expected_us;
+
+
+	measured_us = last_idle_us;
 
 	/*
-	 * measured_us and elapsed_us are the cumulative idle time, since the
-	 * last time we were woken out of idle by an interrupt.
+	 * We correct for the exit latency; we are assuming here that the
+	 * exit latency happens after the event that we're interested in.
 	 */
-	if (data->elapsed_us <= data->elapsed_us + last_idle_us)
-		measured_us = data->elapsed_us + last_idle_us;
+	if (measured_us > data->exit_us)
+		measured_us -= data->exit_us;
+
+
+	/* update our correction ratio */
+
+	new_factor = data->correction_factor[data->bucket]
+			* (DECAY - 1) / DECAY;
+
+	if (data->expected_us > 0 && data->measured_us < MAX_INTERESTING)
+		new_factor += RESOLUTION * measured_us / data->expected_us;
 	else
-		measured_us = -1;
+		/*
+		 * we were idle so long that we count it as a perfect
+		 * prediction
+		 */
+		new_factor += RESOLUTION;
 
-	/* Predict time until next break event */
-	data->current_predicted_us = max(measured_us, data->last_measured_us);
+	/*
+	 * We don't want 0 as factor; we always want at least
+	 * a tiny bit of estimated time.
+	 */
+	if (new_factor == 0)
+		new_factor = 1;
 
-	if (last_idle_us + BREAK_FUZZ <
-	    data->expected_us - target->exit_latency) {
-		data->last_measured_us = measured_us;
-		data->elapsed_us = 0;
-	} else {
-		data->elapsed_us = measured_us;
-	}
+	data->correction_factor[data->bucket] = new_factor;
 }
 
 /**
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c0d9944..6b29155 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -140,6 +140,10 @@ extern int nr_processes(void);
 extern unsigned long nr_running(void);
 extern unsigned long nr_uninterruptible(void);
 extern unsigned long nr_iowait(void);
+extern unsigned long nr_iowait_cpu(void);
+extern unsigned long this_cpu_load(void);
+
+
 extern void calc_global_load(void);
 extern u64 cpu_nr_migrations(int cpu);
 
diff --git a/kernel/sched.c b/kernel/sched.c
index c512a02..1e2f1d0 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3064,6 +3064,21 @@ unsigned long nr_iowait(void)
 	return sum;
 }
 
+unsigned long nr_iowait_cpu(void)
+{
+	int this_cpu = smp_processor_id();
+	struct rq *this_rq = cpu_rq(this_cpu);
+	return atomic_read(&this_rq->nr_iowait);
+}
+
+unsigned long this_cpu_load(void)
+{
+	int this_cpu = smp_processor_id();
+	struct rq *this_rq = cpu_rq(this_cpu);
+	return this_rq->cpu_load[0];
+}
+
+
 /* Variables and functions for calc_load */
 static atomic_long_t calc_load_tasks;
 static unsigned long calc_load_update;
-- 
1.6.0.6


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15  3:42 [PATCH v2] cpuidle: Fix the menu governor to boost IO performance Arjan van de Ven
@ 2009-09-15  3:54 ` Andrew Morton
  2009-09-15  4:15   ` Arjan van de Ven
  2009-09-15  5:03 ` Rik van Riel
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Andrew Morton @ 2009-09-15  3:54 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: linux-kernel, lenb, mingo, yanmin_zhang, jens.axboe, Ivan Kokshaysky

On Tue, 15 Sep 2009 05:42:59 +0200 Arjan van de Ven <arjan@infradead.org> wrote:

> Rather than adding a new governor temporarily, this just puts the fixes
> into the existing menu governor.

Oh, surprised.  I wasn't actually expecting that to happen.

<actually reads his email>

> I don't mind either way, will replace. 

OK.  I'm not particularly strongly opinionated either way.

The timing is awkward.  We could let it sit in Len's tree and
linux-next for a couple of months or we could say what-the-hell and
merge it.

If the latter, your original merge plan sound better ;) But we should
arrange for the new code to default to "on" for test coverage reasons. 
perhaps the original patch did that already?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15  3:54 ` Andrew Morton
@ 2009-09-15  4:15   ` Arjan van de Ven
  2009-09-15  4:18     ` Arjan van de Ven
  0 siblings, 1 reply; 14+ messages in thread
From: Arjan van de Ven @ 2009-09-15  4:15 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, lenb, mingo, yanmin_zhang, jens.axboe, Ivan Kokshaysky

On Mon, 14 Sep 2009 20:54:08 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 15 Sep 2009 05:42:59 +0200 Arjan van de Ven
> <arjan@infradead.org> wrote:
> 
> > Rather than adding a new governor temporarily, this just puts the
> > fixes into the existing menu governor.
> 
> Oh, surprised.  I wasn't actually expecting that to happen.
> 
> <actually reads his email>
> 
> > I don't mind either way, will replace. 
> 
> OK.  I'm not particularly strongly opinionated either way.
> 
> The timing is awkward.  We could let it sit in Len's tree and
> linux-next for a couple of months or we could say what-the-hell and
> merge it.

If the performance impact wasn't this huge I'd say "let it sit".
As it is now, with a nearly 2x performance delta, I'd not be very happy
to expose linux users to this regression-like performance drop for
another 3 months. 

(especially given all the scheduler performance attention there is right
now; picking the wrong C state has clearly very high impact in similar
areas)

> 
> If the latter, your original merge plan sound better ;) But we should
> arrange for the new code to default to "on" for test coverage
> reasons. perhaps the original patch did that already?

the original patch defaulted to the new code yes.

redoing the split is not hard again.. just need to do a "cp" and put
the makefile glue back. I do like how the current patch shows exactly
which few things in the algorithm are changed..
(although it says 230 lines added, a lot of that are comments, the rest
isn't very big at all)



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15  4:15   ` Arjan van de Ven
@ 2009-09-15  4:18     ` Arjan van de Ven
  0 siblings, 0 replies; 14+ messages in thread
From: Arjan van de Ven @ 2009-09-15  4:18 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Andrew Morton, linux-kernel, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

On Tue, 15 Sep 2009 06:15:12 +0200
Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > The timing is awkward.  We could let it sit in Len's tree and
> > linux-next for a couple of months or we could say what-the-hell and
> > merge it.
> 
> If the performance impact wasn't this huge I'd say "let it sit".
> As it is now, with a nearly 2x performance delta, I'd not be very
> happy to expose linux users to this regression-like performance drop
> for another 3 months. 
>

btw I do not like to use the "regression" word lightly, but kernels
prior to 2.6.22 or so did not have this performance drop (at least not
nearly as much), and current ones do. 

 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15  3:42 [PATCH v2] cpuidle: Fix the menu governor to boost IO performance Arjan van de Ven
  2009-09-15  3:54 ` Andrew Morton
@ 2009-09-15  5:03 ` Rik van Riel
  2009-09-15 23:23 ` Andrew Morton
  2009-11-04  9:39 ` Corrado Zoccolo
  3 siblings, 0 replies; 14+ messages in thread
From: Rik van Riel @ 2009-09-15  5:03 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

Arjan van de Ven wrote:

> 		no cstates	current linux	new algorithm
> 1 disk		107 Mb/s	85 Mb/s		105 Mb/s
> 2 disks		215 Mb/s	123 Mb/s	209 Mb/s
> 12 disks	590 Mb/s	320 Mb/s	585 Mb/s

That is huge!  Should this go into -stable too?

> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Yanmin Zhang <yanmin_zhang@linux.intel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15  3:42 [PATCH v2] cpuidle: Fix the menu governor to boost IO performance Arjan van de Ven
  2009-09-15  3:54 ` Andrew Morton
  2009-09-15  5:03 ` Rik van Riel
@ 2009-09-15 23:23 ` Andrew Morton
  2009-09-16  4:35   ` Arjan van de Ven
  2009-09-17 13:39   ` Arjan van de Ven
  2009-11-04  9:39 ` Corrado Zoccolo
  3 siblings, 2 replies; 14+ messages in thread
From: Andrew Morton @ 2009-09-15 23:23 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: linux-kernel, arjan, lenb, mingo, yanmin_zhang, jens.axboe, ink,
	Corrado Zoccolo

On Tue, 15 Sep 2009 05:42:59 +0200
Arjan van de Ven <arjan@infradead.org> wrote:

> Fix the menu idle governor which balances power savings, energy efficiency
> and performance impact.

This patch clashes a bit with
cpuidle-menu-governor-reduce-latency-on-exit.patch (which was sent to
the ACPI maintainers a month ago and ignored).

<im-fed-up-with-this-crap-ill-just-merge-it-and-if-it-breaks-dont-blame-me>


I restaged cpuidle-menu-governor-reduce-latency-on-exit.patch so it
goes after cpuidle-fix-the-menu-governor-to-boost-io-performance.patch.
perhaps you could take a look at Corrado's change?

 

From: Corrado Zoccolo <czoccolo@gmail.com>

Move the state residency accounting and statistics computation off the hot
exit path.

On exit, the need to recompute statistics is recorded, and new statistics
will be computed when menu_select is called again.

The expected effect is to reduce processor wakeup latency from sleep
(C-states).  We are speaking of few hundreds of cycles reduction out of a
several microseconds latency (determined by the hardware transition), so
it is difficult to measure.

Signed-off-by: Corrado Zoccolo <czoccolo@gmail.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: Len Brown <len.brown@intel.com>
Cc: Adam Belay <abelay@novell.com
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/cpuidle/governors/menu.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff -puN drivers/cpuidle/governors/menu.c~cpuidle-menu-governor-reduce-latency-on-exit drivers/cpuidle/governors/menu.c
--- a/drivers/cpuidle/governors/menu.c~cpuidle-menu-governor-reduce-latency-on-exit
+++ a/drivers/cpuidle/governors/menu.c
@@ -96,6 +96,7 @@
 
 struct menu_device {
 	int		last_state_idx;
+	int             needs_update;
 
 	unsigned int	expected_us;
 	u64		predicted_us;
@@ -166,6 +167,8 @@ static inline int performance_multiplier
 
 static DEFINE_PER_CPU(struct menu_device, menu_devices);
 
+static void menu_update(struct cpuidle_device *dev);
+
 /**
  * menu_select - selects the next idle state to enter
  * @dev: the CPU
@@ -180,6 +183,11 @@ static int menu_select(struct cpuidle_de
 	data->last_state_idx = 0;
 	data->exit_us = 0;
 
+	if (data->needs_update) {
+		menu_update(dev);
+		data->needs_update = 0;
+	}
+
 	/* Special case when user has set very strict latency requirement */
 	if (unlikely(latency_req == 0))
 		return 0;
@@ -231,7 +239,7 @@ static int menu_select(struct cpuidle_de
 }
 
 /**
- * menu_reflect - attempts to guess what happened after entry
+ * menu_reflect - records that data structures need update
  * @dev: the CPU
  *
  * NOTE: it's important to be fast here because this operation will add to
@@ -240,6 +248,16 @@ static int menu_select(struct cpuidle_de
 static void menu_reflect(struct cpuidle_device *dev)
 {
 	struct menu_device *data = &__get_cpu_var(menu_devices);
+	data->needs_update = 1;
+}
+
+/**
+ * menu_update - attempts to guess what happened after entry
+ * @dev: the CPU
+ */
+static void menu_update(struct cpuidle_device *dev)
+{
+	struct menu_device *data = &__get_cpu_var(menu_devices);
 	int last_idx = data->last_state_idx;
 	unsigned int last_idle_us = cpuidle_get_last_residency(dev);
 	struct cpuidle_state *target = &dev->states[last_idx];
_


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15 23:23 ` Andrew Morton
@ 2009-09-16  4:35   ` Arjan van de Ven
  2009-09-17 13:39   ` Arjan van de Ven
  1 sibling, 0 replies; 14+ messages in thread
From: Arjan van de Ven @ 2009-09-16  4:35 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, lenb, mingo, yanmin_zhang, jens.axboe, ink,
	Corrado Zoccolo

On Tue, 15 Sep 2009 16:23:06 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 15 Sep 2009 05:42:59 +0200
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > Fix the menu idle governor which balances power savings, energy
> > efficiency and performance impact.
> 
> This patch clashes a bit with
> cpuidle-menu-governor-reduce-latency-on-exit.patch (which was sent to
> the ACPI maintainers a month ago and ignored).
> 
> <im-fed-up-with-this-crap-ill-just-merge-it-and-if-it-breaks-dont-blame-me>
> 
> 
> I restaged cpuidle-menu-governor-reduce-latency-on-exit.patch so it
> goes after
> cpuidle-fix-the-menu-governor-to-boost-io-performance.patch. perhaps
> you could take a look at Corrado's change?

ok this patch is an interesting approach, I'll need to check if it's
valid to do this in the cpuidle framework (need to check if "what was
my last residency" stays valid until all the way deep into the next
idle, I kinda fear it won't).

As for the amount of gain.. yeah we're talking about maybe 100 cycles
on a .. say 200 usec latency. But it's a neat optimization if it can
work.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15 23:23 ` Andrew Morton
  2009-09-16  4:35   ` Arjan van de Ven
@ 2009-09-17 13:39   ` Arjan van de Ven
  1 sibling, 0 replies; 14+ messages in thread
From: Arjan van de Ven @ 2009-09-17 13:39 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-kernel, lenb, mingo, yanmin_zhang, jens.axboe, ink,
	Corrado Zoccolo

On Tue, 15 Sep 2009 16:23:06 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 15 Sep 2009 05:42:59 +0200
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > Fix the menu idle governor which balances power savings, energy
> > efficiency and performance impact.
> 
> This patch clashes a bit with
> cpuidle-menu-governor-reduce-latency-on-exit.patch (which was sent to
> the ACPI maintainers a month ago and ignored).
> 
> <im-fed-up-with-this-crap-ill-just-merge-it-and-if-it-breaks-dont-blame-me>
> 
> 
> I restaged cpuidle-menu-governor-reduce-latency-on-exit.patch so it
> goes after
> cpuidle-fix-the-menu-governor-to-boost-io-performance.patch. perhaps
> you could take a look at Corrado's change?
> 
>  
> 
> From: Corrado Zoccolo <czoccolo@gmail.com>
> 
> Move the state residency accounting and statistics computation off
> the hot exit path.
> 
> On exit, the need to recompute statistics is recorded, and new
> statistics will be computed when menu_select is called again.
> 
> The expected effect is to reduce processor wakeup latency from sleep
> (C-states).  We are speaking of few hundreds of cycles reduction out
> of a several microseconds latency (determined by the hardware
> transition), so it is difficult to measure.
> 
> Signed-off-by: Corrado Zoccolo <czoccolo@gmail.com>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Adam Belay <abelay@novell.com
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

I checked the last residency thing... and it's fine

Acked-by: Arjan van de Ven <arjan@linux.intel.com>



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-09-15  3:42 [PATCH v2] cpuidle: Fix the menu governor to boost IO performance Arjan van de Ven
                   ` (2 preceding siblings ...)
  2009-09-15 23:23 ` Andrew Morton
@ 2009-11-04  9:39 ` Corrado Zoccolo
  2009-11-08 20:40   ` Arjan van de Ven
  3 siblings, 1 reply; 14+ messages in thread
From: Corrado Zoccolo @ 2009-11-04  9:39 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

Hi Arjan,
On Tue, Sep 15, 2009 at 4:42 AM, Arjan van de Ven <arjan@infradead.org> wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: cpuidle: Fix the menu governor to boost IO performance
>
> Fix the menu idle governor which balances power savings, energy efficiency
> and performance impact.

I've tested this patch on an Atom based netbook with SSD, and I see
10% improvement in latencies for reading a single 4k block from disk.

During this test, while looking at powertop, I found that my CPU was
sitting in polling mode for milliseconds (percentage was however
negligible).
I never recalled seeing a non-zero time spent polling, so I looked at
the patch and found:
> +       /*
> +        * We want to default to C1 (hlt), not to busy polling
> +        * unless the timer is happening really really soon.
> +        */
> +       if (data->expected_us > 5)
> +               data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
Commenting the if, (the previous behaviour), I no longer see the
polling, while I still get the performance improvement.

I wonder if that '5' is a bit too much. According to my BIOS ACPI
table, the Atom latency for C1 is ~ 1us, so there is very little
payback in polling on such processors. Should the check use the ACPI
declared C1 latency to decide whether we should poll or go to C1?

An other consideration is that sometimes, even if we expect to idle
for a short time, we end up idling for more (otherwise I would never
have seen ms polling, when expecting at most 5us). Should we set up a
timer, that would fire when switching to an higher C state would
conserve more energy?

Thanks,
Corrado

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-11-04  9:39 ` Corrado Zoccolo
@ 2009-11-08 20:40   ` Arjan van de Ven
  2009-11-08 21:59     ` Corrado Zoccolo
  0 siblings, 1 reply; 14+ messages in thread
From: Arjan van de Ven @ 2009-11-08 20:40 UTC (permalink / raw)
  To: Corrado Zoccolo
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

On Wed, 4 Nov 2009 10:39:13 +0100
Corrado Zoccolo <czoccolo@gmail.com> wrote:

> Hi Arjan,
> On Tue, Sep 15, 2009 at 4:42 AM, Arjan van de Ven
> <arjan@infradead.org> wrote:
> > From: Arjan van de Ven <arjan@linux.intel.com>
> > Subject: cpuidle: Fix the menu governor to boost IO performance
> >
> > Fix the menu idle governor which balances power savings, energy
> > efficiency and performance impact.
> 
> I've tested this patch on an Atom based netbook with SSD, and I see
> 10% improvement in latencies for reading a single 4k block from disk.

great!

> 
> During this test, while looking at powertop, I found that my CPU was
> sitting in polling mode for milliseconds (percentage was however
> negligible).
> I never recalled seeing a non-zero time spent polling, so I looked at
> the patch and found:
> > +       /*
> > +        * We want to default to C1 (hlt), not to busy polling
> > +        * unless the timer is happening really really soon.
> > +        */
> > +       if (data->expected_us > 5)
> > +               data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
> Commenting the if, (the previous behaviour), I no longer see the
> polling, while I still get the performance improvement.
> 
> I wonder if that '5' is a bit too much. According to my BIOS ACPI
> table, the Atom latency for C1 is ~ 1us, so there is very little
> payback in polling on such processors. Should the check use the ACPI
> declared C1 latency to decide whether we should poll or go to C1?

the exit latency is +/- 1 us, the entry latency is similar, and then
you're pretty close to 5 already (esp if you keep in mind that to break
even on energy you also need to be in the C state for a little bit)...

> 
> An other consideration is that sometimes, even if we expect to idle
> for a short time, we end up idling for more (otherwise I would never
> have seen ms polling, when expecting at most 5us). Should we set up a
> timer, that would fire when switching to an higher C state would
> conserve more energy?

this check is supposed to catch the known timer cases; those
are rather accurate in prediction


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-11-08 20:40   ` Arjan van de Ven
@ 2009-11-08 21:59     ` Corrado Zoccolo
  2009-11-08 22:30       ` Arjan van de Ven
  2009-11-08 22:39       ` Arjan van de Ven
  0 siblings, 2 replies; 14+ messages in thread
From: Corrado Zoccolo @ 2009-11-08 21:59 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

On Sun, Nov 8, 2009 at 9:40 PM, Arjan van de Ven <arjan@infradead.org> wrote:
> On Wed, 4 Nov 2009 10:39:13 +0100
> Corrado Zoccolo <czoccolo@gmail.com> wrote:
>
>> Hi Arjan,
>> On Tue, Sep 15, 2009 at 4:42 AM, Arjan van de Ven
>> <arjan@infradead.org> wrote:
>> > From: Arjan van de Ven <arjan@linux.intel.com>
>> > Subject: cpuidle: Fix the menu governor to boost IO performance
>> >
>> > Fix the menu idle governor which balances power savings, energy
>> > efficiency and performance impact.
>>
>> I've tested this patch on an Atom based netbook with SSD, and I see
>> 10% improvement in latencies for reading a single 4k block from disk.
>
> great!
>
>>
>> During this test, while looking at powertop, I found that my CPU was
>> sitting in polling mode for milliseconds (percentage was however
>> negligible).
>> I never recalled seeing a non-zero time spent polling, so I looked at
>> the patch and found:
>> > +       /*
>> > +        * We want to default to C1 (hlt), not to busy polling
>> > +        * unless the timer is happening really really soon.
>> > +        */
>> > +       if (data->expected_us > 5)
>> > +               data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>> Commenting the if, (the previous behaviour), I no longer see the
>> polling, while I still get the performance improvement.
>>
>> I wonder if that '5' is a bit too much. According to my BIOS ACPI
>> table, the Atom latency for C1 is ~ 1us, so there is very little
>> payback in polling on such processors. Should the check use the ACPI
>> declared C1 latency to decide whether we should poll or go to C1?
>
> the exit latency is +/- 1 us, the entry latency is similar, and then
> you're pretty close to 5 already (esp if you keep in mind that to break
> even on energy you also need to be in the C state for a little bit)...

There are also performance considerations for using C1 (HLT).
Quoting from http://www.intel.com/Assets/PDF/manual/248966.pdf (8-19):
On processors supporting HT Technology, operating systems should use the HLT
instruction if one logical processor is active and the other is not.
HLT will allow an idle
logical processor to transition to a halted state; this allows the
active logical
processor to use all the hardware resources in the physical package.
An operating
system that does not use this technique must still execute
instructions on the idle
logical processor that repeatedly check for work. This “idle loop”
consumes execution
resources that could otherwise be used to make progress on the other
active logical
processor.

>
>>
>> An other consideration is that sometimes, even if we expect to idle
>> for a short time, we end up idling for more (otherwise I would never
>> have seen ms polling, when expecting at most 5us). Should we set up a
>> timer, that would fire when switching to an higher C state would
>> conserve more energy?
>
> this check is supposed to catch the known timer cases; those
> are rather accurate in prediction

Unfortunately, I have seen polling residency times > 1ms, so it must
not be so accurate.
Could be that the timer already expired, when we started polling, or
the wake-up went to an other CPU?
Having a timer for the specific CPU that is going idle would help in
such cases, as well as other cases like the governor chosen to go to
C3, but due to BM restrictions, the driver could only achieve C2.

Corrado

>
>
> --
> Arjan van de Ven        Intel Open Source Technology Centre
> For development, discussion and tips for power savings,
> visit http://www.lesswatts.org
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-11-08 21:59     ` Corrado Zoccolo
@ 2009-11-08 22:30       ` Arjan van de Ven
  2009-11-09 13:29         ` Corrado Zoccolo
  2009-11-08 22:39       ` Arjan van de Ven
  1 sibling, 1 reply; 14+ messages in thread
From: Arjan van de Ven @ 2009-11-08 22:30 UTC (permalink / raw)
  To: Corrado Zoccolo
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

On Sun, 8 Nov 2009 22:59:43 +0100
Corrado Zoccolo <czoccolo@gmail.com> wrote:
> >
> > the exit latency is +/- 1 us, the entry latency is similar, and then
> > you're pretty close to 5 already (esp if you keep in mind that to
> > break even on energy you also need to be in the C state for a
> > little bit)...
> 
> There are also performance considerations for using C1 (HLT).
> Quoting from http://www.intel.com/Assets/PDF/manual/248966.pdf (8-19):
> On processors supporting HT Technology, operating systems should use
> the HLT instruction if one logical processor is active and the other
> is not. HLT will allow an idle
> logical processor to transition to a halted state; this allows the
> active logical
> processor to use all the hardware resources in the physical package.

I think we all agree that long term polling is bad ;-)
(even though we use rep nop in the polling loop which is also a HT
yield).

There's just the very short sleeps (where short is "single digit usecs")
where the rules are slightly different.

> > this check is supposed to catch the known timer cases; those
> > are rather accurate in prediction
> 
> Unfortunately, I have seen polling residency times > 1ms, so it must
> not be so accurate.

well the question is... is this a measurement error or an error in
when polling is chosen.
We obviously need to fix it whatever it is, but... first need to chase
down really which it is.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-11-08 21:59     ` Corrado Zoccolo
  2009-11-08 22:30       ` Arjan van de Ven
@ 2009-11-08 22:39       ` Arjan van de Ven
  1 sibling, 0 replies; 14+ messages in thread
From: Arjan van de Ven @ 2009-11-08 22:39 UTC (permalink / raw)
  To: Corrado Zoccolo
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

On Sun, 8 Nov 2009 22:59:43 +0100
Corrado Zoccolo <czoccolo@gmail.com> wrote:

> Unfortunately, I have seen polling residency times > 1ms, so it must
> not be so accurate.


hmm I wonder if the timer hardware in use has a much larger
granularity than usecs... 
because that rounding up could cost us dearly.

wonder if we should do some form of "check for timer expiry" as part of
the polling loop.. that'd cut latency way down as well ;)

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] cpuidle: Fix the menu governor to boost IO performance
  2009-11-08 22:30       ` Arjan van de Ven
@ 2009-11-09 13:29         ` Corrado Zoccolo
  0 siblings, 0 replies; 14+ messages in thread
From: Corrado Zoccolo @ 2009-11-09 13:29 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: linux-kernel, Andrew Morton, lenb, mingo, yanmin_zhang,
	jens.axboe, Ivan Kokshaysky

On Sun, Nov 8, 2009 at 11:30 PM, Arjan van de Ven <arjan@infradead.org> wrote:
> On Sun, 8 Nov 2009 22:59:43 +0100
> I think we all agree that long term polling is bad ;-)
> (even though we use rep nop in the polling loop which is also a HT
> yield).
>
> There's just the very short sleeps (where short is "single digit usecs")
> where the rules are slightly different.
>
>> > this check is supposed to catch the known timer cases; those
>> > are rather accurate in prediction
>>
>> Unfortunately, I have seen polling residency times > 1ms, so it must
>> not be so accurate.
>
> well the question is... is this a measurement error or an error in
> when polling is chosen.
> We obviously need to fix it whatever it is, but... first need to chase
> down really which it is.

Any suggestion on how to check?
Clocksource is hpet, and I have CONFIG_SCHED_HRTICK=y, so the
granularity should not be a problem.

Corrado

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2009-11-09 13:29 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-15  3:42 [PATCH v2] cpuidle: Fix the menu governor to boost IO performance Arjan van de Ven
2009-09-15  3:54 ` Andrew Morton
2009-09-15  4:15   ` Arjan van de Ven
2009-09-15  4:18     ` Arjan van de Ven
2009-09-15  5:03 ` Rik van Riel
2009-09-15 23:23 ` Andrew Morton
2009-09-16  4:35   ` Arjan van de Ven
2009-09-17 13:39   ` Arjan van de Ven
2009-11-04  9:39 ` Corrado Zoccolo
2009-11-08 20:40   ` Arjan van de Ven
2009-11-08 21:59     ` Corrado Zoccolo
2009-11-08 22:30       ` Arjan van de Ven
2009-11-09 13:29         ` Corrado Zoccolo
2009-11-08 22:39       ` Arjan van de Ven

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.