| Message ID | 20190226145415.19411-8-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PM / Domains: Support hierarchical CPU arrangement (PSCI/ARM) | expand |
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.
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.
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
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
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
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];
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(-)