linux-kernel - Re: [PATCH] sched/fair: Track target domain's avg_scan_cost in select_idle

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:   Thu, 6 Jan 2022 17:10:07 +0800
From:   Yicong Yang <yangyicong@...wei.com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
CC:     <yangyicong@...ilicon.com>, <peterz@...radead.org>,
        <mingo@...hat.com>, <juri.lelli@...hat.com>,
        <mgorman@...hsingularity.net>, <linux-kernel@...r.kernel.org>,
        <dietmar.eggemann@....com>, <rostedt@...dmis.org>,
        <bsegall@...gle.com>, <bristot@...hat.com>,
        <prime.zeng@...wei.com>, <linuxarm@...wei.com>,
        <21cnbao@...il.com>, "tiantao (H)" <tiantao6@...ilicon.com>
Subject: Re: [PATCH] sched/fair: Track target domain's avg_scan_cost in
 select_idle_cpu

On 2022/1/4 22:18, Vincent Guittot wrote:
> On Thu, 23 Dec 2021 at 09:23, Yicong Yang <yangyicong@...wei.com> wrote:
>>
>> On 2021/12/22 18:47, Vincent Guittot wrote:
>>> On Sat, 11 Dec 2021 at 11:43, Yicong Yang <yangyicong@...ilicon.com> wrote:
>>>>
>>>> We regulate the LLC domain scan in select_idle_cpu() by comparing
>>>> the average scan cost of this_sd against the average idle time of
>>>> this_rq. This is correct when the domain to scan is the LLC domain
>>>> of this cpu. But when the domain to scan is different from this
>>>> LLC domain, we'll have an inaccurate estimation of the scan cost
>>>> on the target domain as this_sd->avg_scan_cost contains contributions
>>>> of scanning other domains besides the target domain.
>>>>
>>>> Track the avg_scan_cost of the target domain to make the estimation
>>>> more accurate.
>>>>
>>>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
>>>> ---
>>>>  kernel/sched/fair.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index 6e476f6d9435..6301740d98cb 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -6267,7 +6267,7 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
>>>>                 }
>>>>
>>>>                 avg_idle = this_rq->wake_avg_idle;
>>>> -               avg_cost = this_sd->avg_scan_cost + 1;
>>>> +               avg_cost = sd->avg_scan_cost + 1;
>>>>
>>>>                 span_avg = sd->span_weight * avg_idle;
>>>>                 if (span_avg > 4*avg_cost)
>>>> @@ -6305,7 +6305,7 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
>>>>                  */
>>>>                 this_rq->wake_avg_idle -= min(this_rq->wake_avg_idle, time);
>>>>
>>>> -               update_avg(&this_sd->avg_scan_cost, time);
>>>> +               update_avg(&sd->avg_scan_cost, time);
>>>
>>> But then you can have several cpus updating the same value simultaneously
>>>
>>
>> yes. sd->avg_scan_cost should includes the contributions of all the cpus scanned the sd.
>>
>> We regulated the scanning nr based on two things:
>> - avg_idle: to indicate how much time we can have for this time scanning
>> - avg_cost: to indicate how much time we'll spend for scanning the target domain based
>>             on the history cost
>>
>> Previously sd->avg_scan_cost may not reflect the cost as it count the scanning cost
>> on the domain of the scanner cpu, which may not be the domain the cpu scanned.
>> For example, cpu 0 on llc A scanned llc B and llc C, we'll count the cost of scanning B
>> and C on llc A's avg_scan_cost and we'll use this to estimate the cost for scanning
>> llc A, which is not accurate.
> 
> I mean that you can now have several CPUs that will read, modify,
> write sd->avg_scan_cost simultaneously without any protection
> 

uh I misunderstood. not sure I've missed something, but looks like we also have this problem
when updating &this_sd->avg_scan_cost?

>>
>>>>         }
>>>>
>>>>         return idle_cpu;
>>>> --
>>>> 2.33.0
>>>>
>>> .
>>>
> .
>