linux-kernel - Re: [PATCH 1/3] ARM: exynos_defconfig: Enable SCHED

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:   Fri, 31 Jan 2020 21:33:24 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     kgene@...nel.org, linux-arm-kernel@...ts.infradead.org,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org, linux-pm@...r.kernel.org,
        myungjoo.ham@...sung.com, kyungmin.park@...sung.com,
        Chanwoo Choi <cw00.choi@...sung.com>, robh+dt@...nel.org,
        mark.rutland@....com,
        Bartłomiej Żołnierkiewicz 
        <b.zolnierkie@...sung.com>, dietmar.eggemann@....com
Subject: Re: [PATCH 1/3] ARM: exynos_defconfig: Enable SCHED_MC

On Fri, Jan 31, 2020 at 03:59:30PM +0000, Lukasz Luba wrote:
> Hi Krzysztof,
> 
> Thank you for your review, please see my comments below.
> 
> On 1/31/20 12:47 PM, Krzysztof Kozlowski wrote:
> > On Mon, 27 Jan 2020 at 22:55, <lukasz.luba@....com> wrote:
> > > 
> > > From: Lukasz Luba <lukasz.luba@....com>
> > > 
> > > Since the 'capacities-dmips-mhz' are present in the CPU nodes, make use of
> > > this knowledge in smarter decisions during scheduling.
> > > 
> > > The values in 'capacities-dmips-mhz' are normilized, this means that i.e.
> > > when CPU0's capacities-dmips-mhz=100 and CPU1's 'capacities-dmips-mhz'=50,
> > > cpu0 is twice fast as CPU1, at the same frequency. The proper hirarchy
> > > in sched_domain topology could exploit the SoC architecture advantages
> > > like big.LITTLE.
> > 
> > I do not quite get how this is related to rationale behind changing defconfig...
> 
> It is not strictly about EAS, it is useful in general for big.LITTLE
> platform with clusters.
> 
> > 
> > > Enabling the SCHED_MC will create two levels in
> > > sched_domain hierarchy, which might be observed in:
> > 
> > This is looks more convincing... but still what is the need? To work with EAS?
> 
> It is not only for EAS, but in general for the scheduler (load balance,
> task's wake-up path, etc). The scheduler algorithms iterate CPUs in the
> sched groups. To make better decisions, the information about MC domain
> is needed. More about the scheduler domains and i.e. load_balance()
> you can find here:
> 
> https://www.kernel.org/doc/html/latest/scheduler/sched-domains.html

Ahhh, I see, it's independent of later patches. Somehow I had impression
it is a prerequisite...

> 
> > 
> > > grep . /proc/sys/kernel/sched_domain/cpu*/domain*/{name,flags}
> > > /proc/sys/kernel/sched_domain/cpu0/domain0/name:MC
> > > /proc/sys/kernel/sched_domain/cpu0/domain1/name:DIE
> > > ...
> > > /proc/sys/kernel/sched_domain/cpu0/domain0/flags:575
> > > /proc/sys/kernel/sched_domain/cpu0/domain1/flags:4223
> > 
> > Not related to defconfig change and not visible after this commit.
> 
> Without this patch there is only one domain: 'domain0' -> 'DIE'
> cat /proc/sys/kernel/sched_domain/cpu0/domain0/name
> DIE
> 
> When you apply this patch you will get two: 'domain0, 'domain1'
> grep . /proc/sys/kernel/sched_domain/cpu0/domain?/name
> 
> /proc/sys/kernel/sched_domain/cpu0/domain0/name:MC
> /proc/sys/kernel/sched_domain/cpu0/domain1/name:DIE
> 
> I can remove it this information, but it is the most important
> to spot this difference out.
> 
> This is also the main reason I haven't merge the patch 1 + 3.

Indeed. I thought that these will pop up at the end of the patchset, my
bad.

I do not see big benefits of adding these outputs as proofs of working
SCHED_MC because they are kind of obvious. It is not a measurement but
report of current system state. However they don't harm neither, so I am
fine with it.

However please us in commit msg also the name of turned on option, next
or instead of SCHED_MC.  The options might be sometimes cryptic or too
vague and the name actually easily expresses what you want enable.