Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20230925081139.1305766-1-lukasz.luba@arm.com> <20230925081139.1305766-6-lukasz.luba@arm.com>
In-Reply-To: <20230925081139.1305766-6-lukasz.luba@arm.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 26 Sep 2023 20:39:40 +0200
Message-ID: <CAJZ5v0hyeKGuC5-jJDoxBpNksMg1cZ7eoCysjx7O8Ey1i8YSDg@mail.gmail.com>
Subject: Re: [PATCH v4 05/18] PM: EM: Refactor a new function em_compute_costs()
To:     Lukasz Luba <lukasz.luba@arm.com>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        rafael@kernel.org, dietmar.eggemann@arm.com, rui.zhang@intel.com,
        amit.kucheria@verdurent.com, amit.kachhap@gmail.com,
        daniel.lezcano@linaro.org, viresh.kumar@linaro.org,
        len.brown@intel.com, pavel@ucw.cz, mhiramat@kernel.org,
        qyousef@layalina.io, wvw@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Refactor a dedicated function which will be easier to maintain and re-use
> in future. The upcoming changes for the modifiable EM perf_state table
> will use it (instead of duplicating the code).
>
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>

If I'm not mistaken, this patch by itself is not going to change the
observable functionality in any way and it would be good to say that
in the changelog.

This also applies to some other patches in this series.

> ---
>  kernel/power/energy_model.c | 72 ++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 3dafdd7731c4..7ea882401833 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -103,14 +103,52 @@ static void em_debug_create_pd(struct device *dev) {}
>  static void em_debug_remove_pd(struct device *dev) {}
>  #endif
>
> +static int em_compute_costs(struct device *dev, struct em_perf_state *table,
> +                           struct em_data_callback *cb, int nr_states,
> +                           unsigned long flags)
> +{
> +       unsigned long prev_cost = ULONG_MAX;
> +       u64 fmax;
> +       int i, ret;
> +
> +       /* Compute the cost of each performance state. */
> +       fmax = (u64) table[nr_states - 1].frequency;
> +       for (i = nr_states - 1; i >= 0; i--) {
> +               unsigned long power_res, cost;
> +
> +               if (flags & EM_PERF_DOMAIN_ARTIFICIAL) {
> +                       ret = cb->get_cost(dev, table[i].frequency, &cost);
> +                       if (ret || !cost || cost > EM_MAX_POWER) {
> +                               dev_err(dev, "EM: invalid cost %lu %d\n",
> +                                       cost, ret);
> +                               return -EINVAL;
> +                       }
> +               } else {
> +                       power_res = table[i].power;
> +                       cost = div64_u64(fmax * power_res, table[i].frequency);
> +               }
> +
> +               table[i].cost = cost;
> +
> +               if (table[i].cost >= prev_cost) {
> +                       table[i].flags = EM_PERF_STATE_INEFFICIENT;
> +                       dev_dbg(dev, "EM: OPP:%lu is inefficient\n",
> +                               table[i].frequency);
> +               } else {
> +                       prev_cost = table[i].cost;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>                                 int nr_states, struct em_data_callback *cb,
>                                 unsigned long flags)
>  {
> -       unsigned long power, freq, prev_freq = 0, prev_cost = ULONG_MAX;
> +       unsigned long power, freq, prev_freq = 0;
>         struct em_perf_state *table;
>         int i, ret;
> -       u64 fmax;
>
>         table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL);
>         if (!table)
> @@ -154,33 +192,9 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd,
>                 table[i].frequency = prev_freq = freq;
>         }
>
> -       /* Compute the cost of each performance state. */
> -       fmax = (u64) table[nr_states - 1].frequency;
> -       for (i = nr_states - 1; i >= 0; i--) {
> -               unsigned long power_res, cost;
> -
> -               if (flags & EM_PERF_DOMAIN_ARTIFICIAL) {
> -                       ret = cb->get_cost(dev, table[i].frequency, &cost);
> -                       if (ret || !cost || cost > EM_MAX_POWER) {
> -                               dev_err(dev, "EM: invalid cost %lu %d\n",
> -                                       cost, ret);
> -                               goto free_ps_table;
> -                       }
> -               } else {
> -                       power_res = table[i].power;
> -                       cost = div64_u64(fmax * power_res, table[i].frequency);
> -               }
> -
> -               table[i].cost = cost;
> -
> -               if (table[i].cost >= prev_cost) {
> -                       table[i].flags = EM_PERF_STATE_INEFFICIENT;
> -                       dev_dbg(dev, "EM: OPP:%lu is inefficient\n",
> -                               table[i].frequency);
> -               } else {
> -                       prev_cost = table[i].cost;
> -               }
> -       }
> +       ret = em_compute_costs(dev, table, cb, nr_states, flags);
> +       if (ret)
> +               goto free_ps_table;
>
>         pd->table = table;
>         pd->nr_perf_states = nr_states;
> --
> 2.25.1
>