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

[Lukasz Luba]

On 12/13/23 11:45, Rafael J. Wysocki wrote:
> On Wed, Dec 13, 2023 at 12:34 PM Dietmar Eggemann
> <dietmar.eggemann@xxxxxxx> 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.

OK, I'll change that. Thanks Rafael and Dietmar!