On Wed, May 9, 2018 at 11:30 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 09-05-18, 11:23, Rafael J. Wysocki wrote:
On Wed, May 9, 2018 at 11:15 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 09-05-18, 10:56, Rafael J. Wysocki wrote:
I'm kind of concerned about updating the limits via sysfs in which case the cached next frequency may be out of range, so it's better to invalidate it right away then.
That should not be a problem as __cpufreq_driver_target() will anyway clamp the target frequency to be within limits, whatever the cached value of next_freq is.
The fast switch case doesn't use it, though.
cpufreq_driver_fast_switch() does the same clamping :)
And we aren't invalidating the cached next freq immediately currently as well, as we are waiting until the next time the util update handler is called to set sg_policy->next_freq to UINT_MAX.
What else do you have in mind to solve this problem ?
Something like the below?
kernel/sched/cpufreq_schedutil.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/kernel/sched/cpufreq_schedutil.c
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c +++ linux-pm/kernel/sched/cpufreq_schedutil.c @@ -305,7 +305,8 @@ static void sugov_update_single(struct u * Do not reduce the frequency if the CPU has not been idle * recently, as the reduction is likely to be premature then. */
- if (busy && next_f < sg_policy->next_freq) {
if (busy && next_f < sg_policy->next_freq &&
sg_policy->next_freq != UINT_MAX) { next_f = sg_policy->next_freq; /* Reset cached freq as next_freq has changed */This will fix the problem we have identified currently, but adding a special meaning to next_freq == UINT_MAX invites more hidden corner cases like the one we just found. IMHO, using next_freq only for the *real* frequency values makes its usage more transparent and readable. And we already have the need_freq_update flag which we can use for this special purpose, as is done in my patch.
So I prefer to do the above as a -stable fix and make the UNIT_MAX change on top of that.
Okay, that's fine with me. Will send the next version now :)
Just to make sure, you are fine with the "Fixes" tag now (since you objected to that earlier) ?
OK, so to be clear, I'm going to queue up the simple patch I posted with a FIxes: tag. I'll resend it with a changelog shortly.
Then please send a UINT_MAX change on top of that and it won't be an urgent fix any more.