| Message ID | 1516317.nEvPfS8aZ7@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote: > On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote: >> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote: >>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote: [ ... ] > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: cpuidle / menu: Return (-1) if there are no suitable states > > If there is a PM QoS latency limit and all of the sufficiently shallow > C-states are disabled, the cpuidle menu governor returns 0 which on > some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned > if that C-state has been disabled. > > Fix the issue by modifying the menu governor to return (-1) in such > situations. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/cpuidle/governors/menu.c | 2 +- > include/linux/cpuidle.h | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/cpuidle/governors/menu.c > =================================================================== > --- linux-pm.orig/drivers/cpuidle/governors/menu.c > +++ linux-pm/drivers/cpuidle/governors/menu.c > @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr > data->needs_update = 0; > } > > - data->last_state_idx = 0; > + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select function could return 0 even this one is disabled and this is not what you want to happen, no ? > /* Special case when user has set very strict latency requirement */ > if (unlikely(latency_req == 0)) >
On Friday, May 02, 2014 10:47:48 AM Daniel Lezcano wrote: > On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote: > > On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote: > >> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote: > >>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote: > > [ ... ] > > > --- > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Subject: cpuidle / menu: Return (-1) if there are no suitable states > > > > If there is a PM QoS latency limit and all of the sufficiently shallow > > C-states are disabled, the cpuidle menu governor returns 0 which on > > some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned > > if that C-state has been disabled. > > > > Fix the issue by modifying the menu governor to return (-1) in such > > situations. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/cpuidle/governors/menu.c | 2 +- > > include/linux/cpuidle.h | 2 ++ > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > Index: linux-pm/drivers/cpuidle/governors/menu.c > > =================================================================== > > --- linux-pm.orig/drivers/cpuidle/governors/menu.c > > +++ linux-pm/drivers/cpuidle/governors/menu.c > > @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr > > data->needs_update = 0; > > } > > > > - data->last_state_idx = 0; > > + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; > > In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select > function could return 0 even this one is disabled and this is not what > you want to happen, no ? OK, so that's a choice. We can choose to do the above or to return an error code if the 0 state is disabled too. The above is arguably simpler and matches the idea that 0 is a "fallback" state on x86. Of course, it also is confusing, because user space *can* set "disable" for the 0 state on x86, but that actually has no effect today AFAICS. I'm mostly worried about systems where CPUIDLE_DRIVER_STATE_START is 0 and where menu_select() explicitly checks "disabled" and then it returns 0 anyway if it cannot find any other suitable state. In my opinion that needs to be made consistent, but I don't care too much about which way as long as the change is not too intrusive.
On 05/02/2014 02:20 PM, Rafael J. Wysocki wrote: > On Friday, May 02, 2014 10:47:48 AM Daniel Lezcano wrote: >> On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote: >>> On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote: >>>> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote: >>>>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote: >> >> [ ... ] >> >>> --- >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> Subject: cpuidle / menu: Return (-1) if there are no suitable states >>> >>> If there is a PM QoS latency limit and all of the sufficiently shallow >>> C-states are disabled, the cpuidle menu governor returns 0 which on >>> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned >>> if that C-state has been disabled. >>> >>> Fix the issue by modifying the menu governor to return (-1) in such >>> situations. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >>> drivers/cpuidle/governors/menu.c | 2 +- >>> include/linux/cpuidle.h | 2 ++ >>> 2 files changed, 3 insertions(+), 1 deletion(-) >>> >>> Index: linux-pm/drivers/cpuidle/governors/menu.c >>> =================================================================== >>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c >>> +++ linux-pm/drivers/cpuidle/governors/menu.c >>> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr >>> data->needs_update = 0; >>> } >>> >>> - data->last_state_idx = 0; >>> + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; >> >> In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select >> function could return 0 even this one is disabled and this is not what >> you want to happen, no ? > > OK, so that's a choice. We can choose to do the above or to return an error > code if the 0 state is disabled too. The above is arguably simpler and > matches the idea that 0 is a "fallback" state on x86. > > Of course, it also is confusing, because user space *can* set "disable" for > the 0 state on x86, but that actually has no effect today AFAICS. Yes, the poll state is very rarely selected. Regarding the description of this patch, I think it would make sense to move the duplicate pm qos checks to the cpuidle_idle_call function directly and pass the latency req to the select function, so the zero latency check could be done by the caller before entering select. > I'm mostly worried about systems where CPUIDLE_DRIVER_STATE_START is 0 > and where menu_select() explicitly checks "disabled" and then it returns > 0 anyway if it cannot find any other suitable state. For the ARM platform, the state0 and the default idle function are the same, so disabling this state will result in calling the same idle function. > In my opinion that needs to be made consistent, but I don't care too much about > which way as long as the change is not too intrusive. I think we can live with this patch until we remove the CPUIDLE_DRIVER_STATE_START macro. It was introduced to factor out a couple of drivers and now it results in a confusing-hard-to-fix-code.
On Friday, May 02, 2014 03:19:55 PM Daniel Lezcano wrote: > On 05/02/2014 02:20 PM, Rafael J. Wysocki wrote: > > On Friday, May 02, 2014 10:47:48 AM Daniel Lezcano wrote: > >> On 04/30/2014 01:16 AM, Rafael J. Wysocki wrote: > >>> On Tuesday, April 29, 2014 01:28:03 AM Rafael J. Wysocki wrote: > >>>> On Monday, April 28, 2014 01:14:32 PM Daniel Lezcano wrote: > >>>>> On 04/27/2014 02:55 PM, Rafael J. Wysocki wrote: > >> > >> [ ... ] > >> > >>> --- > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> Subject: cpuidle / menu: Return (-1) if there are no suitable states > >>> > >>> If there is a PM QoS latency limit and all of the sufficiently shallow > >>> C-states are disabled, the cpuidle menu governor returns 0 which on > >>> some systems is CPUIDLE_DRIVER_STATE_START and shouldn't be returned > >>> if that C-state has been disabled. > >>> > >>> Fix the issue by modifying the menu governor to return (-1) in such > >>> situations. > >>> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> --- > >>> drivers/cpuidle/governors/menu.c | 2 +- > >>> include/linux/cpuidle.h | 2 ++ > >>> 2 files changed, 3 insertions(+), 1 deletion(-) > >>> > >>> Index: linux-pm/drivers/cpuidle/governors/menu.c > >>> =================================================================== > >>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c > >>> +++ linux-pm/drivers/cpuidle/governors/menu.c > >>> @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr > >>> data->needs_update = 0; > >>> } > >>> > >>> - data->last_state_idx = 0; > >>> + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; > >> > >> In case of x86, CPUIDLE_DRIVER_STATE_START will be 1, so the select > >> function could return 0 even this one is disabled and this is not what > >> you want to happen, no ? > > > > OK, so that's a choice. We can choose to do the above or to return an error > > code if the 0 state is disabled too. The above is arguably simpler and > > matches the idea that 0 is a "fallback" state on x86. > > > > Of course, it also is confusing, because user space *can* set "disable" for > > the 0 state on x86, but that actually has no effect today AFAICS. > > Yes, the poll state is very rarely selected. > > Regarding the description of this patch, I think it would make sense to > move the duplicate pm qos checks to the cpuidle_idle_call function > directly and pass the latency req to the select function, so the zero > latency check could be done by the caller before entering select. I would prefer to have them in cpuidle_select() for various reasons (one of them being to avoid the need to pass latency_req from cpuidle_idle_call() to cpuidle_select() which isn't necessary). > > I'm mostly worried about systems where CPUIDLE_DRIVER_STATE_START is 0 > > and where menu_select() explicitly checks "disabled" and then it returns > > 0 anyway if it cannot find any other suitable state. > > For the ARM platform, the state0 and the default idle function are the > same, so disabling this state will result in calling the same idle function. > > > In my opinion that needs to be made consistent, but I don't care too much about > > which way as long as the change is not too intrusive. > > I think we can live with this patch until we remove the > CPUIDLE_DRIVER_STATE_START macro. It was introduced to factor out a > couple of drivers and now it results in a confusing-hard-to-fix-code. OK Thanks!
Index: linux-pm/drivers/cpuidle/governors/menu.c =================================================================== --- linux-pm.orig/drivers/cpuidle/governors/menu.c +++ linux-pm/drivers/cpuidle/governors/menu.c @@ -296,7 +296,7 @@ static int menu_select(struct cpuidle_dr data->needs_update = 0; } - data->last_state_idx = 0; + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; /* Special case when user has set very strict latency requirement */ if (unlikely(latency_req == 0))