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

From: Valentin Schneider
Date: Fri Jan 24 2020 - 09:14:51 EST


(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.