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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <00aa64e8-5e75-181e-a4f4-72c2ac64081c@arm.com>
Date:   Fri, 24 Jan 2020 14:14:47 +0000
From:   Valentin Schneider <valentin.schneider@....com>
To:     linux-kernel@...r.kernel.org
Cc:     mingo@...hat.com, peterz@...radead.org, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, morten.rasmussen@....com,
        qperret@...gle.com, adharmap@...eaurora.org
Subject: Re: [PATCH v2 1/3] sched/fair: Add asymmetric CPU capacity wakeup
 scan

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