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

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. UTC
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(-)

Comments

Rafael J. Wysocki Feb. 26, 2019, 10:08 p.m. UTC | #1
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], 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.
Rafael J. Wysocki Feb. 26, 2019, 10:17 p.m. UTC | #2
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. 26, 2019, 11:15 p.m. UTC | #3
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!?

>
> 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?

Kind regards
Uffe
Rafael J. Wysocki Feb. 26, 2019, 11:40 p.m. UTC | #4
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.

Cheers,
Rafael
Ulf Hansson Feb. 27, 2019, 12:07 a.m. UTC | #5
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 mbox series

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];