Re: [RFC/RFT][PATCH 1/1] cpufreq: New governor using utilization data from the scheduler

[Juri Lelli]

Hi Rafael,

thanks for this RFC. I'm going to test it more in the next few days, but
I already have some questions from skimming through it. Please find them
inline below.

On 22/02/16 00:18, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> Add a new cpufreq scaling governor, called "schedutil", that uses
> scheduler-provided CPU utilization information as input for making
> its decisions.
> 

I guess the first (macro) question is why did you decide to go with a
complete new governor, where new here is w.r.t. the sched-freq solution.
AFAICT, it is true that your solution directly builds on top of the
latest changes to cpufreq core and governor, but it also seems to have
more than a few points in common with sched-freq, and sched-freq has
been discussed and evaluated for already quite some time. Also, it
appears to me that they both shares (or they might encounter in the
future as development progresses) the same kind of problems, like for
example the fact that we can't trigger opp changes from scheduler
context ATM.

Don't get me wrong. I think that looking at different ways to solve a
problem is always beneficial, since I guess that the goal in the end is
to come up with something that suits everybody's needs. I was only
curious about your thoughts on sched-freq. But we can also wait for the
next RFC from Steve for this macro question. :-)

[...]

> +static void sugov_update_commit(struct policy_dbs_info *policy_dbs, u64 time,
> +				unsigned int next_freq)
> +{
> +	struct sugov_policy *sg_policy = to_sg_policy(policy_dbs);
> +
> +	sg_policy->next_freq = next_freq;
> +	policy_dbs->last_sample_time = time;
> +	policy_dbs->work_in_progress = true;
> +	irq_work_queue(&policy_dbs->irq_work);

Here we basically use the system wq to be able to do the freq transition
in process context. CFS is probably fine with this, but don't you think
we might get into troubles when, in the future, we will want to service
RT/DL requests more properly and they will end up being serviced
together with all the others wq users and at !RT priority?

> +}
> +
> +static void sugov_update_shared(struct update_util_data *data, u64 time,
> +				unsigned long util, unsigned long max)
> +{

We don't have a way to tell from which scheduling class this is coming
from, do we? And if that is true can't a request from CFS overwrite
RT/DL go to max requests?

[...]

Anyway, I'm going to start using our existing testing infrastructure
used to evaluate sched-freq to try to better understand the implications
of your approach.

Best,

- Juri