Re: [PATCH v4 05/18] PM: EM: Refactor a new function em_compute_costs()

[Rafael J. Wysocki]

On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@xxxxxxx> 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@xxxxxxx>

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
>