Messages in this thread | | | Date | Sun, 10 Sep 2023 20:25:01 +0100 | From | Qais Yousef <> | Subject | Re: [RFC PATCH 5/7] sched/schedutil: Add a new tunable to dictate response time |
| |
On 09/07/23 13:44, Peter Zijlstra wrote: > On Mon, Aug 28, 2023 at 12:32:01AM +0100, Qais Yousef wrote: > > +static inline unsigned long > > +sugov_apply_response_time(struct sugov_policy *sg_policy, unsigned long util) > > +{ > > + unsigned long mult; > > + > > + if (sg_policy->freq_response_time_ms == sg_policy->tunables->response_time_ms) > > + return util; > > + > > + mult = sg_policy->freq_response_time_ms * SCHED_CAPACITY_SCALE; > > + mult /= sg_policy->tunables->response_time_ms; > > + mult *= util; > > + > > + return mult >> SCHED_CAPACITY_SHIFT; > > +} > > + > > static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) > > { > > s64 delta_ns; > > @@ -143,6 +184,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy, > > unsigned int freq = arch_scale_freq_invariant() ? > > policy->cpuinfo.max_freq : policy->cur; > > > > + util = sugov_apply_response_time(sg_policy, util); > > freq = map_util_freq(util, freq, max); > > > > if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update) > > Urgh, so instead of caching the multiplier you keep computing what is > essentially a constant over and over and over and over again :/ > > That is, compute the whole 'freq_response_time_ms * SCHED_CAPACITY_SCALE > / response_time_ms' thing *once*, when that file is written to, and then > reduce the whole thing to: > > return (freq_response_mult * util) >> SCHED_CAPACITY_SHIFT; > > No need for that special case, no need for divisions, just go.
Yes! I was too focused am I doing the right thing, I didn't stop to think this is actually a constant and can be done once too. I will fix it if this knobs ends up hanging around.
Thanks!
-- Qais Yousef
| |