On Wed, Dec 9, 2020 at 6:16 AM Viresh Kumar <[email protected]> wrote: > > On 08-12-20, 18:01, Rafael J. Wysocki wrote: > > On Tue, Dec 8, 2020 at 9:52 AM Viresh Kumar <[email protected]> wrote: > > > > > > On 07-12-20, 17:29, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <[email protected]> > > > > > > > > When avoiding reduction of the frequency after the target CPU has > > > > been busy since the previous frequency update, adjust the utilization > > > > instead of adjusting the frequency, because doing so is more prudent > > > > (it is done to counter a possible utilization deficit after all) and > > > > it will allow some code to be shared after a subsequent change. > > > > > > > > Signed-off-by: Rafael J. Wysocki <[email protected]> > > > > --- > > > > kernel/sched/cpufreq_schedutil.c | 11 ++++------- > > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c > > > > =================================================================== > > > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c > > > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c > > > > @@ -437,7 +437,7 @@ static void sugov_update_single(struct u > > > > { > > > > struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, > > > > update_util); > > > > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > > > > - unsigned int cached_freq = sg_policy->cached_raw_freq; > > > > + unsigned long prev_util = sg_cpu->util; > > > > unsigned int next_f; > > > > > > > > sugov_iowait_boost(sg_cpu, time, flags); > > > > @@ -451,17 +451,14 @@ static void sugov_update_single(struct u > > > > sugov_get_util(sg_cpu); > > > > sugov_iowait_apply(sg_cpu, time); > > > > > > > > - next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max); > > > > /* > > > > * Do not reduce the frequency if the CPU has not been idle > > > > * recently, as the reduction is likely to be premature then. > > > > */ > > > > - if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) { > > > > - next_f = sg_policy->next_freq; > > > > + if (sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util) > > > > + sg_cpu->util = prev_util; > > > > > > > > - /* Restore cached freq as next_freq has changed */ > > > > - sg_policy->cached_raw_freq = cached_freq; > > > > - } > > > > + next_f = get_next_freq(sg_policy, sg_cpu->util, sg_cpu->max); > > > > > > I don't think we can replace freq comparison by util, or at least it will > > > give > > > us a different final frequency and the behavior is changed. > > > > > > Lets take an example, lets say current freq is 1 GHz and max is 1024.
Ah, so that's in the freq-dependent case. In the freq-invariant case next_f doesn't depend on the current frequency. > > > Round 1: Lets say util is 1000 > > > > > > next_f = 1GHz * 1.25 * 1000/1024 = 1.2 GHz > > > > > > Round 2: Lets say util has come down to 900 here, > > > > > > before the patch: > > > > > > next_f = 1.2 GHz * 1.25 * 900/1024 = 1.31 GHz > > > > > > after the patch: > > > > > > next_f = 1.2 GHz * 1.25 * 1000/1024 = 1.45 GHz > > > > > > Or did I make a mistake here ? > > > > I think so, if my understanding is correct. > > > > Without the patch, next_f will be reset to the previous value > > (sq_policy->next_freq) if the CPU has been busy and the (new) next_f > > is less than that value. > > > > So the "new" next_f before the patch is 1.31 GHz, but because it is > > less than the previous value (1.45 GHz), it will be reset to that > > value, unless I'm missing something. > > The prev frequency here was 1.2 GHz (after Round 1). 1.45 GHz is the > value we get after this patch, as we take the earlier utilization > (1000) into account instead of 900. So I have misunderstood your example. In the non-invariant case (which is or shortly will be relevant for everybody interested) cpuinfo.max_freq goes into the calculation instead of the current frequency and the mapping between util and freq is linear. In the freq-dependent case it is not linear, of course. So I guess the concern is that this changes the behavior in the freq-dependent case which may not be desirable. Fair enough, but I'm not sure if that is enough of a reason to avoid sharing the code between the "perf" and "freq" paths.
