| Message ID | 4130376.2FFt7p4687@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | cpuidle: menu: Fixes, optimizations and cleanups | expand |
On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: > idx = -1; > - for (i = first_idx; i < drv->state_count; i++) { > + for (i = 0; i < drv->state_count; i++) { > struct cpuidle_state *s = &drv->states[i]; > struct cpuidle_state_usage *su = &dev->states_usage[i]; > > if (s->disabled || su->disable) > continue; > + > if (idx == -1) > idx = i; /* first enabled state */ > + > if (s->target_residency > predicted_us) { > + /* > + * Use a physical idle state, not busy polling, unless > + * a timer is going to trigger really really soon. > + */ > + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && > + i == idx + 1 && latency_req > s->exit_latency && > + data->next_timer_us > max_t(unsigned int, 20, > + s->target_residency)) { Not new in this patch, but this is where I really noticed it; that 20, should that not be something like: POLL_IDLE_TIME_LIMIT / NSEC_PER_USEC ? > + idx = i; > + break; > + } > if (predicted_us < TICK_USEC) > break; > >
On Thu, Oct 4, 2018 at 9:46 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: > > idx = -1; > > - for (i = first_idx; i < drv->state_count; i++) { > > + for (i = 0; i < drv->state_count; i++) { > > struct cpuidle_state *s = &drv->states[i]; > > struct cpuidle_state_usage *su = &dev->states_usage[i]; > > > > if (s->disabled || su->disable) > > continue; > > + > > if (idx == -1) > > idx = i; /* first enabled state */ > > + > > if (s->target_residency > predicted_us) { > > + /* > > + * Use a physical idle state, not busy polling, unless > > + * a timer is going to trigger really really soon. > > + */ > > + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && > > + i == idx + 1 && latency_req > s->exit_latency && > > + data->next_timer_us > max_t(unsigned int, 20, > > + s->target_residency)) { > > Not new in this patch, but this is where I really noticed it; that 20, > should that not be something like: POLL_IDLE_TIME_LIMIT / NSEC_PER_USEC > ? The POLL_IDLE_TIME_LIMIT is how much time we allow it to spin in idle_poll() and I'm not sure it is related. Besides, I want it to go away actually (https://patchwork.kernel.org/patch/10624117/). I could use a separate symbol for this particular magic number, but it has been magic forever and it is used just in this one place, so ...
On Thu, Oct 04, 2018 at 09:53:39AM +0200, Rafael J. Wysocki wrote: > On Thu, Oct 4, 2018 at 9:46 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: > > > idx = -1; > > > - for (i = first_idx; i < drv->state_count; i++) { > > > + for (i = 0; i < drv->state_count; i++) { > > > struct cpuidle_state *s = &drv->states[i]; > > > struct cpuidle_state_usage *su = &dev->states_usage[i]; > > > > > > if (s->disabled || su->disable) > > > continue; > > > + > > > if (idx == -1) > > > idx = i; /* first enabled state */ > > > + > > > if (s->target_residency > predicted_us) { > > > + /* > > > + * Use a physical idle state, not busy polling, unless > > > + * a timer is going to trigger really really soon. > > > + */ > > > + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && > > > + i == idx + 1 && latency_req > s->exit_latency && > > > + data->next_timer_us > max_t(unsigned int, 20, > > > + s->target_residency)) { > > > > Not new in this patch, but this is where I really noticed it; that 20, > > should that not be something like: POLL_IDLE_TIME_LIMIT / NSEC_PER_USEC > > ? > > The POLL_IDLE_TIME_LIMIT is how much time we allow it to spin in > idle_poll() and I'm not sure it is related. Besides, I want it to go > away actually (https://patchwork.kernel.org/patch/10624117/). Ah, ok. Making it go away is better still!
On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rearrange the code in menu_select() so that the loop over idle states > always starts from 0 and get rid of the first_idx variable. > > While at it, add two empty lines to separate conditional statements > one another. > > No intentional behavior changes. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- This code is becoming a bit complex to follow :/ May be I missed something, but it is not possible to enter the condition without idx != 0, no ? (I meant the condition if ((drv->states[idx].flags & FLAG_POLLING)) Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > drivers/cpuidle/governors/menu.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > Index: linux-pm/drivers/cpuidle/governors/menu.c > =================================================================== > --- linux-pm.orig/drivers/cpuidle/governors/menu.c > +++ linux-pm/drivers/cpuidle/governors/menu.c > @@ -285,7 +285,6 @@ static int menu_select(struct cpuidle_dr > struct menu_device *data = this_cpu_ptr(&menu_devices); > int latency_req = cpuidle_governor_latency_req(dev->cpu); > int i; > - int first_idx; > int idx; > unsigned int interactivity_req; > unsigned int expected_interval; > @@ -348,36 +347,33 @@ static int menu_select(struct cpuidle_dr > latency_req = interactivity_req; > } > > - first_idx = 0; > - if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) { > - struct cpuidle_state *s = &drv->states[1]; > - unsigned int polling_threshold; > - > - /* > - * Default to a physical idle state, not to busy polling, unless > - * a timer is going to trigger really really soon. > - */ > - polling_threshold = max_t(unsigned int, 20, s->target_residency); > - if (data->next_timer_us > polling_threshold && > - latency_req > s->exit_latency && !s->disabled && > - !dev->states_usage[1].disable) > - first_idx = 1; > - } > - > /* > * Find the idle state with the lowest power while satisfying > * our constraints. > */ > idx = -1; > - for (i = first_idx; i < drv->state_count; i++) { > + for (i = 0; i < drv->state_count; i++) { > struct cpuidle_state *s = &drv->states[i]; > struct cpuidle_state_usage *su = &dev->states_usage[i]; > > if (s->disabled || su->disable) > continue; > + > if (idx == -1) > idx = i; /* first enabled state */ > + > if (s->target_residency > predicted_us) { > + /* > + * Use a physical idle state, not busy polling, unless > + * a timer is going to trigger really really soon. > + */ > + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && > + i == idx + 1 && latency_req > s->exit_latency && > + data->next_timer_us > max_t(unsigned int, 20, > + s->target_residency)) { > + idx = i; > + break; > + } > if (predicted_us < TICK_USEC) > break; > >
On Thu, Oct 4, 2018 at 4:51 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Rearrange the code in menu_select() so that the loop over idle states > > always starts from 0 and get rid of the first_idx variable. > > > > While at it, add two empty lines to separate conditional statements > > one another. > > > > No intentional behavior changes. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > This code is becoming a bit complex to follow :/ > > May be I missed something, but it is not possible to enter the condition without > idx != 0, no ? (I meant the condition if ((drv->states[idx].flags & > FLAG_POLLING)) Not sure what you mean. We start with idx = -1, i = 0. If state[0] is enabled, idx becomes 0. If the target residency or exit latency of state[0] are already beyond the limits, we just bail out and state[0] will be returned. If not, we go to i = 1, but idx is still 0. If the target residency of state[1] is beyond predicted_us (which is the interesting case), we check (drv->states[0].flags & FLAG_POLLING) and so on. Currently, the polling state must be state[0] (if used at all) anyway. > Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Thanks!
On 04/10/2018 19:19, Rafael J. Wysocki wrote: > On Thu, Oct 4, 2018 at 4:51 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> Rearrange the code in menu_select() so that the loop over idle states >>> always starts from 0 and get rid of the first_idx variable. >>> >>> While at it, add two empty lines to separate conditional statements >>> one another. >>> >>> No intentional behavior changes. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >> >> This code is becoming a bit complex to follow :/ >> >> May be I missed something, but it is not possible to enter the condition without >> idx != 0, no ? (I meant the condition if ((drv->states[idx].flags & >> FLAG_POLLING)) > > Not sure what you mean. Yes, sorry let me clarify. I meant the flag polling is always on state[0], so if the flag is set then idx == 0. We have the conditions: drv->states[idx].flags & CPUIDLE_FLAG_POLLING If it is true then idx is zero. Then comes the second condition: i == idx + 1 because of the above, idx is zero, then it can become i == 1. Then the variable assignation: idx = i can be replaced by idx = 1 > We start with idx = -1, i = 0. If state[0] is enabled, idx becomes 0. > > If the target residency or exit latency of state[0] are already beyond > the limits, we just bail out and state[0] will be returned. > > If not, we go to i = 1, but idx is still 0. If the target residency > of state[1] is beyond predicted_us (which is the interesting case), we > check (drv->states[0].flags & FLAG_POLLING) and so on. > > Currently, the polling state must be state[0] (if used at all) anyway. > >> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> > > Thanks! >
On Fri, Oct 5, 2018 at 10:35 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 04/10/2018 19:19, Rafael J. Wysocki wrote: > > On Thu, Oct 4, 2018 at 4:51 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> > >> On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote: > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> > >>> Rearrange the code in menu_select() so that the loop over idle states > >>> always starts from 0 and get rid of the first_idx variable. > >>> > >>> While at it, add two empty lines to separate conditional statements > >>> one another. > >>> > >>> No intentional behavior changes. > >>> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> --- > >> > >> This code is becoming a bit complex to follow :/ > >> > >> May be I missed something, but it is not possible to enter the condition without > >> idx != 0, no ? (I meant the condition if ((drv->states[idx].flags & > >> FLAG_POLLING)) > > > > Not sure what you mean. > > Yes, sorry let me clarify. > > I meant the flag polling is always on state[0], so if the flag is set > then idx == 0. > > We have the conditions: > > drv->states[idx].flags & CPUIDLE_FLAG_POLLING > > If it is true then idx is zero. > > > Then comes the second condition: > > i == idx + 1 > > because of the above, idx is zero, then it can become i == 1. > > Then the variable assignation: > > idx = i can be replaced by idx = 1 Yes, but the former works too. :-)
Index: linux-pm/drivers/cpuidle/governors/menu.c =================================================================== --- linux-pm.orig/drivers/cpuidle/governors/menu.c +++ linux-pm/drivers/cpuidle/governors/menu.c @@ -285,7 +285,6 @@ static int menu_select(struct cpuidle_dr struct menu_device *data = this_cpu_ptr(&menu_devices); int latency_req = cpuidle_governor_latency_req(dev->cpu); int i; - int first_idx; int idx; unsigned int interactivity_req; unsigned int expected_interval; @@ -348,36 +347,33 @@ static int menu_select(struct cpuidle_dr latency_req = interactivity_req; } - first_idx = 0; - if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) { - struct cpuidle_state *s = &drv->states[1]; - unsigned int polling_threshold; - - /* - * Default to a physical idle state, not to busy polling, unless - * a timer is going to trigger really really soon. - */ - polling_threshold = max_t(unsigned int, 20, s->target_residency); - if (data->next_timer_us > polling_threshold && - latency_req > s->exit_latency && !s->disabled && - !dev->states_usage[1].disable) - first_idx = 1; - } - /* * Find the idle state with the lowest power while satisfying * our constraints. */ idx = -1; - for (i = first_idx; i < drv->state_count; i++) { + for (i = 0; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i]; if (s->disabled || su->disable) continue; + if (idx == -1) idx = i; /* first enabled state */ + if (s->target_residency > predicted_us) { + /* + * Use a physical idle state, not busy polling, unless + * a timer is going to trigger really really soon. + */ + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) && + i == idx + 1 && latency_req > s->exit_latency && + data->next_timer_us > max_t(unsigned int, 20, + s->target_residency)) { + idx = i; + break; + } if (predicted_us < TICK_USEC) break;