linux-kernel - Re: [PATCH v5 00/23] Introduce runtime modifiable Energy Model

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, 13 Dec 2023 12:45:21 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Dietmar Eggemann <dietmar.eggemann@....com>
Cc:     Lukasz Luba <lukasz.luba@....com>, rui.zhang@...el.com,
        amit.kucheria@...durent.com, linux-pm@...r.kernel.org,
        amit.kachhap@...il.com, daniel.lezcano@...aro.org,
        viresh.kumar@...aro.org, len.brown@...el.com, pavel@....cz,
        mhiramat@...nel.org, qyousef@...alina.io, wvw@...gle.com,
        linux-kernel@...r.kernel.org, rafael@...nel.org
Subject: Re: [PATCH v5 00/23] Introduce runtime modifiable Energy Model

On Wed, Dec 13, 2023 at 12:34 PM Dietmar Eggemann
<dietmar.eggemann@....com> wrote:
>
> On 13/12/2023 10:23, Lukasz Luba wrote:
> > Hi Dietmar,
> >
> > Thank you for the review, I will go one-by-one to respond
> > your comments in patches as well. First comments are below.
> >
> > On 12/12/23 18:48, Dietmar Eggemann wrote:
> >> On 29/11/2023 12:08, Lukasz Luba wrote:
>
> [...]
>
> >>> Changelog:
> >>> v5:
> >>> - removed 2 tables design
> >>> - have only one table (runtime_table) used also in thermal (Wei, Rafael)
> >>
> >> Until v4 you had 2 EM's, the static and the modifiable (runtime). Now in
> >> v5 this changed to only have one, the modifiable. IMHO it would be
> >> better to change the existing table to be modifiable rather than staring
> >> with two EM's and then removing the static one. I assume you end up with
> >> way less code changes and the patch-set will become easier to digest for
> >> reviewers.
> >
> > The patches are structured in this way following Daniel's recommendation
> > I got when I was adding similar big changes to EM in 2020 (support all
> > devices in kernel). The approach is as follows:
> > 0. Do some basic clean-up/refactoring if needed for a new feature, to
> >    re-use some code if possible in future
> > 1. Introduce new feature next to the existing one
> > 2. Add API and all needed infrastructure (structures, fields) for
> >    drivers
> > 3. Re-wire the existing drivers/frameworks to the new feature via new
> >    API; ideally keep 1 patch per driver so the maintainer can easily
> >    grasp the changes and ACK it, because it will go via different tree
> >    (Rafael's tree); in case of some code clash in the driver's code
> >    during merge - it will be a single driver so easier to handle
> > 4. when all drivers and frameworks are wired up with the new feature
> >    remove the old feature (structures, fields, APIs, etc)
> > 5. Update the documentation with new latest state of desing
> >
> > In this approach the patches are less convoluted. Because if I remove
> > the old feature and add new in a single patch (e.g. the main structure)
> > that patch will have to modify all drivers to still compile. It
> > would be a big messy patch for this re-design.
> >
> > I can see in some later comment from Rafael that he is OK with current
> > patch set structure.
>
> OK, in case Rafael and Daniel prefer this, then it's fine.
>
> I just find it weird that we now have
>
> 70 struct em_perf_domain {
> 71         struct em_perf_table __rcu *runtime_table;
>                                        ^^^^^^^^^^^^^
>
> as the only EM table.

I agree that it would be better to call it something like em_table.