All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-13 21:52 ` Julius Werner
  0 siblings, 0 replies; 26+ messages in thread
From: Julius Werner @ 2012-11-13 21:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Len Brown, Rafael J. Wysocki, Kevin Hilman, Andrew Morton,
	Srivatsa S. Bhat, linux-acpi, linux-pm, linuxppc-dev,
	Deepthi Dharwar, Trinabh Gupta, Daniel Lezcano, Sameer Nanda,
	Julius Werner

Many cpuidle drivers measure their time spent in an idle state by
reading the wallclock time before and after idling and calculating the
difference. This leads to erroneous results when the wallclock time gets
updated by another processor in the meantime, adding that clock
adjustment to the idle state's time counter.

If the clock adjustment was negative, the result is even worse due to an
erroneous cast from int to unsigned long long of the last_residency
variable. The negative 32 bit integer will zero-extend and result in a
forward time jump of roughly four billion milliseconds or 1.3 hours on
the idle state residency counter.

This patch changes all affected cpuidle drivers to use the monotonic
clock for their measurements instead. It also removes the erroneous
cast, making sure that negative residency values are applied correctly
even though they should not appear anymore.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
 drivers/acpi/processor_idle.c                   |   12 ++++++------
 drivers/cpuidle/cpuidle.c                       |    3 +--
 drivers/idle/intel_idle.c                       |   13 ++++---------
 4 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
index 45d00e5..4d806b4 100644
--- a/arch/powerpc/platforms/pseries/processor_idle.c
+++ b/arch/powerpc/platforms/pseries/processor_idle.c
@@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
 static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
 {
 
-	*kt_before = ktime_get_real();
+	*kt_before = ktime_get();
 	*in_purr = mfspr(SPRN_PURR);
 	/*
 	 * Indicate to the HV that we are idle. Now would be
@@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
 	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
 	get_lppaca()->idle = 0;
 
-	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
+	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
 }
 
 static int snooze_loop(struct cpuidle_device *dev,
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index e8086c7..8c98d73 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 
 	lapic_timer_state_broadcast(pr, cx, 1);
-	kt1 = ktime_get_real();
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
+	kt2 = ktime_get();
 	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
 
 	/* Update device last_residency*/
@@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	kt1 = ktime_get_real();
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
+	kt2 = ktime_get();
 	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
 	idle_time = idle_time_ns;
 	do_div(idle_time, NSEC_PER_USEC);
@@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	 */
 	lapic_timer_state_broadcast(pr, cx, 1);
 
-	kt1 = ktime_get_real();
+	kt1 = ktime_get();
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
@@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		c3_cpu_count--;
 		raw_spin_unlock(&c3_lock);
 	}
-	kt2 = ktime_get_real();
+	kt2 = ktime_get();
 	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
 	idle_time = idle_time_ns;
 	do_div(idle_time, NSEC_PER_USEC);
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7f15b85..1536edd 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		/* This can be moved to within driver enter routine
 		 * but that results in multiple copies of same code.
 		 */
-		dev->states_usage[entered_state].time +=
-				(unsigned long long)dev->last_residency;
+		dev->states_usage[entered_state].time += dev->last_residency;
 		dev->states_usage[entered_state].usage++;
 	} else {
 		dev->last_residency = 0;
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index b0f6b4c..6329a97 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -56,7 +56,7 @@
 #include <linux/kernel.h>
 #include <linux/cpuidle.h>
 #include <linux/clockchips.h>
-#include <linux/hrtimer.h>	/* ktime_get_real() */
+#include <linux/hrtimer.h>	/* ktime_get() */
 #include <trace/events/power.h>
 #include <linux/sched.h>
 #include <linux/notifier.h>
@@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
 	unsigned int cstate;
-	ktime_t kt_before, kt_after;
-	s64 usec_delta;
+	ktime_t kt_before;
 	int cpu = smp_processor_id();
 
 	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
@@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
-	kt_before = ktime_get_real();
+	kt_before = ktime_get();
 
 	stop_critical_timings();
 	if (!need_resched()) {
@@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
 
 	start_critical_timings();
 
-	kt_after = ktime_get_real();
-	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
+	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
 
 	local_irq_enable();
 
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
 
-	/* Update cpuidle counters */
-	dev->last_residency = (int)usec_delta;
-
 	return index;
 }
 
-- 
1.7.8.6

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

* [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-13 21:52 ` Julius Werner
  0 siblings, 0 replies; 26+ messages in thread
From: Julius Werner @ 2012-11-13 21:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, linux-pm,
	Daniel Lezcano, Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat,
	Julius Werner, Andrew Morton, linuxppc-dev, Sameer Nanda,
	Len Brown

Many cpuidle drivers measure their time spent in an idle state by
reading the wallclock time before and after idling and calculating the
difference. This leads to erroneous results when the wallclock time gets
updated by another processor in the meantime, adding that clock
adjustment to the idle state's time counter.

If the clock adjustment was negative, the result is even worse due to an
erroneous cast from int to unsigned long long of the last_residency
variable. The negative 32 bit integer will zero-extend and result in a
forward time jump of roughly four billion milliseconds or 1.3 hours on
the idle state residency counter.

This patch changes all affected cpuidle drivers to use the monotonic
clock for their measurements instead. It also removes the erroneous
cast, making sure that negative residency values are applied correctly
even though they should not appear anymore.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
 drivers/acpi/processor_idle.c                   |   12 ++++++------
 drivers/cpuidle/cpuidle.c                       |    3 +--
 drivers/idle/intel_idle.c                       |   13 ++++---------
 4 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
index 45d00e5..4d806b4 100644
--- a/arch/powerpc/platforms/pseries/processor_idle.c
+++ b/arch/powerpc/platforms/pseries/processor_idle.c
@@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
 static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
 {
 
-	*kt_before = ktime_get_real();
+	*kt_before = ktime_get();
 	*in_purr = mfspr(SPRN_PURR);
 	/*
 	 * Indicate to the HV that we are idle. Now would be
@@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
 	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
 	get_lppaca()->idle = 0;
 
-	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
+	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
 }
 
 static int snooze_loop(struct cpuidle_device *dev,
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index e8086c7..8c98d73 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 
 	lapic_timer_state_broadcast(pr, cx, 1);
-	kt1 = ktime_get_real();
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
+	kt2 = ktime_get();
 	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
 
 	/* Update device last_residency*/
@@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	kt1 = ktime_get_real();
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
+	kt2 = ktime_get();
 	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
 	idle_time = idle_time_ns;
 	do_div(idle_time, NSEC_PER_USEC);
@@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	 */
 	lapic_timer_state_broadcast(pr, cx, 1);
 
-	kt1 = ktime_get_real();
+	kt1 = ktime_get();
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
@@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		c3_cpu_count--;
 		raw_spin_unlock(&c3_lock);
 	}
-	kt2 = ktime_get_real();
+	kt2 = ktime_get();
 	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
 	idle_time = idle_time_ns;
 	do_div(idle_time, NSEC_PER_USEC);
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7f15b85..1536edd 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		/* This can be moved to within driver enter routine
 		 * but that results in multiple copies of same code.
 		 */
-		dev->states_usage[entered_state].time +=
-				(unsigned long long)dev->last_residency;
+		dev->states_usage[entered_state].time += dev->last_residency;
 		dev->states_usage[entered_state].usage++;
 	} else {
 		dev->last_residency = 0;
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index b0f6b4c..6329a97 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -56,7 +56,7 @@
 #include <linux/kernel.h>
 #include <linux/cpuidle.h>
 #include <linux/clockchips.h>
-#include <linux/hrtimer.h>	/* ktime_get_real() */
+#include <linux/hrtimer.h>	/* ktime_get() */
 #include <trace/events/power.h>
 #include <linux/sched.h>
 #include <linux/notifier.h>
@@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
 	unsigned int cstate;
-	ktime_t kt_before, kt_after;
-	s64 usec_delta;
+	ktime_t kt_before;
 	int cpu = smp_processor_id();
 
 	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
@@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
-	kt_before = ktime_get_real();
+	kt_before = ktime_get();
 
 	stop_critical_timings();
 	if (!need_resched()) {
@@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
 
 	start_critical_timings();
 
-	kt_after = ktime_get_real();
-	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
+	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
 
 	local_irq_enable();
 
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
 
-	/* Update cpuidle counters */
-	dev->last_residency = (int)usec_delta;
-
 	return index;
 }
 
-- 
1.7.8.6

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-13 21:52 ` Julius Werner
@ 2012-11-14  9:06   ` Deepthi Dharwar
  -1 siblings, 0 replies; 26+ messages in thread
From: Deepthi Dharwar @ 2012-11-14  9:06 UTC (permalink / raw)
  To: Julius Werner
  Cc: linux-kernel, Kevin Hilman, Trinabh Gupta, linux-pm,
	Daniel Lezcano, Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat,
	Andrew Morton, linuxppc-dev, Sameer Nanda, Len Brown

On 11/14/2012 03:22 AM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to use the monotonic
> clock for their measurements instead. It also removes the erroneous
> cast, making sure that negative residency values are applied correctly
> even though they should not appear anymore.

Currently tegra/cpuidle uses ktime_get(). Good to have it for all
the other arch idle residency time logging too.
Tested patch on pseries.

Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>

Cheers,
Deepthi

> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>  drivers/cpuidle/cpuidle.c                       |    3 +--
>  drivers/idle/intel_idle.c                       |   13 ++++---------
>  4 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
> 
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
> 
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
> 
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..8c98d73 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
> 
> 
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
> 
>  	/* Update device last_residency*/
> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
> 
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);
> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
> 
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..1536edd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
>  		 */
> -		dev->states_usage[entered_state].time +=
> -				(unsigned long long)dev->last_residency;
> +		dev->states_usage[entered_state].time += dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
>  	} else {
>  		dev->last_residency = 0;
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b0f6b4c..6329a97 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,7 +56,7 @@
>  #include <linux/kernel.h>
>  #include <linux/cpuidle.h>
>  #include <linux/clockchips.h>
> -#include <linux/hrtimer.h>	/* ktime_get_real() */
> +#include <linux/hrtimer.h>	/* ktime_get() */
>  #include <trace/events/power.h>
>  #include <linux/sched.h>
>  #include <linux/notifier.h>
> @@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>  	unsigned int cstate;
> -	ktime_t kt_before, kt_after;
> -	s64 usec_delta;
> +	ktime_t kt_before;
>  	int cpu = smp_processor_id();
> 
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
> @@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
> 
> -	kt_before = ktime_get_real();
> +	kt_before = ktime_get();
> 
>  	stop_critical_timings();
>  	if (!need_resched()) {
> @@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
> 
>  	start_critical_timings();
> 
> -	kt_after = ktime_get_real();
> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
> +	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
> 
>  	local_irq_enable();
> 
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
> 
> -	/* Update cpuidle counters */
> -	dev->last_residency = (int)usec_delta;
> -
>  	return index;
>  }
> 


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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-14  9:06   ` Deepthi Dharwar
  0 siblings, 0 replies; 26+ messages in thread
From: Deepthi Dharwar @ 2012-11-14  9:06 UTC (permalink / raw)
  To: Julius Werner
  Cc: Kevin Hilman, Trinabh Gupta, linux-pm, Daniel Lezcano,
	linux-kernel, Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat,
	Andrew Morton, linuxppc-dev, Sameer Nanda, Len Brown

On 11/14/2012 03:22 AM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to use the monotonic
> clock for their measurements instead. It also removes the erroneous
> cast, making sure that negative residency values are applied correctly
> even though they should not appear anymore.

Currently tegra/cpuidle uses ktime_get(). Good to have it for all
the other arch idle residency time logging too.
Tested patch on pseries.

Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>

Cheers,
Deepthi

> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>  drivers/cpuidle/cpuidle.c                       |    3 +--
>  drivers/idle/intel_idle.c                       |   13 ++++---------
>  4 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
> 
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
> 
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
> 
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..8c98d73 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
> 
> 
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
> 
>  	/* Update device last_residency*/
> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
> 
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);
> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
> 
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..1536edd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
>  		 */
> -		dev->states_usage[entered_state].time +=
> -				(unsigned long long)dev->last_residency;
> +		dev->states_usage[entered_state].time += dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
>  	} else {
>  		dev->last_residency = 0;
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b0f6b4c..6329a97 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,7 +56,7 @@
>  #include <linux/kernel.h>
>  #include <linux/cpuidle.h>
>  #include <linux/clockchips.h>
> -#include <linux/hrtimer.h>	/* ktime_get_real() */
> +#include <linux/hrtimer.h>	/* ktime_get() */
>  #include <trace/events/power.h>
>  #include <linux/sched.h>
>  #include <linux/notifier.h>
> @@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>  	unsigned int cstate;
> -	ktime_t kt_before, kt_after;
> -	s64 usec_delta;
> +	ktime_t kt_before;
>  	int cpu = smp_processor_id();
> 
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
> @@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
> 
> -	kt_before = ktime_get_real();
> +	kt_before = ktime_get();
> 
>  	stop_critical_timings();
>  	if (!need_resched()) {
> @@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
> 
>  	start_critical_timings();
> 
> -	kt_after = ktime_get_real();
> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
> +	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
> 
>  	local_irq_enable();
> 
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
> 
> -	/* Update cpuidle counters */
> -	dev->last_residency = (int)usec_delta;
> -
>  	return index;
>  }
> 

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-14  9:06   ` Deepthi Dharwar
  (?)
@ 2012-11-14 10:57     ` Daniel Lezcano
  -1 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-14 10:57 UTC (permalink / raw)
  To: Deepthi Dharwar
  Cc: Julius Werner, linux-kernel, Kevin Hilman, Trinabh Gupta,
	linux-pm, Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat,
	Andrew Morton, linuxppc-dev, Sameer Nanda, Len Brown

On 11/14/2012 10:06 AM, Deepthi Dharwar wrote:
> On 11/14/2012 03:22 AM, Julius Werner wrote:
>> Many cpuidle drivers measure their time spent in an idle state by
>> reading the wallclock time before and after idling and calculating the
>> difference. This leads to erroneous results when the wallclock time gets
>> updated by another processor in the meantime, adding that clock
>> adjustment to the idle state's time counter.
>>
>> If the clock adjustment was negative, the result is even worse due to an
>> erroneous cast from int to unsigned long long of the last_residency
>> variable. The negative 32 bit integer will zero-extend and result in a
>> forward time jump of roughly four billion milliseconds or 1.3 hours on
>> the idle state residency counter.
>>
>> This patch changes all affected cpuidle drivers to use the monotonic
>> clock for their measurements instead. It also removes the erroneous
>> cast, making sure that negative residency values are applied correctly
>> even though they should not appear anymore.
> 
> Currently tegra/cpuidle uses ktime_get(). Good to have it for all
> the other arch idle residency time logging too.

Actually it is used by all arm cpuidle drivers through the wrapper
"cpuidle_wrap_enter" and the en_core_tk_irqen flag.

> Tested patch on pseries.
> 
> Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> 
> Cheers,
> Deepthi
> 
>>
>> Signed-off-by: Julius Werner <jwerner@chromium.org>
>> ---
>>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>>  drivers/cpuidle/cpuidle.c                       |    3 +--
>>  drivers/idle/intel_idle.c                       |   13 ++++---------
>>  4 files changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
>> index 45d00e5..4d806b4 100644
>> --- a/arch/powerpc/platforms/pseries/processor_idle.c
>> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
>> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>>  {
>>
>> -	*kt_before = ktime_get_real();
>> +	*kt_before = ktime_get();
>>  	*in_purr = mfspr(SPRN_PURR);
>>  	/*
>>  	 * Indicate to the HV that we are idle. Now would be
>> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>>  	get_lppaca()->idle = 0;
>>
>> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
>> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>>  }
>>
>>  static int snooze_loop(struct cpuidle_device *dev,
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index e8086c7..8c98d73 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>>
>>
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	acpi_idle_do_entry(cx);
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>>
>>  	/* Update device last_residency*/
>> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>>  	if (cx->type == ACPI_STATE_C3)
>>  		ACPI_FLUSH_CPU_CACHE();
>>
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	/* Tell the scheduler that we are going deep-idle: */
>>  	sched_clock_idle_sleep_event();
>>  	acpi_idle_do_entry(cx);
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>>  	idle_time = idle_time_ns;
>>  	do_div(idle_time, NSEC_PER_USEC);
>> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  	 */
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>>
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	/*
>>  	 * disable bus master
>>  	 * bm_check implies we need ARB_DIS
>> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  		c3_cpu_count--;
>>  		raw_spin_unlock(&c3_lock);
>>  	}
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>>  	idle_time = idle_time_ns;
>>  	do_div(idle_time, NSEC_PER_USEC);
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 7f15b85..1536edd 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>>  		/* This can be moved to within driver enter routine
>>  		 * but that results in multiple copies of same code.
>>  		 */
>> -		dev->states_usage[entered_state].time +=
>> -				(unsigned long long)dev->last_residency;
>> +		dev->states_usage[entered_state].time += dev->last_residency;
>>  		dev->states_usage[entered_state].usage++;
>>  	} else {
>>  		dev->last_residency = 0;
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index b0f6b4c..6329a97 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -56,7 +56,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/clockchips.h>
>> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>> +#include <linux/hrtimer.h>	/* ktime_get() */
>>  #include <trace/events/power.h>
>>  #include <linux/sched.h>
>>  #include <linux/notifier.h>
>> @@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>>  	unsigned int cstate;
>> -	ktime_t kt_before, kt_after;
>> -	s64 usec_delta;
>> +	ktime_t kt_before;
>>  	int cpu = smp_processor_id();
>>
>>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
>> @@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>>
>> -	kt_before = ktime_get_real();
>> +	kt_before = ktime_get();
>>
>>  	stop_critical_timings();
>>  	if (!need_resched()) {
>> @@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
>>
>>  	start_critical_timings();
>>
>> -	kt_after = ktime_get_real();
>> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
>> +	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
>>
>>  	local_irq_enable();
>>
>>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>>
>> -	/* Update cpuidle counters */
>> -	dev->last_residency = (int)usec_delta;
>> -
>>  	return index;
>>  }
>>
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-14 10:57     ` Daniel Lezcano
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-14 10:57 UTC (permalink / raw)
  To: Deepthi Dharwar
  Cc: Julius Werner, linux-kernel, Kevin Hilman, Trinabh Gupta,
	linux-pm, Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat,
	Andrew Morton, linuxppc-dev, Sameer Nanda, Len Brown

On 11/14/2012 10:06 AM, Deepthi Dharwar wrote:
> On 11/14/2012 03:22 AM, Julius Werner wrote:
>> Many cpuidle drivers measure their time spent in an idle state by
>> reading the wallclock time before and after idling and calculating the
>> difference. This leads to erroneous results when the wallclock time gets
>> updated by another processor in the meantime, adding that clock
>> adjustment to the idle state's time counter.
>>
>> If the clock adjustment was negative, the result is even worse due to an
>> erroneous cast from int to unsigned long long of the last_residency
>> variable. The negative 32 bit integer will zero-extend and result in a
>> forward time jump of roughly four billion milliseconds or 1.3 hours on
>> the idle state residency counter.
>>
>> This patch changes all affected cpuidle drivers to use the monotonic
>> clock for their measurements instead. It also removes the erroneous
>> cast, making sure that negative residency values are applied correctly
>> even though they should not appear anymore.
> 
> Currently tegra/cpuidle uses ktime_get(). Good to have it for all
> the other arch idle residency time logging too.

Actually it is used by all arm cpuidle drivers through the wrapper
"cpuidle_wrap_enter" and the en_core_tk_irqen flag.

> Tested patch on pseries.
> 
> Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> 
> Cheers,
> Deepthi
> 
>>
>> Signed-off-by: Julius Werner <jwerner@chromium.org>
>> ---
>>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>>  drivers/cpuidle/cpuidle.c                       |    3 +--
>>  drivers/idle/intel_idle.c                       |   13 ++++---------
>>  4 files changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
>> index 45d00e5..4d806b4 100644
>> --- a/arch/powerpc/platforms/pseries/processor_idle.c
>> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
>> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>>  {
>>
>> -	*kt_before = ktime_get_real();
>> +	*kt_before = ktime_get();
>>  	*in_purr = mfspr(SPRN_PURR);
>>  	/*
>>  	 * Indicate to the HV that we are idle. Now would be
>> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>>  	get_lppaca()->idle = 0;
>>
>> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
>> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>>  }
>>
>>  static int snooze_loop(struct cpuidle_device *dev,
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index e8086c7..8c98d73 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>>
>>
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	acpi_idle_do_entry(cx);
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>>
>>  	/* Update device last_residency*/
>> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>>  	if (cx->type == ACPI_STATE_C3)
>>  		ACPI_FLUSH_CPU_CACHE();
>>
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	/* Tell the scheduler that we are going deep-idle: */
>>  	sched_clock_idle_sleep_event();
>>  	acpi_idle_do_entry(cx);
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>>  	idle_time = idle_time_ns;
>>  	do_div(idle_time, NSEC_PER_USEC);
>> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  	 */
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>>
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	/*
>>  	 * disable bus master
>>  	 * bm_check implies we need ARB_DIS
>> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  		c3_cpu_count--;
>>  		raw_spin_unlock(&c3_lock);
>>  	}
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>>  	idle_time = idle_time_ns;
>>  	do_div(idle_time, NSEC_PER_USEC);
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 7f15b85..1536edd 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>>  		/* This can be moved to within driver enter routine
>>  		 * but that results in multiple copies of same code.
>>  		 */
>> -		dev->states_usage[entered_state].time +=
>> -				(unsigned long long)dev->last_residency;
>> +		dev->states_usage[entered_state].time += dev->last_residency;
>>  		dev->states_usage[entered_state].usage++;
>>  	} else {
>>  		dev->last_residency = 0;
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index b0f6b4c..6329a97 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -56,7 +56,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/clockchips.h>
>> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>> +#include <linux/hrtimer.h>	/* ktime_get() */
>>  #include <trace/events/power.h>
>>  #include <linux/sched.h>
>>  #include <linux/notifier.h>
>> @@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>>  	unsigned int cstate;
>> -	ktime_t kt_before, kt_after;
>> -	s64 usec_delta;
>> +	ktime_t kt_before;
>>  	int cpu = smp_processor_id();
>>
>>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
>> @@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>>
>> -	kt_before = ktime_get_real();
>> +	kt_before = ktime_get();
>>
>>  	stop_critical_timings();
>>  	if (!need_resched()) {
>> @@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
>>
>>  	start_critical_timings();
>>
>> -	kt_after = ktime_get_real();
>> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
>> +	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
>>
>>  	local_irq_enable();
>>
>>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>>
>> -	/* Update cpuidle counters */
>> -	dev->last_residency = (int)usec_delta;
>> -
>>  	return index;
>>  }
>>
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-14 10:57     ` Daniel Lezcano
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-14 10:57 UTC (permalink / raw)
  To: Deepthi Dharwar
  Cc: Kevin Hilman, Trinabh Gupta, linux-pm, linux-kernel,
	Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat, Julius Werner,
	Andrew Morton, linuxppc-dev, Sameer Nanda, Len Brown

On 11/14/2012 10:06 AM, Deepthi Dharwar wrote:
> On 11/14/2012 03:22 AM, Julius Werner wrote:
>> Many cpuidle drivers measure their time spent in an idle state by
>> reading the wallclock time before and after idling and calculating the
>> difference. This leads to erroneous results when the wallclock time gets
>> updated by another processor in the meantime, adding that clock
>> adjustment to the idle state's time counter.
>>
>> If the clock adjustment was negative, the result is even worse due to an
>> erroneous cast from int to unsigned long long of the last_residency
>> variable. The negative 32 bit integer will zero-extend and result in a
>> forward time jump of roughly four billion milliseconds or 1.3 hours on
>> the idle state residency counter.
>>
>> This patch changes all affected cpuidle drivers to use the monotonic
>> clock for their measurements instead. It also removes the erroneous
>> cast, making sure that negative residency values are applied correctly
>> even though they should not appear anymore.
> 
> Currently tegra/cpuidle uses ktime_get(). Good to have it for all
> the other arch idle residency time logging too.

Actually it is used by all arm cpuidle drivers through the wrapper
"cpuidle_wrap_enter" and the en_core_tk_irqen flag.

> Tested patch on pseries.
> 
> Reviewed-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> 
> Cheers,
> Deepthi
> 
>>
>> Signed-off-by: Julius Werner <jwerner@chromium.org>
>> ---
>>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>>  drivers/cpuidle/cpuidle.c                       |    3 +--
>>  drivers/idle/intel_idle.c                       |   13 ++++---------
>>  4 files changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
>> index 45d00e5..4d806b4 100644
>> --- a/arch/powerpc/platforms/pseries/processor_idle.c
>> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
>> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>>  {
>>
>> -	*kt_before = ktime_get_real();
>> +	*kt_before = ktime_get();
>>  	*in_purr = mfspr(SPRN_PURR);
>>  	/*
>>  	 * Indicate to the HV that we are idle. Now would be
>> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>>  	get_lppaca()->idle = 0;
>>
>> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
>> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>>  }
>>
>>  static int snooze_loop(struct cpuidle_device *dev,
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index e8086c7..8c98d73 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>>
>>
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	acpi_idle_do_entry(cx);
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>>
>>  	/* Update device last_residency*/
>> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>>  	if (cx->type == ACPI_STATE_C3)
>>  		ACPI_FLUSH_CPU_CACHE();
>>
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	/* Tell the scheduler that we are going deep-idle: */
>>  	sched_clock_idle_sleep_event();
>>  	acpi_idle_do_entry(cx);
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>>  	idle_time = idle_time_ns;
>>  	do_div(idle_time, NSEC_PER_USEC);
>> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  	 */
>>  	lapic_timer_state_broadcast(pr, cx, 1);
>>
>> -	kt1 = ktime_get_real();
>> +	kt1 = ktime_get();
>>  	/*
>>  	 * disable bus master
>>  	 * bm_check implies we need ARB_DIS
>> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>>  		c3_cpu_count--;
>>  		raw_spin_unlock(&c3_lock);
>>  	}
>> -	kt2 = ktime_get_real();
>> +	kt2 = ktime_get();
>>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>>  	idle_time = idle_time_ns;
>>  	do_div(idle_time, NSEC_PER_USEC);
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 7f15b85..1536edd 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>>  		/* This can be moved to within driver enter routine
>>  		 * but that results in multiple copies of same code.
>>  		 */
>> -		dev->states_usage[entered_state].time +=
>> -				(unsigned long long)dev->last_residency;
>> +		dev->states_usage[entered_state].time += dev->last_residency;
>>  		dev->states_usage[entered_state].usage++;
>>  	} else {
>>  		dev->last_residency = 0;
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index b0f6b4c..6329a97 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -56,7 +56,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/cpuidle.h>
>>  #include <linux/clockchips.h>
>> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>> +#include <linux/hrtimer.h>	/* ktime_get() */
>>  #include <trace/events/power.h>
>>  #include <linux/sched.h>
>>  #include <linux/notifier.h>
>> @@ -281,8 +281,7 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>>  	unsigned int cstate;
>> -	ktime_t kt_before, kt_after;
>> -	s64 usec_delta;
>> +	ktime_t kt_before;
>>  	int cpu = smp_processor_id();
>>
>>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
>> @@ -297,7 +296,7 @@ static int intel_idle(struct cpuidle_device *dev,
>>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>>
>> -	kt_before = ktime_get_real();
>> +	kt_before = ktime_get();
>>
>>  	stop_critical_timings();
>>  	if (!need_resched()) {
>> @@ -310,17 +309,13 @@ static int intel_idle(struct cpuidle_device *dev,
>>
>>  	start_critical_timings();
>>
>> -	kt_after = ktime_get_real();
>> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
>> +	dev->last_residency = ktime_to_us(ktime_sub(ktime_get(), kt_before));
>>
>>  	local_irq_enable();
>>
>>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>>
>> -	/* Update cpuidle counters */
>> -	dev->last_residency = (int)usec_delta;
>> -
>>  	return index;
>>  }
>>
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-13 21:52 ` Julius Werner
@ 2012-11-14 11:05   ` Daniel Lezcano
  -1 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-14 11:05 UTC (permalink / raw)
  To: Julius Werner
  Cc: linux-kernel, Len Brown, Rafael J. Wysocki, Kevin Hilman,
	Andrew Morton, Srivatsa S. Bhat, linux-acpi, linux-pm,
	linuxppc-dev, Deepthi Dharwar, Trinabh Gupta, Sameer Nanda,
	Lists Linaro-dev

On 11/13/2012 10:52 PM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to use the monotonic
> clock for their measurements instead. It also removes the erroneous
> cast, making sure that negative residency values are applied correctly
> even though they should not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>  drivers/cpuidle/cpuidle.c                       |    3 +--
>  drivers/idle/intel_idle.c                       |   13 ++++---------
>  4 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
>  
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
>  
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
>  
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..8c98d73 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  
>  
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>  
>  	/* Update device last_residency*/
> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);
> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);


Maybe you can remove all these computations and set the flag
en_core_tk_irqen for the driver ? That will be handled by the cpuidle
framework, no ?

Same comment for the intel_idle driver.

Thanks
  -- Daniel

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-14 11:05   ` Daniel Lezcano
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-14 11:05 UTC (permalink / raw)
  To: Julius Werner
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm, linux-kernel, Rafael J. Wysocki, linux-acpi,
	Srivatsa S. Bhat, Andrew Morton, linuxppc-dev, Sameer Nanda,
	Len Brown

On 11/13/2012 10:52 PM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to use the monotonic
> clock for their measurements instead. It also removes the erroneous
> cast, making sure that negative residency values are applied correctly
> even though they should not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 ++--
>  drivers/acpi/processor_idle.c                   |   12 ++++++------
>  drivers/cpuidle/cpuidle.c                       |    3 +--
>  drivers/idle/intel_idle.c                       |   13 ++++---------
>  4 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
>  
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
>  
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
>  
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..8c98d73 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -751,9 +751,9 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  
>  
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>  
>  	/* Update device last_residency*/
> @@ -843,11 +843,11 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);
> @@ -934,7 +934,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  
> -	kt1 = ktime_get_real();
> +	kt1 = ktime_get();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,7 +965,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> +	kt2 = ktime_get();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
>  	do_div(idle_time, NSEC_PER_USEC);


Maybe you can remove all these computations and set the flag
en_core_tk_irqen for the driver ? That will be handled by the cpuidle
framework, no ?

Same comment for the intel_idle driver.

Thanks
  -- Daniel

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-14 11:05   ` Daniel Lezcano
@ 2012-11-14 17:15     ` Julius Werner
  -1 siblings, 0 replies; 26+ messages in thread
From: Julius Werner @ 2012-11-14 17:15 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux-kernel, Len Brown, Rafael J. Wysocki, Kevin Hilman,
	Andrew Morton, Srivatsa S. Bhat, linux-acpi, linux-pm,
	linuxppc-dev, Deepthi Dharwar, Trinabh Gupta, Sameer Nanda,
	Lists Linaro-dev

> Maybe you can remove all these computations and set the flag
> en_core_tk_irqen for the driver ? That will be handled by the cpuidle
> framework, no ?
>
> Same comment for the intel_idle driver.

Yeah, I thought about that, too. I was a little too afraid of touching
the sched_clock_idle_wakeup_event() parameter that is tied to the
measurement, but it seems to have been vestigial for some time now and
other drivers also just set it 0. I will whip up another version of
the patch (won't change the PPC further though, if this version works
I would just leave it at that... thanks for testing, Deepthi).

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-14 17:15     ` Julius Werner
  0 siblings, 0 replies; 26+ messages in thread
From: Julius Werner @ 2012-11-14 17:15 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm, linux-kernel, Rafael J. Wysocki, linux-acpi,
	Srivatsa S. Bhat, Andrew Morton, linuxppc-dev, Sameer Nanda,
	Len Brown

> Maybe you can remove all these computations and set the flag
> en_core_tk_irqen for the driver ? That will be handled by the cpuidle
> framework, no ?
>
> Same comment for the intel_idle driver.

Yeah, I thought about that, too. I was a little too afraid of touching
the sched_clock_idle_wakeup_event() parameter that is tied to the
measurement, but it seems to have been vestigial for some time now and
other drivers also just set it 0. I will whip up another version of
the patch (won't change the PPC further though, if this version works
I would just leave it at that... thanks for testing, Deepthi).

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

* [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-14 17:15     ` Julius Werner
@ 2012-11-15  1:56       ` Julius Werner
  -1 siblings, 0 replies; 26+ messages in thread
From: Julius Werner @ 2012-11-15  1:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: Len Brown, Rafael J. Wysocki, Kevin Hilman, Andrew Morton,
	Srivatsa S. Bhat, linux-acpi, linux-pm, linuxppc-dev,
	Deepthi Dharwar, Trinabh Gupta, Sameer Nanda, Lists Linaro-dev,
	Daniel Lezcano, Julius Werner

Many cpuidle drivers measure their time spent in an idle state by
reading the wallclock time before and after idling and calculating the
difference. This leads to erroneous results when the wallclock time gets
updated by another processor in the meantime, adding that clock
adjustment to the idle state's time counter.

If the clock adjustment was negative, the result is even worse due to an
erroneous cast from int to unsigned long long of the last_residency
variable. The negative 32 bit integer will zero-extend and result in a
forward time jump of roughly four billion milliseconds or 1.3 hours on
the idle state residency counter.

This patch changes all affected cpuidle drivers to either use the
monotonic clock for their measurements or make use of the generic time
measurement wrapper in cpuidle.c, which was already working correctly.
Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
should always already be disabled before entering the idle function, and
not get reenabled until the generic wrapper has performed its second
measurement). It also removes the erroneous cast, making sure that
negative residency values are applied correctly even though they should
not appear anymore.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 arch/powerpc/platforms/pseries/processor_idle.c |    4 +-
 drivers/acpi/processor_idle.c                   |   57 +---------------------
 drivers/cpuidle/cpuidle.c                       |    3 +-
 drivers/idle/intel_idle.c                       |   14 +-----
 4 files changed, 7 insertions(+), 71 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
index 45d00e5..4d806b4 100644
--- a/arch/powerpc/platforms/pseries/processor_idle.c
+++ b/arch/powerpc/platforms/pseries/processor_idle.c
@@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
 static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
 {
 
-	*kt_before = ktime_get_real();
+	*kt_before = ktime_get();
 	*in_purr = mfspr(SPRN_PURR);
 	/*
 	 * Indicate to the HV that we are idle. Now would be
@@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
 	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
 	get_lppaca()->idle = 0;
 
-	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
+	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
 }
 
 static int snooze_loop(struct cpuidle_device *dev,
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index e8086c7..f1a5da4 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -735,31 +735,18 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 		struct cpuidle_driver *drv, int index)
 {
-	ktime_t  kt1, kt2;
-	s64 idle_time;
 	struct acpi_processor *pr;
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
 
 	pr = __this_cpu_read(processors);
-	dev->last_residency = 0;
 
 	if (unlikely(!pr))
 		return -EINVAL;
 
-	local_irq_disable();
-
-
 	lapic_timer_state_broadcast(pr, cx, 1);
-	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
-	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
-
-	/* Update device last_residency*/
-	dev->last_residency = (int)idle_time;
 
-	local_irq_enable();
 	lapic_timer_state_broadcast(pr, cx, 0);
 
 	return index;
@@ -806,19 +793,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	struct acpi_processor *pr;
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
-	ktime_t  kt1, kt2;
-	s64 idle_time_ns;
-	s64 idle_time;
 
 	pr = __this_cpu_read(processors);
-	dev->last_residency = 0;
 
 	if (unlikely(!pr))
 		return -EINVAL;
 
-	local_irq_disable();
-
-
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -829,7 +809,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 		if (unlikely(need_resched())) {
 			current_thread_info()->status |= TS_POLLING;
-			local_irq_enable();
 			return -EINVAL;
 		}
 	}
@@ -843,22 +822,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	kt1 = ktime_get_real();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
-	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
-	idle_time = idle_time_ns;
-	do_div(idle_time, NSEC_PER_USEC);
 
-	/* Update device last_residency*/
-	dev->last_residency = (int)idle_time;
+	sched_clock_idle_wakeup_event(0);
 
-	/* Tell the scheduler how much we idled: */
-	sched_clock_idle_wakeup_event(idle_time_ns);
-
-	local_irq_enable();
 	if (cx->entry_method != ACPI_CSTATE_FFH)
 		current_thread_info()->status |= TS_POLLING;
 
@@ -883,13 +852,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	struct acpi_processor *pr;
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
-	ktime_t  kt1, kt2;
-	s64 idle_time_ns;
-	s64 idle_time;
-
 
 	pr = __this_cpu_read(processors);
-	dev->last_residency = 0;
 
 	if (unlikely(!pr))
 		return -EINVAL;
@@ -899,16 +863,11 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 			return drv->states[drv->safe_state_index].enter(dev,
 						drv, drv->safe_state_index);
 		} else {
-			local_irq_disable();
 			acpi_safe_halt();
-			local_irq_enable();
 			return -EBUSY;
 		}
 	}
 
-	local_irq_disable();
-
-
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -919,7 +878,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 
 		if (unlikely(need_resched())) {
 			current_thread_info()->status |= TS_POLLING;
-			local_irq_enable();
 			return -EINVAL;
 		}
 	}
@@ -934,7 +892,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	 */
 	lapic_timer_state_broadcast(pr, cx, 1);
 
-	kt1 = ktime_get_real();
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
@@ -965,18 +922,9 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		c3_cpu_count--;
 		raw_spin_unlock(&c3_lock);
 	}
-	kt2 = ktime_get_real();
-	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
-	idle_time = idle_time_ns;
-	do_div(idle_time, NSEC_PER_USEC);
-
-	/* Update device last_residency*/
-	dev->last_residency = (int)idle_time;
 
-	/* Tell the scheduler how much we idled: */
-	sched_clock_idle_wakeup_event(idle_time_ns);
+	sched_clock_idle_wakeup_event(0);
 
-	local_irq_enable();
 	if (cx->entry_method != ACPI_CSTATE_FFH)
 		current_thread_info()->status |= TS_POLLING;
 
@@ -987,6 +935,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 struct cpuidle_driver acpi_idle_driver = {
 	.name =		"acpi_idle",
 	.owner =	THIS_MODULE,
+	.en_core_tk_irqen = 1,
 };
 
 /**
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7f15b85..1536edd 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		/* This can be moved to within driver enter routine
 		 * but that results in multiple copies of same code.
 		 */
-		dev->states_usage[entered_state].time +=
-				(unsigned long long)dev->last_residency;
+		dev->states_usage[entered_state].time += dev->last_residency;
 		dev->states_usage[entered_state].usage++;
 	} else {
 		dev->last_residency = 0;
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index b0f6b4c..c49c04d 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -56,7 +56,6 @@
 #include <linux/kernel.h>
 #include <linux/cpuidle.h>
 #include <linux/clockchips.h>
-#include <linux/hrtimer.h>	/* ktime_get_real() */
 #include <trace/events/power.h>
 #include <linux/sched.h>
 #include <linux/notifier.h>
@@ -72,6 +71,7 @@
 static struct cpuidle_driver intel_idle_driver = {
 	.name = "intel_idle",
 	.owner = THIS_MODULE,
+	.en_core_tk_irqen = 1,
 };
 /* intel_idle.max_cstate=0 disables driver */
 static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;
@@ -281,8 +281,6 @@ static int intel_idle(struct cpuidle_device *dev,
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
 	unsigned int cstate;
-	ktime_t kt_before, kt_after;
-	s64 usec_delta;
 	int cpu = smp_processor_id();
 
 	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
@@ -297,8 +295,6 @@ static int intel_idle(struct cpuidle_device *dev,
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
-	kt_before = ktime_get_real();
-
 	stop_critical_timings();
 	if (!need_resched()) {
 
@@ -310,17 +306,9 @@ static int intel_idle(struct cpuidle_device *dev,
 
 	start_critical_timings();
 
-	kt_after = ktime_get_real();
-	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
-
-	local_irq_enable();
-
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
 
-	/* Update cpuidle counters */
-	dev->last_residency = (int)usec_delta;
-
 	return index;
 }
 
-- 
1.7.8.6


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

* [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-15  1:56       ` Julius Werner
  0 siblings, 0 replies; 26+ messages in thread
From: Julius Werner @ 2012-11-15  1:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm, Daniel Lezcano, Rafael J. Wysocki, linux-acpi,
	Srivatsa S. Bhat, Julius Werner, Andrew Morton, linuxppc-dev,
	Sameer Nanda, Len Brown

Many cpuidle drivers measure their time spent in an idle state by
reading the wallclock time before and after idling and calculating the
difference. This leads to erroneous results when the wallclock time gets
updated by another processor in the meantime, adding that clock
adjustment to the idle state's time counter.

If the clock adjustment was negative, the result is even worse due to an
erroneous cast from int to unsigned long long of the last_residency
variable. The negative 32 bit integer will zero-extend and result in a
forward time jump of roughly four billion milliseconds or 1.3 hours on
the idle state residency counter.

This patch changes all affected cpuidle drivers to either use the
monotonic clock for their measurements or make use of the generic time
measurement wrapper in cpuidle.c, which was already working correctly.
Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
should always already be disabled before entering the idle function, and
not get reenabled until the generic wrapper has performed its second
measurement). It also removes the erroneous cast, making sure that
negative residency values are applied correctly even though they should
not appear anymore.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 arch/powerpc/platforms/pseries/processor_idle.c |    4 +-
 drivers/acpi/processor_idle.c                   |   57 +---------------------
 drivers/cpuidle/cpuidle.c                       |    3 +-
 drivers/idle/intel_idle.c                       |   14 +-----
 4 files changed, 7 insertions(+), 71 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
index 45d00e5..4d806b4 100644
--- a/arch/powerpc/platforms/pseries/processor_idle.c
+++ b/arch/powerpc/platforms/pseries/processor_idle.c
@@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
 static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
 {
 
-	*kt_before = ktime_get_real();
+	*kt_before = ktime_get();
 	*in_purr = mfspr(SPRN_PURR);
 	/*
 	 * Indicate to the HV that we are idle. Now would be
@@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
 	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
 	get_lppaca()->idle = 0;
 
-	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
+	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
 }
 
 static int snooze_loop(struct cpuidle_device *dev,
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index e8086c7..f1a5da4 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -735,31 +735,18 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 		struct cpuidle_driver *drv, int index)
 {
-	ktime_t  kt1, kt2;
-	s64 idle_time;
 	struct acpi_processor *pr;
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
 
 	pr = __this_cpu_read(processors);
-	dev->last_residency = 0;
 
 	if (unlikely(!pr))
 		return -EINVAL;
 
-	local_irq_disable();
-
-
 	lapic_timer_state_broadcast(pr, cx, 1);
-	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
-	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
-
-	/* Update device last_residency*/
-	dev->last_residency = (int)idle_time;
 
-	local_irq_enable();
 	lapic_timer_state_broadcast(pr, cx, 0);
 
 	return index;
@@ -806,19 +793,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	struct acpi_processor *pr;
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
-	ktime_t  kt1, kt2;
-	s64 idle_time_ns;
-	s64 idle_time;
 
 	pr = __this_cpu_read(processors);
-	dev->last_residency = 0;
 
 	if (unlikely(!pr))
 		return -EINVAL;
 
-	local_irq_disable();
-
-
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -829,7 +809,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 
 		if (unlikely(need_resched())) {
 			current_thread_info()->status |= TS_POLLING;
-			local_irq_enable();
 			return -EINVAL;
 		}
 	}
@@ -843,22 +822,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	kt1 = ktime_get_real();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
-	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
-	idle_time = idle_time_ns;
-	do_div(idle_time, NSEC_PER_USEC);
 
-	/* Update device last_residency*/
-	dev->last_residency = (int)idle_time;
+	sched_clock_idle_wakeup_event(0);
 
-	/* Tell the scheduler how much we idled: */
-	sched_clock_idle_wakeup_event(idle_time_ns);
-
-	local_irq_enable();
 	if (cx->entry_method != ACPI_CSTATE_FFH)
 		current_thread_info()->status |= TS_POLLING;
 
@@ -883,13 +852,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	struct acpi_processor *pr;
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
-	ktime_t  kt1, kt2;
-	s64 idle_time_ns;
-	s64 idle_time;
-
 
 	pr = __this_cpu_read(processors);
-	dev->last_residency = 0;
 
 	if (unlikely(!pr))
 		return -EINVAL;
@@ -899,16 +863,11 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 			return drv->states[drv->safe_state_index].enter(dev,
 						drv, drv->safe_state_index);
 		} else {
-			local_irq_disable();
 			acpi_safe_halt();
-			local_irq_enable();
 			return -EBUSY;
 		}
 	}
 
-	local_irq_disable();
-
-
 	if (cx->entry_method != ACPI_CSTATE_FFH) {
 		current_thread_info()->status &= ~TS_POLLING;
 		/*
@@ -919,7 +878,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 
 		if (unlikely(need_resched())) {
 			current_thread_info()->status |= TS_POLLING;
-			local_irq_enable();
 			return -EINVAL;
 		}
 	}
@@ -934,7 +892,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	 */
 	lapic_timer_state_broadcast(pr, cx, 1);
 
-	kt1 = ktime_get_real();
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
@@ -965,18 +922,9 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		c3_cpu_count--;
 		raw_spin_unlock(&c3_lock);
 	}
-	kt2 = ktime_get_real();
-	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
-	idle_time = idle_time_ns;
-	do_div(idle_time, NSEC_PER_USEC);
-
-	/* Update device last_residency*/
-	dev->last_residency = (int)idle_time;
 
-	/* Tell the scheduler how much we idled: */
-	sched_clock_idle_wakeup_event(idle_time_ns);
+	sched_clock_idle_wakeup_event(0);
 
-	local_irq_enable();
 	if (cx->entry_method != ACPI_CSTATE_FFH)
 		current_thread_info()->status |= TS_POLLING;
 
@@ -987,6 +935,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 struct cpuidle_driver acpi_idle_driver = {
 	.name =		"acpi_idle",
 	.owner =	THIS_MODULE,
+	.en_core_tk_irqen = 1,
 };
 
 /**
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7f15b85..1536edd 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 		/* This can be moved to within driver enter routine
 		 * but that results in multiple copies of same code.
 		 */
-		dev->states_usage[entered_state].time +=
-				(unsigned long long)dev->last_residency;
+		dev->states_usage[entered_state].time += dev->last_residency;
 		dev->states_usage[entered_state].usage++;
 	} else {
 		dev->last_residency = 0;
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index b0f6b4c..c49c04d 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -56,7 +56,6 @@
 #include <linux/kernel.h>
 #include <linux/cpuidle.h>
 #include <linux/clockchips.h>
-#include <linux/hrtimer.h>	/* ktime_get_real() */
 #include <trace/events/power.h>
 #include <linux/sched.h>
 #include <linux/notifier.h>
@@ -72,6 +71,7 @@
 static struct cpuidle_driver intel_idle_driver = {
 	.name = "intel_idle",
 	.owner = THIS_MODULE,
+	.en_core_tk_irqen = 1,
 };
 /* intel_idle.max_cstate=0 disables driver */
 static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;
@@ -281,8 +281,6 @@ static int intel_idle(struct cpuidle_device *dev,
 	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
 	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
 	unsigned int cstate;
-	ktime_t kt_before, kt_after;
-	s64 usec_delta;
 	int cpu = smp_processor_id();
 
 	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
@@ -297,8 +295,6 @@ static int intel_idle(struct cpuidle_device *dev,
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
 
-	kt_before = ktime_get_real();
-
 	stop_critical_timings();
 	if (!need_resched()) {
 
@@ -310,17 +306,9 @@ static int intel_idle(struct cpuidle_device *dev,
 
 	start_critical_timings();
 
-	kt_after = ktime_get_real();
-	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
-
-	local_irq_enable();
-
 	if (!(lapic_timer_reliable_states & (1 << (cstate))))
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
 
-	/* Update cpuidle counters */
-	dev->last_residency = (int)usec_delta;
-
 	return index;
 }
 
-- 
1.7.8.6

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-15  1:56       ` Julius Werner
@ 2012-11-15  9:04         ` Preeti Murthy
  -1 siblings, 0 replies; 26+ messages in thread
From: Preeti Murthy @ 2012-11-15  9:04 UTC (permalink / raw)
  To: Julius Werner
  Cc: linux-kernel, Len Brown, Rafael J. Wysocki, Kevin Hilman,
	Andrew Morton, Srivatsa S. Bhat, linux-acpi, linux-pm,
	linuxppc-dev, Deepthi Dharwar, Trinabh Gupta, Sameer Nanda,
	Lists Linaro-dev, Daniel Lezcano, Peter Zijlstra

Hi all,

The code looks correct and inviting to me as it has led to good cleanups.
I dont think passing 0 as the argument to the function
sched_clock_idle_wakeup_event()
should lead to problems,as it does not do anything useful with the
passed arguments.

My only curiosity is what was the purpose of passing idle residency time to
sched_clock_idle_wakeup_event() when this data could always be retrieved from
dev->last_residency for each cpu,which gets almost immediately updated.

But this does not seem to come in way of this patch for now.Anyway I
have added Peter to
the list so that he can opine about this issue if possible and needed.

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>


Regards
Preeti U Murthy

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-15  9:04         ` Preeti Murthy
  0 siblings, 0 replies; 26+ messages in thread
From: Preeti Murthy @ 2012-11-15  9:04 UTC (permalink / raw)
  To: Julius Werner
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	Peter Zijlstra, linux-pm, Daniel Lezcano, linux-kernel,
	Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat, Andrew Morton,
	linuxppc-dev, Sameer Nanda, Len Brown

Hi all,

The code looks correct and inviting to me as it has led to good cleanups.
I dont think passing 0 as the argument to the function
sched_clock_idle_wakeup_event()
should lead to problems,as it does not do anything useful with the
passed arguments.

My only curiosity is what was the purpose of passing idle residency time to
sched_clock_idle_wakeup_event() when this data could always be retrieved from
dev->last_residency for each cpu,which gets almost immediately updated.

But this does not seem to come in way of this patch for now.Anyway I
have added Peter to
the list so that he can opine about this issue if possible and needed.

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>


Regards
Preeti U Murthy

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-15  1:56       ` Julius Werner
  (?)
@ 2012-11-15 10:52           ` Daniel Lezcano
  -1 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-15 10:52 UTC (permalink / raw)
  To: Julius Werner
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafael J. Wysocki,
	linux-acpi-u79uwXL29TY76Z2rM5mHXA, Srivatsa S. Bhat,
	Andrew Morton, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ, Sameer Nanda,
	Len Brown

On 11/15/2012 02:56 AM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Tested on a Core 2 Duo (processor_idle driver).

Tested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-15 10:52           ` Daniel Lezcano
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-15 10:52 UTC (permalink / raw)
  To: Julius Werner
  Cc: linux-kernel, Len Brown, Rafael J. Wysocki, Kevin Hilman,
	Andrew Morton, Srivatsa S. Bhat, linux-acpi, linux-pm,
	linuxppc-dev, Deepthi Dharwar, Trinabh Gupta, Sameer Nanda,
	Lists Linaro-dev

On 11/15/2012 02:56 AM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Tested on a Core 2 Duo (processor_idle driver).

Tested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-15 10:52           ` Daniel Lezcano
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Lezcano @ 2012-11-15 10:52 UTC (permalink / raw)
  To: Julius Werner
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm, linux-kernel, Rafael J. Wysocki, linux-acpi,
	Srivatsa S. Bhat, Andrew Morton, linuxppc-dev, Sameer Nanda,
	Len Brown

On 11/15/2012 02:56 AM, Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Tested on a Core 2 Duo (processor_idle driver).

Tested-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-15  1:56       ` Julius Werner
@ 2012-11-21  0:17         ` Rafael J. Wysocki
  -1 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2012-11-21  0:17 UTC (permalink / raw)
  To: Len Brown, Len Brown
  Cc: Julius Werner, linux-kernel, Kevin Hilman, Andrew Morton,
	Srivatsa S. Bhat, linux-acpi, linux-pm, linuxppc-dev,
	Deepthi Dharwar, Trinabh Gupta, Sameer Nanda, Lists Linaro-dev,
	Daniel Lezcano

On Wednesday, November 14, 2012 05:56:30 PM Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Len, I need your ACK on intel_idle change in this patch if you agree with it.

Thanks,
Rafael


> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 +-
>  drivers/acpi/processor_idle.c                   |   57 +---------------------
>  drivers/cpuidle/cpuidle.c                       |    3 +-
>  drivers/idle/intel_idle.c                       |   14 +-----
>  4 files changed, 7 insertions(+), 71 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
>  
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
>  
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
>  
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..f1a5da4 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -735,31 +735,18 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
>  static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index)
>  {
> -	ktime_t  kt1, kt2;
> -	s64 idle_time;
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	local_irq_enable();
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  
>  	return index;
> @@ -806,19 +793,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -829,7 +809,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -843,22 +822,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
> -	kt1 = ktime_get_real();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
>  
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
> +	sched_clock_idle_wakeup_event(0);
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> -
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -883,13 +852,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
> -
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
> @@ -899,16 +863,11 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  			return drv->states[drv->safe_state_index].enter(dev,
>  						drv, drv->safe_state_index);
>  		} else {
> -			local_irq_disable();
>  			acpi_safe_halt();
> -			local_irq_enable();
>  			return -EBUSY;
>  		}
>  	}
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -919,7 +878,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -934,7 +892,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  
> -	kt1 = ktime_get_real();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,18 +922,9 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> +	sched_clock_idle_wakeup_event(0);
>  
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -987,6 +935,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  struct cpuidle_driver acpi_idle_driver = {
>  	.name =		"acpi_idle",
>  	.owner =	THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  
>  /**
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..1536edd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
>  		 */
> -		dev->states_usage[entered_state].time +=
> -				(unsigned long long)dev->last_residency;
> +		dev->states_usage[entered_state].time += dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
>  	} else {
>  		dev->last_residency = 0;
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b0f6b4c..c49c04d 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,7 +56,6 @@
>  #include <linux/kernel.h>
>  #include <linux/cpuidle.h>
>  #include <linux/clockchips.h>
> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>  #include <trace/events/power.h>
>  #include <linux/sched.h>
>  #include <linux/notifier.h>
> @@ -72,6 +71,7 @@
>  static struct cpuidle_driver intel_idle_driver = {
>  	.name = "intel_idle",
>  	.owner = THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  /* intel_idle.max_cstate=0 disables driver */
>  static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;
> @@ -281,8 +281,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>  	unsigned int cstate;
> -	ktime_t kt_before, kt_after;
> -	s64 usec_delta;
>  	int cpu = smp_processor_id();
>  
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
> @@ -297,8 +295,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>  
> -	kt_before = ktime_get_real();
> -
>  	stop_critical_timings();
>  	if (!need_resched()) {
>  
> @@ -310,17 +306,9 @@ static int intel_idle(struct cpuidle_device *dev,
>  
>  	start_critical_timings();
>  
> -	kt_after = ktime_get_real();
> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
> -
> -	local_irq_enable();
> -
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>  
> -	/* Update cpuidle counters */
> -	dev->last_residency = (int)usec_delta;
> -
>  	return index;
>  }
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-21  0:17         ` Rafael J. Wysocki
  0 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2012-11-21  0:17 UTC (permalink / raw)
  To: Len Brown, Len Brown
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm, Daniel Lezcano, linux-kernel, linux-acpi,
	Srivatsa S. Bhat, Julius Werner, Andrew Morton, linuxppc-dev,
	Sameer Nanda

On Wednesday, November 14, 2012 05:56:30 PM Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Len, I need your ACK on intel_idle change in this patch if you agree with it.

Thanks,
Rafael


> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 +-
>  drivers/acpi/processor_idle.c                   |   57 +---------------------
>  drivers/cpuidle/cpuidle.c                       |    3 +-
>  drivers/idle/intel_idle.c                       |   14 +-----
>  4 files changed, 7 insertions(+), 71 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
>  
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
>  
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
>  
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..f1a5da4 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -735,31 +735,18 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
>  static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index)
>  {
> -	ktime_t  kt1, kt2;
> -	s64 idle_time;
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	local_irq_enable();
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  
>  	return index;
> @@ -806,19 +793,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -829,7 +809,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -843,22 +822,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
> -	kt1 = ktime_get_real();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
>  
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
> +	sched_clock_idle_wakeup_event(0);
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> -
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -883,13 +852,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
> -
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
> @@ -899,16 +863,11 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  			return drv->states[drv->safe_state_index].enter(dev,
>  						drv, drv->safe_state_index);
>  		} else {
> -			local_irq_disable();
>  			acpi_safe_halt();
> -			local_irq_enable();
>  			return -EBUSY;
>  		}
>  	}
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -919,7 +878,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -934,7 +892,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  
> -	kt1 = ktime_get_real();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,18 +922,9 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> +	sched_clock_idle_wakeup_event(0);
>  
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -987,6 +935,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  struct cpuidle_driver acpi_idle_driver = {
>  	.name =		"acpi_idle",
>  	.owner =	THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  
>  /**
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..1536edd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
>  		 */
> -		dev->states_usage[entered_state].time +=
> -				(unsigned long long)dev->last_residency;
> +		dev->states_usage[entered_state].time += dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
>  	} else {
>  		dev->last_residency = 0;
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b0f6b4c..c49c04d 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,7 +56,6 @@
>  #include <linux/kernel.h>
>  #include <linux/cpuidle.h>
>  #include <linux/clockchips.h>
> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>  #include <trace/events/power.h>
>  #include <linux/sched.h>
>  #include <linux/notifier.h>
> @@ -72,6 +71,7 @@
>  static struct cpuidle_driver intel_idle_driver = {
>  	.name = "intel_idle",
>  	.owner = THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  /* intel_idle.max_cstate=0 disables driver */
>  static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;
> @@ -281,8 +281,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>  	unsigned int cstate;
> -	ktime_t kt_before, kt_after;
> -	s64 usec_delta;
>  	int cpu = smp_processor_id();
>  
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
> @@ -297,8 +295,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>  
> -	kt_before = ktime_get_real();
> -
>  	stop_critical_timings();
>  	if (!need_resched()) {
>  
> @@ -310,17 +306,9 @@ static int intel_idle(struct cpuidle_device *dev,
>  
>  	start_critical_timings();
>  
> -	kt_after = ktime_get_real();
> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
> -
> -	local_irq_enable();
> -
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>  
> -	/* Update cpuidle counters */
> -	dev->last_residency = (int)usec_delta;
> -
>  	return index;
>  }
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-15  9:04         ` Preeti Murthy
@ 2012-11-27  6:14           ` Len Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Len Brown @ 2012-11-27  6:14 UTC (permalink / raw)
  To: Preeti Murthy
  Cc: Julius Werner, linux-kernel, Rafael J. Wysocki, Kevin Hilman,
	Andrew Morton, Srivatsa S. Bhat, linux-acpi, linux-pm,
	linuxppc-dev, Deepthi Dharwar, Trinabh Gupta, Sameer Nanda,
	Lists Linaro-dev, Daniel Lezcano, Peter Zijlstra

On 11/15/2012 04:04 AM, Preeti Murthy wrote:
> Hi all,
> 
> The code looks correct and inviting to me as it has led to good cleanups.
> I dont think passing 0 as the argument to the function
> sched_clock_idle_wakeup_event()
> should lead to problems,as it does not do anything useful with the
> passed arguments.
> 
> My only curiosity is what was the purpose of passing idle residency time to
> sched_clock_idle_wakeup_event() when this data could always be retrieved from
> dev->last_residency for each cpu,which gets almost immediately updated.

sched_clock_idle_wakeup_event() is part of the scheduler.
The scheduler doesn't know what a cpuidle_device is, and
probably should not grow such a dependency.

cheers,
-Len Brown, Intel Open Source Technology Center

> But this does not seem to come in way of this patch for now.Anyway I
> have added Peter to
> the list so that he can opine about this issue if possible and needed.
> 
> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> 
> Regards
> Preeti U Murthy
> 


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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-27  6:14           ` Len Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Len Brown @ 2012-11-27  6:14 UTC (permalink / raw)
  To: Preeti Murthy
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	Peter Zijlstra, linux-pm, Daniel Lezcano, linux-kernel,
	Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat, Julius Werner,
	Andrew Morton, linuxppc-dev, Sameer Nanda

On 11/15/2012 04:04 AM, Preeti Murthy wrote:
> Hi all,
> 
> The code looks correct and inviting to me as it has led to good cleanups.
> I dont think passing 0 as the argument to the function
> sched_clock_idle_wakeup_event()
> should lead to problems,as it does not do anything useful with the
> passed arguments.
> 
> My only curiosity is what was the purpose of passing idle residency time to
> sched_clock_idle_wakeup_event() when this data could always be retrieved from
> dev->last_residency for each cpu,which gets almost immediately updated.

sched_clock_idle_wakeup_event() is part of the scheduler.
The scheduler doesn't know what a cpuidle_device is, and
probably should not grow such a dependency.

cheers,
-Len Brown, Intel Open Source Technology Center

> But this does not seem to come in way of this patch for now.Anyway I
> have added Peter to
> the list so that he can opine about this issue if possible and needed.
> 
> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> 
> Regards
> Preeti U Murthy
> 

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-21  0:17         ` Rafael J. Wysocki
@ 2012-11-27  6:15           ` Len Brown
  -1 siblings, 0 replies; 26+ messages in thread
From: Len Brown @ 2012-11-27  6:15 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Julius Werner, linux-kernel, Kevin Hilman, Andrew Morton,
	Srivatsa S. Bhat, linux-acpi, linux-pm, linuxppc-dev,
	Deepthi Dharwar, Trinabh Gupta, Sameer Nanda, Lists Linaro-dev,
	Daniel Lezcano


>>  drivers/idle/intel_idle.c                       |   14 +-----

Acked-by: Len Brown <len.brown@intel.com>

thanks!
-Len


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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-27  6:15           ` Len Brown
  0 siblings, 0 replies; 26+ messages in thread
From: Len Brown @ 2012-11-27  6:15 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	linux-pm, Daniel Lezcano, linux-kernel, linux-acpi,
	Srivatsa S. Bhat, Julius Werner, Andrew Morton, linuxppc-dev,
	Sameer Nanda


>>  drivers/idle/intel_idle.c                       |   14 +-----

Acked-by: Len Brown <len.brown@intel.com>

thanks!
-Len

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
  2012-11-15  1:56       ` Julius Werner
@ 2012-11-27 19:12         ` Rafael J. Wysocki
  -1 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2012-11-27 19:12 UTC (permalink / raw)
  To: linux-pm
  Cc: Julius Werner, linux-kernel, Len Brown, Kevin Hilman,
	Andrew Morton, Srivatsa S. Bhat, linux-acpi, linuxppc-dev,
	Deepthi Dharwar, Trinabh Gupta, Sameer Nanda, Lists Linaro-dev,
	Daniel Lezcano

On Wednesday, November 14, 2012 05:56:30 PM Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.

Applied to the linux-next branch of the linux-pm.git tree.

Thanks,
Rafael


> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 +-
>  drivers/acpi/processor_idle.c                   |   57 +---------------------
>  drivers/cpuidle/cpuidle.c                       |    3 +-
>  drivers/idle/intel_idle.c                       |   14 +-----
>  4 files changed, 7 insertions(+), 71 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
>  
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
>  
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
>  
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..f1a5da4 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -735,31 +735,18 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
>  static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index)
>  {
> -	ktime_t  kt1, kt2;
> -	s64 idle_time;
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	local_irq_enable();
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  
>  	return index;
> @@ -806,19 +793,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -829,7 +809,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -843,22 +822,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
> -	kt1 = ktime_get_real();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
>  
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
> +	sched_clock_idle_wakeup_event(0);
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> -
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -883,13 +852,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
> -
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
> @@ -899,16 +863,11 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  			return drv->states[drv->safe_state_index].enter(dev,
>  						drv, drv->safe_state_index);
>  		} else {
> -			local_irq_disable();
>  			acpi_safe_halt();
> -			local_irq_enable();
>  			return -EBUSY;
>  		}
>  	}
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -919,7 +878,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -934,7 +892,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  
> -	kt1 = ktime_get_real();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,18 +922,9 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> +	sched_clock_idle_wakeup_event(0);
>  
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -987,6 +935,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  struct cpuidle_driver acpi_idle_driver = {
>  	.name =		"acpi_idle",
>  	.owner =	THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  
>  /**
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..1536edd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
>  		 */
> -		dev->states_usage[entered_state].time +=
> -				(unsigned long long)dev->last_residency;
> +		dev->states_usage[entered_state].time += dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
>  	} else {
>  		dev->last_residency = 0;
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b0f6b4c..c49c04d 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,7 +56,6 @@
>  #include <linux/kernel.h>
>  #include <linux/cpuidle.h>
>  #include <linux/clockchips.h>
> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>  #include <trace/events/power.h>
>  #include <linux/sched.h>
>  #include <linux/notifier.h>
> @@ -72,6 +71,7 @@
>  static struct cpuidle_driver intel_idle_driver = {
>  	.name = "intel_idle",
>  	.owner = THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  /* intel_idle.max_cstate=0 disables driver */
>  static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;
> @@ -281,8 +281,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>  	unsigned int cstate;
> -	ktime_t kt_before, kt_after;
> -	s64 usec_delta;
>  	int cpu = smp_processor_id();
>  
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
> @@ -297,8 +295,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>  
> -	kt_before = ktime_get_real();
> -
>  	stop_critical_timings();
>  	if (!need_resched()) {
>  
> @@ -310,17 +306,9 @@ static int intel_idle(struct cpuidle_device *dev,
>  
>  	start_critical_timings();
>  
> -	kt_after = ktime_get_real();
> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
> -
> -	local_irq_enable();
> -
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>  
> -	/* Update cpuidle counters */
> -	dev->last_residency = (int)usec_delta;
> -
>  	return index;
>  }
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
@ 2012-11-27 19:12         ` Rafael J. Wysocki
  0 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2012-11-27 19:12 UTC (permalink / raw)
  To: linux-pm
  Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
	Daniel Lezcano, linux-kernel, linux-acpi, Srivatsa S. Bhat,
	Julius Werner, Andrew Morton, linuxppc-dev, Sameer Nanda,
	Len Brown

On Wednesday, November 14, 2012 05:56:30 PM Julius Werner wrote:
> Many cpuidle drivers measure their time spent in an idle state by
> reading the wallclock time before and after idling and calculating the
> difference. This leads to erroneous results when the wallclock time gets
> updated by another processor in the meantime, adding that clock
> adjustment to the idle state's time counter.
> 
> If the clock adjustment was negative, the result is even worse due to an
> erroneous cast from int to unsigned long long of the last_residency
> variable. The negative 32 bit integer will zero-extend and result in a
> forward time jump of roughly four billion milliseconds or 1.3 hours on
> the idle state residency counter.
> 
> This patch changes all affected cpuidle drivers to either use the
> monotonic clock for their measurements or make use of the generic time
> measurement wrapper in cpuidle.c, which was already working correctly.
> Some superfluous CLIs/STIs in the ACPI code are removed (interrupts
> should always already be disabled before entering the idle function, and
> not get reenabled until the generic wrapper has performed its second
> measurement). It also removes the erroneous cast, making sure that
> negative residency values are applied correctly even though they should
> not appear anymore.

Applied to the linux-next branch of the linux-pm.git tree.

Thanks,
Rafael


> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  arch/powerpc/platforms/pseries/processor_idle.c |    4 +-
>  drivers/acpi/processor_idle.c                   |   57 +---------------------
>  drivers/cpuidle/cpuidle.c                       |    3 +-
>  drivers/idle/intel_idle.c                       |   14 +-----
>  4 files changed, 7 insertions(+), 71 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 45d00e5..4d806b4 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -36,7 +36,7 @@ static struct cpuidle_state *cpuidle_state_table;
>  static inline void idle_loop_prolog(unsigned long *in_purr, ktime_t *kt_before)
>  {
>  
> -	*kt_before = ktime_get_real();
> +	*kt_before = ktime_get();
>  	*in_purr = mfspr(SPRN_PURR);
>  	/*
>  	 * Indicate to the HV that we are idle. Now would be
> @@ -50,7 +50,7 @@ static inline  s64 idle_loop_epilog(unsigned long in_purr, ktime_t kt_before)
>  	get_lppaca()->wait_state_cycles += mfspr(SPRN_PURR) - in_purr;
>  	get_lppaca()->idle = 0;
>  
> -	return ktime_to_us(ktime_sub(ktime_get_real(), kt_before));
> +	return ktime_to_us(ktime_sub(ktime_get(), kt_before));
>  }
>  
>  static int snooze_loop(struct cpuidle_device *dev,
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index e8086c7..f1a5da4 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -735,31 +735,18 @@ static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
>  static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  		struct cpuidle_driver *drv, int index)
>  {
> -	ktime_t  kt1, kt2;
> -	s64 idle_time;
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	lapic_timer_state_broadcast(pr, cx, 1);
> -	kt1 = ktime_get_real();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	local_irq_enable();
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  
>  	return index;
> @@ -806,19 +793,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -829,7 +809,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -843,22 +822,12 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->type == ACPI_STATE_C3)
>  		ACPI_FLUSH_CPU_CACHE();
>  
> -	kt1 = ktime_get_real();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
>  
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
> +	sched_clock_idle_wakeup_event(0);
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> -
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -883,13 +852,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	struct acpi_processor *pr;
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	struct acpi_processor_cx *cx = cpuidle_get_statedata(state_usage);
> -	ktime_t  kt1, kt2;
> -	s64 idle_time_ns;
> -	s64 idle_time;
> -
>  
>  	pr = __this_cpu_read(processors);
> -	dev->last_residency = 0;
>  
>  	if (unlikely(!pr))
>  		return -EINVAL;
> @@ -899,16 +863,11 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  			return drv->states[drv->safe_state_index].enter(dev,
>  						drv, drv->safe_state_index);
>  		} else {
> -			local_irq_disable();
>  			acpi_safe_halt();
> -			local_irq_enable();
>  			return -EBUSY;
>  		}
>  	}
>  
> -	local_irq_disable();
> -
> -
>  	if (cx->entry_method != ACPI_CSTATE_FFH) {
>  		current_thread_info()->status &= ~TS_POLLING;
>  		/*
> @@ -919,7 +878,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  
>  		if (unlikely(need_resched())) {
>  			current_thread_info()->status |= TS_POLLING;
> -			local_irq_enable();
>  			return -EINVAL;
>  		}
>  	}
> @@ -934,7 +892,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  
> -	kt1 = ktime_get_real();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -965,18 +922,9 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		raw_spin_unlock(&c3_lock);
>  	}
> -	kt2 = ktime_get_real();
> -	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
> -	idle_time = idle_time_ns;
> -	do_div(idle_time, NSEC_PER_USEC);
> -
> -	/* Update device last_residency*/
> -	dev->last_residency = (int)idle_time;
>  
> -	/* Tell the scheduler how much we idled: */
> -	sched_clock_idle_wakeup_event(idle_time_ns);
> +	sched_clock_idle_wakeup_event(0);
>  
> -	local_irq_enable();
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> @@ -987,6 +935,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  struct cpuidle_driver acpi_idle_driver = {
>  	.name =		"acpi_idle",
>  	.owner =	THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  
>  /**
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 7f15b85..1536edd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -109,8 +109,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  		/* This can be moved to within driver enter routine
>  		 * but that results in multiple copies of same code.
>  		 */
> -		dev->states_usage[entered_state].time +=
> -				(unsigned long long)dev->last_residency;
> +		dev->states_usage[entered_state].time += dev->last_residency;
>  		dev->states_usage[entered_state].usage++;
>  	} else {
>  		dev->last_residency = 0;
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index b0f6b4c..c49c04d 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -56,7 +56,6 @@
>  #include <linux/kernel.h>
>  #include <linux/cpuidle.h>
>  #include <linux/clockchips.h>
> -#include <linux/hrtimer.h>	/* ktime_get_real() */
>  #include <trace/events/power.h>
>  #include <linux/sched.h>
>  #include <linux/notifier.h>
> @@ -72,6 +71,7 @@
>  static struct cpuidle_driver intel_idle_driver = {
>  	.name = "intel_idle",
>  	.owner = THIS_MODULE,
> +	.en_core_tk_irqen = 1,
>  };
>  /* intel_idle.max_cstate=0 disables driver */
>  static int max_cstate = MWAIT_MAX_NUM_CSTATES - 1;
> @@ -281,8 +281,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	struct cpuidle_state_usage *state_usage = &dev->states_usage[index];
>  	unsigned long eax = (unsigned long)cpuidle_get_statedata(state_usage);
>  	unsigned int cstate;
> -	ktime_t kt_before, kt_after;
> -	s64 usec_delta;
>  	int cpu = smp_processor_id();
>  
>  	cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1;
> @@ -297,8 +295,6 @@ static int intel_idle(struct cpuidle_device *dev,
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
>  
> -	kt_before = ktime_get_real();
> -
>  	stop_critical_timings();
>  	if (!need_resched()) {
>  
> @@ -310,17 +306,9 @@ static int intel_idle(struct cpuidle_device *dev,
>  
>  	start_critical_timings();
>  
> -	kt_after = ktime_get_real();
> -	usec_delta = ktime_to_us(ktime_sub(kt_after, kt_before));
> -
> -	local_irq_enable();
> -
>  	if (!(lapic_timer_reliable_states & (1 << (cstate))))
>  		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
>  
> -	/* Update cpuidle counters */
> -	dev->last_residency = (int)usec_delta;
> -
>  	return index;
>  }
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

end of thread, other threads:[~2012-11-27 19:14 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-13 21:52 [PATCH] cpuidle: Measure idle state durations with monotonic clock Julius Werner
2012-11-13 21:52 ` Julius Werner
2012-11-14  9:06 ` Deepthi Dharwar
2012-11-14  9:06   ` Deepthi Dharwar
2012-11-14 10:57   ` Daniel Lezcano
2012-11-14 10:57     ` Daniel Lezcano
2012-11-14 10:57     ` Daniel Lezcano
2012-11-14 11:05 ` Daniel Lezcano
2012-11-14 11:05   ` Daniel Lezcano
2012-11-14 17:15   ` Julius Werner
2012-11-14 17:15     ` Julius Werner
2012-11-15  1:56     ` Julius Werner
2012-11-15  1:56       ` Julius Werner
2012-11-15  9:04       ` Preeti Murthy
2012-11-15  9:04         ` Preeti Murthy
2012-11-27  6:14         ` Len Brown
2012-11-27  6:14           ` Len Brown
     [not found]       ` <1352944590-8776-1-git-send-email-jwerner-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2012-11-15 10:52         ` Daniel Lezcano
2012-11-15 10:52           ` Daniel Lezcano
2012-11-15 10:52           ` Daniel Lezcano
2012-11-21  0:17       ` Rafael J. Wysocki
2012-11-21  0:17         ` Rafael J. Wysocki
2012-11-27  6:15         ` Len Brown
2012-11-27  6:15           ` Len Brown
2012-11-27 19:12       ` Rafael J. Wysocki
2012-11-27 19:12         ` Rafael J. Wysocki

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.