All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: ville.syrjala@linux.intel.com
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH] Revert "cpuidle: Replace ktime_get() with local_clock()"
Date: Thu, 20 Apr 2017 15:08:13 +0200	[thread overview]
Message-ID: <20170420130813.h7dycr5cptbrvdkz@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20170420124447.13716-1-ville.syrjala@linux.intel.com>

On Thu, Apr 20, 2017 at 03:44:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This reverts commit e93e59ce5b85e6c2b444f09fd1f707274ec066dc.
> 
> The TSC stops in deeper C states, 

On some old hardware (Core2 era and before) only. You've forgotten to
mention what hardware you've observed problems with.

> so using local_clock() in cpuidle

But on said hardware, local_clock() isn't an immediate TSC user.

> to track the C state residency seems like a bad idea. With local_clock()
> powertop is reporting mostly 0% residency for C states here. Presumably
> the core is still spending most of its time in some deep C-state since
> the totals typically add up to only 5% or so, so perhaps the governor
> isn't getting totally confused by these bogus numbers. But let's go
> back to using ktime_get() as that at least works correctly across the
> board.

Does this cure it?

---
 drivers/cpuidle/cpuidle.c | 2 ++
 kernel/sched/clock.c      | 7 +++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 548b90be7685..e0d4ad108887 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -219,6 +219,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	entered_state = target_state->enter(dev, drv, index);
 	start_critical_timings();
 
+	sched_clock_idle_wakeup_event(0);
+
 	time_end = ns_to_ktime(local_clock());
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index 00a45c45beca..15e848706be4 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -347,6 +347,9 @@ void sched_clock_tick(void)
 {
 	struct sched_clock_data *scd;
 
+	if (timekeeping_suspended)
+		return;
+
 	WARN_ON_ONCE(!irqs_disabled());
 
 	/*
@@ -378,11 +381,7 @@ EXPORT_SYMBOL_GPL(sched_clock_idle_sleep_event);
  */
 void sched_clock_idle_wakeup_event(u64 delta_ns)
 {
-	if (timekeeping_suspended)
-		return;
-
 	sched_clock_tick();
-	touch_softlockup_watchdog_sched();
 }
 EXPORT_SYMBOL_GPL(sched_clock_idle_wakeup_event);
 

WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org>
To: ville.syrjala@linux.intel.com
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH] Revert "cpuidle: Replace ktime_get() with local_clock()"
Date: Thu, 20 Apr 2017 15:08:13 +0200	[thread overview]
Message-ID: <20170420130813.h7dycr5cptbrvdkz@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20170420124447.13716-1-ville.syrjala@linux.intel.com>

On Thu, Apr 20, 2017 at 03:44:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> This reverts commit e93e59ce5b85e6c2b444f09fd1f707274ec066dc.
> 
> The TSC stops in deeper C states, 

On some old hardware (Core2 era and before) only. You've forgotten to
mention what hardware you've observed problems with.

> so using local_clock() in cpuidle

But on said hardware, local_clock() isn't an immediate TSC user.

> to track the C state residency seems like a bad idea. With local_clock()
> powertop is reporting mostly 0% residency for C states here. Presumably
> the core is still spending most of its time in some deep C-state since
> the totals typically add up to only 5% or so, so perhaps the governor
> isn't getting totally confused by these bogus numbers. But let's go
> back to using ktime_get() as that at least works correctly across the
> board.

Does this cure it?

---
 drivers/cpuidle/cpuidle.c | 2 ++
 kernel/sched/clock.c      | 7 +++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 548b90be7685..e0d4ad108887 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -219,6 +219,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	entered_state = target_state->enter(dev, drv, index);
 	start_critical_timings();
 
+	sched_clock_idle_wakeup_event(0);
+
 	time_end = ns_to_ktime(local_clock());
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index 00a45c45beca..15e848706be4 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -347,6 +347,9 @@ void sched_clock_tick(void)
 {
 	struct sched_clock_data *scd;
 
+	if (timekeeping_suspended)
+		return;
+
 	WARN_ON_ONCE(!irqs_disabled());
 
 	/*
@@ -378,11 +381,7 @@ EXPORT_SYMBOL_GPL(sched_clock_idle_sleep_event);
  */
 void sched_clock_idle_wakeup_event(u64 delta_ns)
 {
-	if (timekeeping_suspended)
-		return;
-
 	sched_clock_tick();
-	touch_softlockup_watchdog_sched();
 }
 EXPORT_SYMBOL_GPL(sched_clock_idle_wakeup_event);
 

  parent reply	other threads:[~2017-04-20 13:08 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-20 12:44 [PATCH] Revert "cpuidle: Replace ktime_get() with local_clock()" ville.syrjala
2017-04-20 13:07 ` Daniel Lezcano
2017-04-20 13:08 ` Peter Zijlstra [this message]
2017-04-20 13:08   ` Peter Zijlstra
2017-04-20 13:43   ` Ville Syrjälä
2017-04-20 13:43     ` Ville Syrjälä
2017-04-20 13:49     ` Peter Zijlstra
2017-04-20 13:49       ` Peter Zijlstra
2017-04-20 14:37   ` Daniel Lezcano
2017-04-20 14:41     ` Peter Zijlstra
2017-04-20 14:47       ` Peter Zijlstra

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20170420130813.h7dycr5cptbrvdkz@hirez.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rafael.j.wysocki@intel.com \
    --cc=stable@vger.kernel.org \
    --cc=ville.syrjala@linux.intel.com \
    /path/to/YOUR_REPLY

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

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