Re: [PATCH v2 1/3] sched/fair: Add asymmetric CPU capacity wakeup scan

[Valentin Schneider <valentin.schneider@arm.com>]

(copied over from the v1 thread)

On 24/01/2020 12:59, Quentin Perret wrote:
> Hey Valentin,
> 
> On Friday 24 Jan 2020 at 12:42:53 (+0000), Valentin Schneider wrote:
>> +/*
>> + * Scan the asym_capacity domain for idle CPUs; pick the first idle one on which
>> + * the task fits.
>> + */
>> +static int select_idle_capacity(struct task_struct *p, int target)
>> +{
>> +	struct sched_domain *sd;
>> +	struct cpumask *cpus;
>> +	int cpu;
>> +
>> +	if (!static_branch_unlikely(&sched_asym_cpucapacity))
>> +		return -1;
>> +
>> +	sd = rcu_dereference(per_cpu(sd_asym_cpucapacity, target));
>> +	if (!sd)
>> +		return -1;
>> +
> 
> You might want 'sync_entity_load_avg(&p->se)' here no ?
> find_idlest_cpu() and wake_cap() need one, but since we're going to use
> them, you'll want to sync here too I think.
> 

Yeah, I think you're right, it's the exact same scenario here.

>> +	cpus = this_cpu_cpumask_var_ptr(select_idle_mask);
>> +	cpumask_and(cpus, sched_domain_span(sd), p->cpus_ptr);
>> +
>> +	for_each_cpu_wrap(cpu, cpus, target) {
>> +		if (!available_idle_cpu(cpu))
>> +			continue;
>> +		if (!task_fits_capacity(p, capacity_of(cpu)))
>> +			continue;
>> +
>> +		return cpu;
> 
> If we found an idle CPU, but not one big enough, should we still go
> ahead and choose it ? Misfit / idle balance will fix that later when a
> big CPU does become available.
> 

If we fail to find a big enough CPU, we'll just fallback to the rest of
select_idle_sibling() which will pick an idle CPU, just without caring
about capacity.

Now an alternative here would be to:
- return the first idle CPU on which the task fits (what the above does)
- else, return the biggest idle CPU we found (this could e.g. still steer
  the task towards a medium on a tri-capacity system)

I think what we were trying to go with here is to not entirely hijack
select_idle_sibling(). If we go with the above alternative, topologies
with sched_asym_cpucapacity enabled would only ever see
select_idle_capacity() and not the rest of select_idle_sibling(). Not sure
if it's a bad thing or not, but it's something to ponder over.