[v11,7/8] cpuidle: Pre-store next timer/tick before selecting an idle state

Message ID 20190226145415.19411-8-ulf.hansson@linaro.org
State New
Headers show
Series
  • PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM)
Related show

Commit Message

Ulf Hansson Feb. 26, 2019, 2:54 p.m.
A common piece of data used by cpuidle governors, is the information about
when the next timer/tick is going to fire. Rather than having each governor
calling tick_nohz_get_next_timer|hrtimer() separately, let's consolidate
the code by calling these functions before invoking the ->select() callback
of the governor - and store the output data in the struct cpuidle_device.

Besides the consolidation benefit, the purpose of this change is also to
make the information about the next timer/tick, available outside the
cpuidle framework. Following changes that implements a new genpd governor,
makes use of this.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---

Changes in v11:
	- New patch.

---
 drivers/cpuidle/cpuidle.c        | 2 ++
 drivers/cpuidle/governors/menu.c | 6 ++----
 drivers/cpuidle/governors/teo.c  | 6 ++----
 include/linux/cpuidle.h          | 2 ++
 4 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.17.1

Comments

Rafael J. Wysocki Feb. 26, 2019, 10:17 p.m. | #1
On Tue, Feb 26, 2019 at 11:08 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>

> On Tue, Feb 26, 2019 at 3:54 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> >

> > A common piece of data used by cpuidle governors, is the information about

> > when the next timer/tick is going to fire. Rather than having each governor

> > calling tick_nohz_get_next_timer|hrtimer() separately, let's consolidate

> > the code by calling these functions before invoking the ->select() callback

> > of the governor - and store the output data in the struct cpuidle_device.

>

> That misses the point IMO.

>

> You don't need to store two values in struct cpuidle_device, but just

> one, and not before running ->select(), but before invoking the

> driver's ->enter() callback.

>

> At that point, the decision on whether or not to stop the scheduler

> tick has been made already and it should be sufficient to store the

> return value of tick_nohz_get_next_hrtimer() introduced by patch

> [3/8],


Or the difference between in and ts->idle_entrytime for that matter,
whichever is more useful.

> because that value represents the next timer regardless of what

> has been decided with respect to the tick.

>

> And you won't need the tick_nohz_get_next_timer() any more then.
Ulf Hansson Feb. 27, 2019, 12:07 a.m. | #2
On Wed, 27 Feb 2019 at 00:41, Rafael J. Wysocki <rafael@kernel.org> wrote:
>

> On Wed, Feb 27, 2019 at 12:16 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> >

> > On Tue, 26 Feb 2019 at 23:08, Rafael J. Wysocki <rafael@kernel.org> wrote:

> > >

> > > On Tue, Feb 26, 2019 at 3:54 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

> > > >

> > > > A common piece of data used by cpuidle governors, is the information about

> > > > when the next timer/tick is going to fire. Rather than having each governor

> > > > calling tick_nohz_get_next_timer|hrtimer() separately, let's consolidate

> > > > the code by calling these functions before invoking the ->select() callback

> > > > of the governor - and store the output data in the struct cpuidle_device.

> > >

> > > That misses the point IMO.

> > >

> > > You don't need to store two values in struct cpuidle_device, but just

> > > one, and not before running ->select(), but before invoking the

> > > driver's ->enter() callback.

> >

> > Okay! Thanks for letting me know!

> >

> > >

> > > At that point, the decision on whether or not to stop the scheduler

> > > tick has been made already and it should be sufficient to store the

> > > return value of tick_nohz_get_next_hrtimer() introduced by patch

> > > [3/8], because that value represents the next timer regardless of what

> > > has been decided with respect to the tick.

> >

> > Just to make sure I get this correctly, because it seems like I have

> > missed a few points here....

> >

> > If we decided to keep the tick running, then

> > tick_nohz_get_next_hrtimer() gives the next tick or the next hrtimer,

> > whatever that comes first. There are no other timer that can expire

> > earlier than this, right!?

> >

> > If we decided to stop the tick, then tick_nohz_get_next_hrtimer() will

> > give us the next hrtimer. Again, then there are no other timer that

> > can't expire earlier than this, right!?

>

> Right in both cases.

>

> IOW, that is the event that will wake up the CPU unless any other

> (non-timer) interrupts (or equivalent events) occur in the meantime.

>

> > >

> > > And you won't need the tick_nohz_get_next_timer() any more then.

> >

> > Alright, this kind of brings this hole thing back closer to v10 - and

> > then we should stick to use tick_nohz_get_sleep_length() as is for the

> > cpuidle governors. That is what you are saying?

>

> For the menu and teo governors - yes.  IMO

> tick_nohz_get_sleep_length() is as good as it gets in there.


Okay, got it! Thanks for your helpful answers!

I will post a v12 as soon as I can, so we can look into the other
parts of the series.

Kind regards
Uffe

Patch

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 7f108309e871..3b148253036b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -312,6 +312,8 @@  int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 		   bool *stop_tick)
 {
+	dev->next_timer = tick_nohz_get_next_timer();
+	dev->next_hrtimer = tick_nohz_get_next_hrtimer();
 	return cpuidle_curr_governor->select(drv, dev, stop_tick);
 }
 
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 95e9122d6047..cdbe434e783d 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -287,8 +287,6 @@  static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 	unsigned long nr_iowaiters;
 	ktime_t delta_next;
 	ktime_t now = ktime_get();
-	ktime_t next_hrtimer = tick_nohz_get_next_hrtimer();
-	ktime_t next_timer = tick_nohz_get_next_timer();
 
 	if (data->needs_update) {
 		menu_update(drv, dev);
@@ -298,14 +296,14 @@  static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 	/*
 	 * Compute the duration before the next timer, whatever the origin
 	 */
-	delta_next = ktime_sub(next_timer, now);
+	delta_next = ktime_sub(dev->next_timer, now);
 	data->next_timer_us = ktime_to_us(delta_next);
 
 	/*
 	 * Compute the duration before next hrtimer which is the tick
 	 * or an earliest hrtimer
 	 */
-	delta_next = ktime_sub(next_hrtimer, now);
+	delta_next = ktime_sub(dev->next_hrtimer, now);
 
 	nr_iowaiters = nr_iowait_cpu(dev->cpu);
 	data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index bef1e95c597e..7af9851d9d40 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -245,8 +245,6 @@  static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 	int max_early_idx, idx, i;
 	ktime_t delta_tick;
 	ktime_t now = ktime_get();
-	ktime_t next_hrtimer = tick_nohz_get_next_hrtimer();
-	ktime_t next_timer = tick_nohz_get_next_timer();
 
 	if (cpu_data->last_state >= 0) {
 		teo_update(drv, dev);
@@ -255,8 +253,8 @@  static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
 
 	cpu_data->time_span_ns = local_clock();
 
-	cpu_data->sleep_length_ns = ktime_sub(next_timer, now);
-	delta_tick = ktime_sub(next_hrtimer, now);
+	cpu_data->sleep_length_ns = ktime_sub(dev->next_timer, now);
+	delta_tick = ktime_sub(dev->next_hrtimer, now);
 	duration_us = ktime_to_us(cpu_data->sleep_length_ns);
 
 	count = 0;
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 3b39472324a3..81ec924206f0 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -83,6 +83,8 @@  struct cpuidle_device {
 	unsigned int		use_deepest_state:1;
 	unsigned int		poll_time_limit:1;
 	unsigned int		cpu;
+	ktime_t			next_timer;
+	ktime_t			next_hrtimer;
 
 	int			last_residency;
 	struct cpuidle_state_usage	states_usage[CPUIDLE_STATE_MAX];