| Message ID | 7927358.NyiUUSuA9g@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | cpuidle: Take possible negative "sleep length" values into account | expand |
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; + } 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; } }
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.
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; } }