| Message ID | 1464231181-30741-4-git-send-email-smuckle@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 25-05-16, 19:53, Steve Muckle wrote: > The slow-path frequency transition path is relatively expensive as it > requires waking up a thread to do work. Should support be added for > remote CPU cpufreq updates that is also expensive since it requires an > IPI. These activities should be avoided if they are not necessary. > > To that end, calculate the actual driver-supported frequency required by > the new utilization value in schedutil by using the recently added > cpufreq_driver_resolve_freq callback. If it is the same as the > previously requested driver frequency then there is no need to continue > with the update assuming the cpu frequency limits have not changed. This > will have additional benefits should the semantics of the rate limit be > changed to apply solely to frequency transitions rather than to > frequency calculations in schedutil. Maybe mention here that this patch isn't avoiding those IPIs yet, I got an impression earlier that they are avoided with it. > The last raw required frequency is cached. This allows the driver > frequency lookup to be skipped in the event that the new raw required > frequency matches the last one, assuming a frequency update has not been > forced due to limits changing (indicated by a next_freq value of > UINT_MAX, see sugov_should_update_freq). I am not sure this is required, see below.. > Signed-off-by: Steve Muckle <smuckle@linaro.org> > --- > kernel/sched/cpufreq_schedutil.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 154ae3a51e86..ef73ca4d8d78 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -45,6 +45,8 @@ struct sugov_cpu { > struct update_util_data update_util; > struct sugov_policy *sg_policy; > > + unsigned int cached_raw_freq; > + > /* The fields below are only needed when sharing a policy. */ > unsigned long util; > unsigned long max; > @@ -104,7 +106,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, > > /** > * get_next_freq - Compute a new frequency for a given cpufreq policy. > - * @policy: cpufreq policy object to compute the new frequency for. > + * @sg_cpu: schedutil cpu object to compute the new frequency for. > * @util: Current CPU utilization. > * @max: CPU capacity. > * > @@ -119,14 +121,24 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, > * next_freq = C * curr_freq * util_raw / max > * > * Take C = 1.25 for the frequency tipping point at (util / max) = 0.8. > + * > + * The lowest driver-supported frequency which is equal or greater than the raw > + * next_freq (as calculated above) is returned. > */ > -static unsigned int get_next_freq(struct cpufreq_policy *policy, > - unsigned long util, unsigned long max) > +static unsigned int get_next_freq(struct sugov_cpu *sg_cpu, unsigned long util, > + unsigned long max) > { > + struct sugov_policy *sg_policy = sg_cpu->sg_policy; > + struct cpufreq_policy *policy = sg_policy->policy; > unsigned int freq = arch_scale_freq_invariant() ? > policy->cpuinfo.max_freq : policy->cur; > > - return (freq + (freq >> 2)) * util / max; > + freq = (freq + (freq >> 2)) * util / max; > + > + if (freq == sg_cpu->cached_raw_freq && sg_policy->next_freq != UINT_MAX) > + return sg_policy->next_freq; I am not sure what the benefit of the second comparison to UINT_MAX is. The output of below code will be same if the freq was == cached_raw_freq, no matter what. I also have a doubt (I am quite sure Rafael will have a reason for that, which I am failing to understand now), on why we are doing next_freq == UINT_MAX in sugov_should_update_freq(). I understand that because the limits might have changed, need_freq_update would have been set to true. We should evaluate next-freq again without worrying about the load or the time since last evaluation. But what will happen by forcefully calling the cpufreq routines to change the frequency, if next_freq hasn't changed even after limits updates? Wouldn't that call always return early because the new freq and the current freq are going to be same ? @Rafael: Sorry for asking this so late :(
2016-05-26 10:53 GMT+08:00 Steve Muckle <steve.muckle@linaro.org>: > The slow-path frequency transition path is relatively expensive as it > requires waking up a thread to do work. Should support be added for > remote CPU cpufreq updates that is also expensive since it requires an > IPI. These activities should be avoided if they are not necessary. > > To that end, calculate the actual driver-supported frequency required by > the new utilization value in schedutil by using the recently added > cpufreq_driver_resolve_freq callback. If it is the same as the > previously requested driver frequency then there is no need to continue > with the update assuming the cpu frequency limits have not changed. This > will have additional benefits should the semantics of the rate limit be > changed to apply solely to frequency transitions rather than to > frequency calculations in schedutil. sugov_should_update_freq() still be called before get_nex_freq() after the patch applied, so rate limit still apply to both frequency transitions and frequency calculations, right? Regards, Wanpeng Li -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 26, 2016 at 9:16 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 25-05-16, 19:53, Steve Muckle wrote: >> The slow-path frequency transition path is relatively expensive as it >> requires waking up a thread to do work. Should support be added for >> remote CPU cpufreq updates that is also expensive since it requires an >> IPI. These activities should be avoided if they are not necessary. >> >> To that end, calculate the actual driver-supported frequency required by >> the new utilization value in schedutil by using the recently added >> cpufreq_driver_resolve_freq callback. If it is the same as the >> previously requested driver frequency then there is no need to continue >> with the update assuming the cpu frequency limits have not changed. This >> will have additional benefits should the semantics of the rate limit be >> changed to apply solely to frequency transitions rather than to >> frequency calculations in schedutil. [cut] > I also have a doubt (I am quite sure Rafael will have a reason for > that, which I am failing to understand now), on why we are doing > next_freq == UINT_MAX in sugov_should_update_freq(). > > I understand that because the limits might have changed, > need_freq_update would have been set to true. We should evaluate > next-freq again without worrying about the load or the time since last > evaluation. This is in response to the "limits" event (or to the ->limits call after my recent patches). That event basically means "something has changed, so if you have cached anything, invalidate it" to the governor. Accordingly, it invalidates next_freq, because that's a cached value. > But what will happen by forcefully calling the cpufreq routines to > change the frequency, if next_freq hasn't changed even after limits > updates? I can't really parse the above question, so I'm not going to try to answer it. :-) > Wouldn't that call always return early because the new freq > and the current freq are going to be same ? > > @Rafael: Sorry for asking this so late :( It is not too late. If there's a problem somewhere, it needs to be fixed, but at this point I have no idea what you are asking about. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 26, 2016 at 12:46:29PM +0530, Viresh Kumar wrote: > On 25-05-16, 19:53, Steve Muckle wrote: > > The slow-path frequency transition path is relatively expensive as it > > requires waking up a thread to do work. Should support be added for > > remote CPU cpufreq updates that is also expensive since it requires an > > IPI. These activities should be avoided if they are not necessary. > > > > To that end, calculate the actual driver-supported frequency required by > > the new utilization value in schedutil by using the recently added > > cpufreq_driver_resolve_freq callback. If it is the same as the > > previously requested driver frequency then there is no need to continue > > with the update assuming the cpu frequency limits have not changed. This > > will have additional benefits should the semantics of the rate limit be > > changed to apply solely to frequency transitions rather than to > > frequency calculations in schedutil. > > Maybe mention here that this patch isn't avoiding those IPIs yet, I > got an impression earlier that they are avoided with it. Perhaps the problem is that my cover letter should have more clearly specified that the earlier referenced series was an unaccepted draft? I'll be more careful to note that in the future. The text above (the second sentence) seems okay to me in that it mentions remote updates are not currently supported. Let me know if there is specific text you would like included. thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 01:41:02PM +0800, Wanpeng Li wrote: > 2016-05-26 10:53 GMT+08:00 Steve Muckle <steve.muckle@linaro.org>: > > The slow-path frequency transition path is relatively expensive as it > > requires waking up a thread to do work. Should support be added for > > remote CPU cpufreq updates that is also expensive since it requires an > > IPI. These activities should be avoided if they are not necessary. > > > > To that end, calculate the actual driver-supported frequency required by > > the new utilization value in schedutil by using the recently added > > cpufreq_driver_resolve_freq callback. If it is the same as the > > previously requested driver frequency then there is no need to continue > > with the update assuming the cpu frequency limits have not changed. This > > will have additional benefits should the semantics of the rate limit be > > changed to apply solely to frequency transitions rather than to > > frequency calculations in schedutil. > > sugov_should_update_freq() still be called before get_nex_freq() after > the patch applied, so rate limit still apply to both frequency > transitions and frequency calculations, right? Yes this series does not change the semantics of the rate limit. It only includes the changes required for resolving raw target frequencies to driver-supported frequencies so redundant operations can be avoided. FWIW the approach I took to change the rate_limit semantics [0] just moves where last_freq_update_time is set. I was going to return to that once these changes are complete. [0]: https://lkml.org/lkml/2016/5/9/857 thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30-05-16, 09:35, Steve Muckle wrote: > The text above (the second sentence) seems okay to me in that it > mentions remote updates are not currently supported. Let me know if > there is specific text you would like included. Perhaps I got confused between cover-letter and this log. Leave that. Its fine.
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 154ae3a51e86..ef73ca4d8d78 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -45,6 +45,8 @@ struct sugov_cpu { struct update_util_data update_util; struct sugov_policy *sg_policy; + unsigned int cached_raw_freq; + /* The fields below are only needed when sharing a policy. */ unsigned long util; unsigned long max; @@ -104,7 +106,7 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, /** * get_next_freq - Compute a new frequency for a given cpufreq policy. - * @policy: cpufreq policy object to compute the new frequency for. + * @sg_cpu: schedutil cpu object to compute the new frequency for. * @util: Current CPU utilization. * @max: CPU capacity. * @@ -119,14 +121,24 @@ static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, * next_freq = C * curr_freq * util_raw / max * * Take C = 1.25 for the frequency tipping point at (util / max) = 0.8. + * + * The lowest driver-supported frequency which is equal or greater than the raw + * next_freq (as calculated above) is returned. */ -static unsigned int get_next_freq(struct cpufreq_policy *policy, - unsigned long util, unsigned long max) +static unsigned int get_next_freq(struct sugov_cpu *sg_cpu, unsigned long util, + unsigned long max) { + struct sugov_policy *sg_policy = sg_cpu->sg_policy; + struct cpufreq_policy *policy = sg_policy->policy; unsigned int freq = arch_scale_freq_invariant() ? policy->cpuinfo.max_freq : policy->cur; - return (freq + (freq >> 2)) * util / max; + freq = (freq + (freq >> 2)) * util / max; + + if (freq == sg_cpu->cached_raw_freq && sg_policy->next_freq != UINT_MAX) + return sg_policy->next_freq; + sg_cpu->cached_raw_freq = freq; + return cpufreq_driver_resolve_freq(policy, freq); } static void sugov_update_single(struct update_util_data *hook, u64 time, @@ -141,13 +153,14 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, return; next_f = util == ULONG_MAX ? policy->cpuinfo.max_freq : - get_next_freq(policy, util, max); + get_next_freq(sg_cpu, util, max); sugov_update_commit(sg_policy, time, next_f); } -static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy, +static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, unsigned long util, unsigned long max) { + struct sugov_policy *sg_policy = sg_cpu->sg_policy; struct cpufreq_policy *policy = sg_policy->policy; unsigned int max_f = policy->cpuinfo.max_freq; u64 last_freq_update_time = sg_policy->last_freq_update_time; @@ -187,7 +200,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy, } } - return get_next_freq(policy, util, max); + return get_next_freq(sg_cpu, util, max); } static void sugov_update_shared(struct update_util_data *hook, u64 time, @@ -204,7 +217,7 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, sg_cpu->last_update = time; if (sugov_should_update_freq(sg_policy, time)) { - next_f = sugov_next_freq_shared(sg_policy, util, max); + next_f = sugov_next_freq_shared(sg_cpu, util, max); sugov_update_commit(sg_policy, time, next_f); } @@ -432,6 +445,7 @@ static int sugov_start(struct cpufreq_policy *policy) sg_cpu->util = ULONG_MAX; sg_cpu->max = 0; sg_cpu->last_update = 0; + sg_cpu->cached_raw_freq = 0; cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util, sugov_update_shared); } else {
The slow-path frequency transition path is relatively expensive as it requires waking up a thread to do work. Should support be added for remote CPU cpufreq updates that is also expensive since it requires an IPI. These activities should be avoided if they are not necessary. To that end, calculate the actual driver-supported frequency required by the new utilization value in schedutil by using the recently added cpufreq_driver_resolve_freq callback. If it is the same as the previously requested driver frequency then there is no need to continue with the update assuming the cpu frequency limits have not changed. This will have additional benefits should the semantics of the rate limit be changed to apply solely to frequency transitions rather than to frequency calculations in schedutil. The last raw required frequency is cached. This allows the driver frequency lookup to be skipped in the event that the new raw required frequency matches the last one, assuming a frequency update has not been forced due to limits changing (indicated by a next_freq value of UINT_MAX, see sugov_should_update_freq). Signed-off-by: Steve Muckle <smuckle@linaro.org> --- kernel/sched/cpufreq_schedutil.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)