Re: [PATCH v3 1/6] cpufreq: schedutil: reset sg_cpus's flags at IDLE enter

[Viresh Kumar]

On 12-12-17, 14:38, Juri Lelli wrote:
> Hi Viresh,
> 
> On 12/12/17 17:07, Viresh Kumar wrote:
> 
> [...]
> 
> > From: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Date: Tue, 12 Dec 2017 15:43:26 +0530
> > Subject: [PATCH] sched: Keep track of cpufreq utilization update flags
> > 
> > Currently the schedutil governor overwrites the sg_cpu->flags field on
> > every call to the utilization handler. It was pretty good as the initial
> > implementation of utilization handlers, there are several drawbacks
> > though.
> > 
> > The biggest drawback is that the sg_cpu->flags field doesn't always
> > represent the correct type of tasks that are enqueued on a CPU's rq. For
> > example, if a fair task is enqueued while a RT or DL task is running, we
> > will overwrite the flags with value 0 and that may take the CPU to lower
> > OPPs unintentionally. There can be other corner cases as well which we
> > aren't aware of currently.
> > 
> > This patch changes the current implementation to keep track of all the
> > task types that are currently enqueued to the CPUs rq. There are two
> > flags for every scheduling class now, one to set the flag and other one
> > to clear it. The flag is set by the scheduling classes from the existing
> > set of calls to cpufreq_update_util(), and the flag is cleared when the
> > last task of the scheduling class is dequeued. For now, the util update
> > handlers return immediately if they were called to clear the flag.
> > 
> > We can add more optimizations over this patch separately.
> > 
> > The last parameter of sugov_set_iowait_boost() is also dropped as the
> > function can get it from sg_cpu anyway.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> 
> [...]
> 
> > @@ -655,7 +669,7 @@ static int sugov_start(struct cpufreq_policy *policy)
> >  		memset(sg_cpu, 0, sizeof(*sg_cpu));
> >  		sg_cpu->cpu = cpu;
> >  		sg_cpu->sg_policy = sg_policy;
> > -		sg_cpu->flags = SCHED_CPUFREQ_RT;
> > +		sg_cpu->flags = 0;
> >  		sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
> >  	}
> 
> Why this change during initialization?

Firstly I am not sure why it was set to SCHED_CPUFREQ_RT, as schedutil wouldn't
change the frequency until the first time the util handler is called. And once
that is called we were updating the flag anyway. So, unless I misunderstood its
purpose, it was doing anything helpful.

I need to remove it otherwise the RT flag may remain set for a very long time
unnecessarily. That would be until the time the last RT task is not dequeued.
Consider this for example: we are at max freq when sugov_start() is called and
it sets the RT flag, but there is no RT task to run. Now, we have tons of CFS
tasks but we always keep running at max because of the flag. Even the schedutil
RT thread doesn't get a chance to run/deququed, because we never want a freq
change with the RT flag and stay at max.

Makes sense ?

-- 
viresh