[v2,2/2] sched/fair, cpufreq: Introduce 'runnable boosting'

  The responsiveness of the Per Entity Load Tracking (PELT) util_avg in
mobile devices is still considered too low for utilization changes
during task ramp-up.

In Android this manifests in the fact that the first frames of a UI
activity are very prone to be jankframes (a frame which doesn't meet
the required frame rendering time, e.g. 16ms@60Hz) since the CPU
frequency is normally low at this point and has to ramp up quickly.

The beginning of an UI activity is also characterized by the occurrence
of CPU contention, especially on little CPUs. Current little CPUs can
have an original CPU capacity of only ~ 150 which means that the actual
CPU capacity at lower frequency can even be much smaller.

Schedutil maps CPU util_avg into CPU frequency request via:

  util = effective_cpu_util(..., cpu_util_cfs(cpu), ...) ->
  util = map_util_perf(util) -> freq = map_util_freq(util, ...)

CPU contention for CFS tasks can be detected by the 'CPU runnable_avg >
util_avg' condition in cpu_util_cfs(..., boost) -> cpu_util(..., boost).
Schedutil activates 'runnable boosting' by setting the new parameter
'boost = 1'.

To be in sync with schedutil's CPU frequency selection, Energy Aware
Scheduling (EAS) also calls cpu_util(..., boost = 1) during max util
detection.

Moreover, 'runnable boosting' is also used in load-balance for busiest
CPU selection when the migration type is 'migrate_util', i.e. only at
sched domains which don't have the SD_SHARE_PKG_RESOURCES flag set.

Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
 kernel/sched/core.c              |  2 +-
 kernel/sched/cpufreq_schedutil.c |  3 ++-
 kernel/sched/fair.c              | 29 ++++++++++++++++++-----------
 kernel/sched/sched.h             |  4 ++--
 4 files changed, 23 insertions(+), 15 deletions(-)
  

On Fri, May 12, 2023 at 12:10:29PM +0200, Dietmar Eggemann wrote:

> -static unsigned long cpu_util(int cpu, struct task_struct *p, int dst_cpu)
> +static unsigned long
> +cpu_util(int cpu, struct task_struct *p, int dst_cpu, int boost)
>  {
>  	struct cfs_rq *cfs_rq = &cpu_rq(cpu)->cfs;
>  	unsigned long util = READ_ONCE(cfs_rq->avg.util_avg);
> +	unsigned long runnable;
> +
> +	runnable = boost ? READ_ONCE(cfs_rq->avg.runnable_avg) : 0;
> +	util = max(util, runnable);
>  
	if (boost)
		util = max(util, READ_ONCE(cfs_rq->avg.runnable_avg));


> @@ -7239,9 +7246,9 @@ static unsigned long cpu_util(int cpu, struct task_struct *p, int dst_cpu)
>   *
>   * Return: (Estimated) utilization for the specified CPU.
>   */

Given that cpu_util() is the base function should this comment move
there?

> -unsigned long cpu_util_cfs(int cpu)
> +unsigned long cpu_util_cfs(int cpu, int boost)
>  {
> -	return cpu_util(cpu, NULL, -1);
> +	return cpu_util(cpu, NULL, -1, boost);
>  }

AFAICT the @boost argument is always a constant (0 or 1). Would it not
make more sense to simply add:

unsigned long cpu_util_cfs_boost(int cpu)
{
	return cpu_util(cpu, NULL, -1, 1);
}

and use that in the few sites that actually need it?
  

On Fri, May 12, 2023 at 12:10:29PM +0200, Dietmar Eggemann wrote:

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index e3211455b203..3b902f533214 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -155,10 +155,11 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
>  
>  static void sugov_get_util(struct sugov_cpu *sg_cpu)
>  {
> +	unsigned long util = cpu_util_cfs(sg_cpu->cpu, 1);
>  	struct rq *rq = cpu_rq(sg_cpu->cpu);
>  
>  	sg_cpu->bw_dl = cpu_bw_dl(rq);
> -	sg_cpu->util = effective_cpu_util(sg_cpu->cpu, cpu_util_cfs(sg_cpu->cpu),
> +	sg_cpu->util = effective_cpu_util(sg_cpu->cpu, util,
>  					  FREQUENCY_UTIL, NULL);
>  }
>  
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1220cfbee258..3a10fe5988d6 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c

> @@ -7355,7 +7362,7 @@ eenv_pd_max_util(struct energy_env *eenv, struct cpumask *pd_cpus,
>  
>  	for_each_cpu(cpu, pd_cpus) {
>  		struct task_struct *tsk = (cpu == dst_cpu) ? p : NULL;
> -		unsigned long util = cpu_util(cpu, p, dst_cpu);
> +		unsigned long util = cpu_util(cpu, p, dst_cpu, 1);
>  		unsigned long cpu_util;
>  
>  		/*

> @@ -10561,7 +10568,7 @@ static struct rq *find_busiest_queue(struct lb_env *env,
>  			break;
>  
>  		case migrate_util:
> -			util = cpu_util_cfs(i);
> +			util = cpu_util_cfs(i, 1);
>  
>  			/*
>  			 * Don't try to pull utilization from a CPU with one

When you move that comment from cpu_util_cfs() to cpu_util() please also
add a paragraph about why boost=1 and why these locations, because I'm
sure we're going to be asking ouselves that at some point.
  

On 12/05/2023 13:22, Peter Zijlstra wrote:
> On Fri, May 12, 2023 at 12:10:29PM +0200, Dietmar Eggemann wrote:
> 
>> -static unsigned long cpu_util(int cpu, struct task_struct *p, int dst_cpu)
>> +static unsigned long
>> +cpu_util(int cpu, struct task_struct *p, int dst_cpu, int boost)
>>  {
>>  	struct cfs_rq *cfs_rq = &cpu_rq(cpu)->cfs;
>>  	unsigned long util = READ_ONCE(cfs_rq->avg.util_avg);
>> +	unsigned long runnable;
>> +
>> +	runnable = boost ? READ_ONCE(cfs_rq->avg.runnable_avg) : 0;
>> +	util = max(util, runnable);
>>  
> 	if (boost)
> 		util = max(util, READ_ONCE(cfs_rq->avg.runnable_avg));

I need the util_est = max(util_est, runnable) further down as well. Just
want to fetch runnable only once.

util = 50, task_util = 5, util_est = 60, task_util_est = 10, runnable = 70

max(70 + 5, 60 + 10) != max (70 + 5, 70 + 10) when dst_cpu == cpu

>> @@ -7239,9 +7246,9 @@ static unsigned long cpu_util(int cpu, struct task_struct *p, int dst_cpu)
>>   *
>>   * Return: (Estimated) utilization for the specified CPU.
>>   */
> 
> Given that cpu_util() is the base function should this comment move
> there?

Yes, will move it.

>> -unsigned long cpu_util_cfs(int cpu)
>> +unsigned long cpu_util_cfs(int cpu, int boost)
>>  {
>> -	return cpu_util(cpu, NULL, -1);
>> +	return cpu_util(cpu, NULL, -1, boost);
>>  }
> 
> AFAICT the @boost argument is always a constant (0 or 1). Would it not
> make more sense to simply add:
> 
> unsigned long cpu_util_cfs_boost(int cpu)
> {
> 	return cpu_util(cpu, NULL, -1, 1);
> }
> 
> and use that in the few sites that actually need it?

Yes, will change it.
  

On 12/05/2023 13:32, Peter Zijlstra wrote:
> On Fri, May 12, 2023 at 12:10:29PM +0200, Dietmar Eggemann wrote:

[...]

>> @@ -10561,7 +10568,7 @@ static struct rq *find_busiest_queue(struct lb_env *env,
>>  			break;
>>  
>>  		case migrate_util:
>> -			util = cpu_util_cfs(i);
>> +			util = cpu_util_cfs(i, 1);
>>  
>>  			/*
>>  			 * Don't try to pull utilization from a CPU with one
> 
> When you move that comment from cpu_util_cfs() to cpu_util() please also
> add a paragraph about why boost=1 and why these locations, because I'm
> sure we're going to be asking ouselves that at some point.

Will do.
  

On Fri, May 12, 2023 at 04:39:51PM +0200, Dietmar Eggemann wrote:
> On 12/05/2023 13:22, Peter Zijlstra wrote:
> > On Fri, May 12, 2023 at 12:10:29PM +0200, Dietmar Eggemann wrote:
> > 
> >> -static unsigned long cpu_util(int cpu, struct task_struct *p, int dst_cpu)
> >> +static unsigned long
> >> +cpu_util(int cpu, struct task_struct *p, int dst_cpu, int boost)
> >>  {
> >>  	struct cfs_rq *cfs_rq = &cpu_rq(cpu)->cfs;
> >>  	unsigned long util = READ_ONCE(cfs_rq->avg.util_avg);
> >> +	unsigned long runnable;
> >> +
> >> +	runnable = boost ? READ_ONCE(cfs_rq->avg.runnable_avg) : 0;
> >> +	util = max(util, runnable);
> >>  
> > 	if (boost)
> > 		util = max(util, READ_ONCE(cfs_rq->avg.runnable_avg));
> 
> I need the util_est = max(util_est, runnable) further down as well. Just
> want to fetch runnable only once.

Ah, fair enought; but still, please use a regular if().