linux-kernel - Re: [PATCH] sched/fair: fix sgc->{min,max}

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]

Date:   Wed, 1 Jan 2020 18:55:48 +0000
From:   Valentin Schneider <valentin.schneider@....com>
To:     Peng Liu <iwtbavbm@...il.com>, linux-kernel@...r.kernel.org
Cc:     mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        qais.yousef@....com, morten.rasmussen@....com
Subject: Re: [PATCH] sched/fair: fix sgc->{min,max}_capacity miscalculate

On 01/01/2020 14:13, Peng Liu wrote:
>> ---
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 08a233e97a01..9f6c015639ef 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -7773,8 +7773,8 @@ void update_group_capacity(struct sched_domain *sd, int cpu)
>>  		 */
>>  
>>  		for_each_cpu(cpu, sched_group_span(sdg)) {
>> -			struct sched_group_capacity *sgc;
>>  			struct rq *rq = cpu_rq(cpu);
>> +			unsigned long cpu_cap;
>>  
>>  			/*
>>  			 * build_sched_domains() -> init_sched_groups_capacity()
>> @@ -7787,15 +7787,15 @@ void update_group_capacity(struct sched_domain *sd, int cpu)
>>  			 * This avoids capacity from being 0 and
>>  			 * causing divide-by-zero issues on boot.
>>  			 */
>> -			if (unlikely(!rq->sd)) {
>> -				capacity += capacity_of(cpu);
>> -			} else {
>> -				sgc = rq->sd->groups->sgc;
>> -				capacity += sgc->capacity;
>> -			}
>> +			if (unlikely(!rq->sd))
>> +				cpu_cap = capacity_of(cpu);
> 
> --------------------------------------------------------------
>> +			else
>> +				cpu_cap = rq->sd->groups->sgc->capacity;
> 
> sgc->capacity is the *sum* of all CPU's capacity in that group, right?

Right

> {min,max}_capacity are per CPU variables(*part* of a group). So we can't
> compare *part* to *sum*. Am I overlooking something? Thanks.
> 

AIUI rq->sd->groups->sgc->capacity should be the capacity of the rq's CPU
(IOW the groups here should be made of single CPUs).

This should be true regardless of overlapping domains, since they sit on top
of the regular domains. Let me paint an example with a simple 2-core SMT2
system:

  MC  [          ]
  SMT [    ][    ]
       0  1  2  3

cpu_rq(0)->sd will point to the sched_domain of CPU0 at SMT level (it is the
"base domain", IOW the lowest domain in the topology hierarchy). Its groups
will be:

  {0} ----> {1}
    ^       /
     `-----'

and sd->groups will point at the group spanning the "local" CPU, in our case
CPU0, and thus here will be a group containing only CPU0.


I do not know why sched_group_capacity is used here however. As I understand
things, we could use cpu_capacity() unconditionally.


>> +
>> +			min_capacity = min(cpu_cap, min_capacity);
>> +			max_capacity = max(cpu_cap, max_capacity);
>>  
>> -			min_capacity = min(capacity, min_capacity);
>> -			max_capacity = max(capacity, max_capacity);
>> +			capacity += cpu_cap;
>>  		}
>>  	} else  {
>>  		/*