| Message ID | 2764850.e9J7NaK4W3@kreacher |
|---|---|
| Headers | show |
| Series | cpuidle: Take possible negative "sleep length" values into account | expand |
On Tue, Mar 30, 2021 at 4:00 AM Zhou Ti (x2019cwm) <x2019cwm@stfx.ca> wrote: > > On Mon 2021-03-29 14:37 Rafael J. Wysocki wrote: > > Make the menu governor check the tick_nohz_get_next_hrtimer() > > return value so as to avoid dealing with negative "sleep length" > > values and make it use that value directly when the tick is stopped. > > > > While at it, rename local variable delta_next in menu_select() to > > delta_tick which better reflects its purpose. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/cpuidle/governors/menu.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > Index: linux-pm/drivers/cpuidle/governors/menu.c > > =================================================================== > > --- linux-pm.orig/drivers/cpuidle/governors/menu.c > > +++ linux-pm/drivers/cpuidle/governors/menu.c > > @@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr > > u64 predicted_ns; > > u64 interactivity_req; > > unsigned long nr_iowaiters; > > - ktime_t delta_next; > > + ktime_t delta, delta_tick; > > int i, idx; > > > > if (data->needs_update) { > > @@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr > > } > > > > /* determine the expected residency time, round up */ > > - data->next_timer_ns = tick_nohz_get_sleep_length(&delta_next); > > + delta = tick_nohz_get_sleep_length(&delta_tick); > > + if (unlikely(delta < 0)) { > > + delta = 0; > > + delta_tick = 0; > > + } > > + data->next_timer_ns = delta; > > > > nr_iowaiters = nr_iowait_cpu(dev->cpu); > > data->bucket = which_bucket(data->next_timer_ns, nr_iowaiters); > > @@ -318,7 +323,7 @@ static int menu_select(struct cpuidle_dr > > * state selection. > > */ > > if (predicted_ns < TICK_NSEC) > > - predicted_ns = delta_next; > > + predicted_ns = data->next_timer_ns; > > } else { > > /* > > * Use the performance multiplier and the user-configurable > > @@ -377,7 +382,7 @@ static int menu_select(struct cpuidle_dr > > * stuck in the shallow one for too long. > > */ > > if (drv->states[idx].target_residency_ns < TICK_NSEC && > > - s->target_residency_ns <= delta_next) > > + s->target_residency_ns <= delta_tick) > > idx = i; > > > > return idx; > > @@ -399,7 +404,7 @@ static int menu_select(struct cpuidle_dr > > predicted_ns < TICK_NSEC) && !tick_nohz_tick_stopped()) { > > *stop_tick = false; > > > > - if (idx > 0 && drv->states[idx].target_residency_ns > delta_next) { > > + if (idx > 0 && drv->states[idx].target_residency_ns > delta_tick) { > > /* > > * The tick is not going to be stopped and the target > > * residency of the state to be returned is not within > > @@ -411,7 +416,7 @@ static int menu_select(struct cpuidle_dr > > continue; > > > > idx = i; > > - if (drv->states[i].target_residency_ns <= delta_next) > > + if (drv->states[i].target_residency_ns <= delta_tick) > > break; > > } > > } > > How about this. > I think it's possible to avoid the new variable delta. > > --- > > --- linux-pm/drivers/cpuidle/governors/menu.c.orig 2021-03-29 22:44:02.316971970 -0300 > +++ linux-pm/drivers/cpuidle/governors/menu.c 2021-03-29 22:51:15.804377168 -0300 > @@ -271,7 +271,7 @@ static int menu_select(struct cpuidle_dr > u64 predicted_ns; > u64 interactivity_req; > unsigned long nr_iowaiters; > - ktime_t delta_next; > + ktime_t delta_tick; > int i, idx; > > if (data->needs_update) { > @@ -280,7 +280,12 @@ static int menu_select(struct cpuidle_dr > } > > /* determine the expected residency time, round up */ > - data->next_timer_ns = tick_nohz_get_sleep_length(&delta_next); > + data->next_timer_ns = tick_nohz_get_sleep_length(&delta_tick); > + > + if (unlikely(data->next_timer_ns >> 63)) { > + data->next_timer_ns = 0; > + delta_tick = 0; > + } Well, not really. Using a new local var is cleaner IMO.
On Mon, Mar 29, 2021 at 8:38 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > Hi All, > > As follows from the discussion triggered by the patch at > > https://lore.kernel.org/lkml/20210311123708.23501-2-frederic@kernel.org/ > > the cpuidle governors using tick_nohz_get_sleep_length() assume it to always > return positive values which is not correct in general. > > To address this issues, first document the fact that negative values can > be returned by tick_nohz_get_sleep_length() (patch [1/5]). Then, in > preparation for more substantial changes, change the data type of two > fields in struct cpuidle_state to s64 so they can be used in computations > involving negative numbers safely (patch [2/5]). > > Next, adjust the teo governor a bit so that negative "sleep length" values > are counted like zero by it (patch [3/5]) and modify it so as to avoid > mishandling negative "sleep length" values (patch [4/5]). > > Finally, make the menu governor take negative "sleep length" values into > account properly (patch [5/5]). > > Please see the changelogs of the patches for details. Given no objections or concerns regarding this lot, let me queue it up. Thanks!