Hi Juri,
On Wed, Jul 5, 2017 at 1:59 AM, Juri Lelli <juri.le...@arm.com> wrote:
> To be able to treat utilization signals of different scheduling classes
> in different ways (e.g., CFS signal might be stale while DEADLINE signal
> is never stale by design) we need to split sugov_cpu::util signal in two:
> util_cfs and util_dl.
>
> This patch does that by also changing sugov_get_util() parameter list.
> After this change, aggregation of the different signals has to be performed
> by sugov_get_util() users (so that they can decide what to do with the
> different signals).
>
> Suggested-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Signed-off-by: Juri Lelli <juri.le...@arm.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Cc: Viresh Kumar <viresh.ku...@linaro.org>
> Cc: Luca Abeni <luca.ab...@santannapisa.it>
> Cc: Claudio Scordino <clau...@evidence.eu.com>
> ---
> Changes from RFCv0:
>
> - refactor aggregation of utilization in sugov_aggregate_util()
> ---
> kernel/sched/cpufreq_schedutil.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c
> b/kernel/sched/cpufreq_schedutil.c
> index ba6227625f24..e835fa886225 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -58,7 +58,8 @@ struct sugov_cpu {
> u64 last_update;
>
> /* The fields below are only needed when sharing a policy. */
> - unsigned long util;
> + unsigned long util_cfs;
> + unsigned long util_dl;
> unsigned long max;
> unsigned int flags;
>
> @@ -154,20 +155,24 @@ static unsigned int get_next_freq(struct sugov_policy
> *sg_policy,
> return cpufreq_driver_resolve_freq(policy, freq);
> }
>
> -static void sugov_get_util(unsigned long *util, unsigned long *max)
> +static void sugov_get_util(struct sugov_cpu *sg_cpu)
> {
> struct rq *rq = this_rq();
> - unsigned long dl_util = (rq->dl.running_bw * SCHED_CAPACITY_SCALE)
> - >> BW_SHIFT;
>
> - *max = arch_scale_cpu_capacity(NULL, smp_processor_id());
> + sg_cpu->max = arch_scale_cpu_capacity(NULL, smp_processor_id());
> + sg_cpu->util_cfs = rq->cfs.avg.util_avg;
> + sg_cpu->util_dl = (rq->dl.running_bw * SCHED_CAPACITY_SCALE)
> + >> BW_SHIFT;
> +}
>
> +static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
> +{
> /*
> * Ideally we would like to set util_dl as min/guaranteed freq and
> * util_cfs + util_dl as requested freq. However, cpufreq is not yet
> * ready for such an interface. So, we only do the latter for now.
> */
> - *util = min(rq->cfs.avg.util_avg + dl_util, *max);
> + return min(sg_cpu->util_cfs + sg_cpu->util_dl, sg_cpu->max);
> }
I am wondering why the need for a separate aggregation API. To me, it
looks like using sugov_get_util to set the sg_cpu util elements and
then do the aggregation at the same time would have the same effect
(without changing the existing parameter list). Is this to handle a
future usecase where aggregation may need to be done differently? For
all the user's of sugov_get_util, aggregation is done in the same way.
Anyway if I missed something, sorry for the noise.
thanks,
-Joel
