| Message ID | cover.1497002895.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | cpufreq: schedutil: Fix 4.12 regressions | expand |
Hi, On Fri, Jun 9, 2017 at 12:15 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Hi Rafael, > > I have identified some regressions with the schedutil governor which > happen due to one of your patches that got merged in 4.12-rc1. > > This series fixes all the drivers which provide a ->target_index() > callback but doesn't fix the drivers which provide ->target() callback. > > Such platforms need to implement the ->resolve_freq() callback in order > to get this fixed and I only had hardware for testing intel_pstate, > which I fixed in this series. > > I am wondering if there is another way to fix this issue (than what I > tried) or if we should revert the offending commit (39b64aa1c007) and > look for other solutions. To my eyes, patch [1/3] fixes the problem and then the remaining ones deal with the issues resulting from that. I'd rather revert and revisit at this point. Thanks, Rafael
On 9 June 2017 at 17:54, Rafael J. Wysocki <rafael@kernel.org> wrote: > Hi, > > On Fri, Jun 9, 2017 at 12:15 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> Hi Rafael, >> >> I have identified some regressions with the schedutil governor which >> happen due to one of your patches that got merged in 4.12-rc1. >> >> This series fixes all the drivers which provide a ->target_index() >> callback but doesn't fix the drivers which provide ->target() callback. >> >> Such platforms need to implement the ->resolve_freq() callback in order >> to get this fixed and I only had hardware for testing intel_pstate, >> which I fixed in this series. >> >> I am wondering if there is another way to fix this issue (than what I >> tried) or if we should revert the offending commit (39b64aa1c007) and >> look for other solutions. > > To my eyes, patch [1/3] fixes the problem and then the remaining ones > deal with the issues resulting from that. So I saw the issue reported and fixed by 2/3 first and noticed 1/3 while doing code reviews. So, 1/3 isn't the culprit really as the problem happens without it as well. -- viresh
On Fri, Jun 9, 2017 at 2:32 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 9 June 2017 at 17:54, Rafael J. Wysocki <rafael@kernel.org> wrote: >> Hi, >> >> On Fri, Jun 9, 2017 at 12:15 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >>> Hi Rafael, >>> >>> I have identified some regressions with the schedutil governor which >>> happen due to one of your patches that got merged in 4.12-rc1. >>> >>> This series fixes all the drivers which provide a ->target_index() >>> callback but doesn't fix the drivers which provide ->target() callback. >>> >>> Such platforms need to implement the ->resolve_freq() callback in order >>> to get this fixed and I only had hardware for testing intel_pstate, >>> which I fixed in this series. >>> >>> I am wondering if there is another way to fix this issue (than what I >>> tried) or if we should revert the offending commit (39b64aa1c007) and >>> look for other solutions. >> >> To my eyes, patch [1/3] fixes the problem and then the remaining ones >> deal with the issues resulting from that. > > So I saw the issue reported and fixed by 2/3 first and noticed 1/3 while > doing code reviews. So, 1/3 isn't the culprit really as the problem happens > without it as well. I see. OK, let me consider that. Thanks, Rafael
On Fri, Jun 9, 2017 at 3:15 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > When the schedutil governor calls cpufreq_driver_resolve_freq() for the > intel_pstate (in passive mode) driver, it simply returns the requested > frequency as there is no ->resolve_freq() callback provided. > > The result is that get_next_freq() doesn't get a chance to know the > frequency which will be set eventually and we can hit a potential > regression as explained in the following paragraph. > > For example, consider the possible range of frequencies as 900 MHz, 1 > GHz, 1.1 GHz, and 1.2 GHz. If the current frequency is 1.1 GHz and the > next frequency (based on current utilization) is 1 GHz, then the > schedutil governor will try to set the average of these as the next > frequency (i.e. 1.05 GHz). > > Because we always try to find the lowest frequency greater than equal to > the target frequency, the intel_pstate driver will end up setting the > frequency as 1.1 GHz. > > Though the sg_policy->next_freq field gets updated with the average > frequency only. And so we will finally select the min frequency when the > next_freq is 1 more than the min frequency as the average then will be > equal to the min frequency. But that will also take lots of iterations > of the schedutil update callbacks. > > Fix that by providing a resolve_freq() callback. > > Tested on desktop with Intel Skylake processors. > > Fixes: 39b64aa1c007 ("cpufreq: schedutil: Reduce frequencies slower") > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/intel_pstate.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index 029a93bfb558..e177352180c3 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -2213,6 +2213,19 @@ static int intel_cpufreq_target(struct cpufreq_policy *policy, > return 0; > } > > +unsigned int intel_cpufreq_resolve_freq(struct cpufreq_policy *policy, > + unsigned int target_freq) Should be defined as static? Thanks, Joel
On 10-06-17, 02:26, Joel Fernandes wrote: > On Fri, Jun 9, 2017 at 3:15 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > When the schedutil governor calls cpufreq_driver_resolve_freq() for the > > intel_pstate (in passive mode) driver, it simply returns the requested > > frequency as there is no ->resolve_freq() callback provided. > > > > The result is that get_next_freq() doesn't get a chance to know the > > frequency which will be set eventually and we can hit a potential > > regression as explained in the following paragraph. > > > > For example, consider the possible range of frequencies as 900 MHz, 1 > > GHz, 1.1 GHz, and 1.2 GHz. If the current frequency is 1.1 GHz and the > > next frequency (based on current utilization) is 1 GHz, then the > > schedutil governor will try to set the average of these as the next > > frequency (i.e. 1.05 GHz). > > > > Because we always try to find the lowest frequency greater than equal to > > the target frequency, the intel_pstate driver will end up setting the > > frequency as 1.1 GHz. > > > > Though the sg_policy->next_freq field gets updated with the average > > frequency only. And so we will finally select the min frequency when the > > next_freq is 1 more than the min frequency as the average then will be > > equal to the min frequency. But that will also take lots of iterations > > of the schedutil update callbacks. > > > > Fix that by providing a resolve_freq() callback. > > > > Tested on desktop with Intel Skylake processors. > > > > Fixes: 39b64aa1c007 ("cpufreq: schedutil: Reduce frequencies slower") > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/cpufreq/intel_pstate.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > > index 029a93bfb558..e177352180c3 100644 > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -2213,6 +2213,19 @@ static int intel_cpufreq_target(struct cpufreq_policy *policy, > > return 0; > > } > > > > +unsigned int intel_cpufreq_resolve_freq(struct cpufreq_policy *policy, > > + unsigned int target_freq) > > Should be defined as static? Yes. -- viresh