Re: [PATCH v4 10/18] PM: EM: Add RCU mechanism which safely cleans the old data

[Rafael J. Wysocki]

On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
>
> The EM is going to support runtime modifications of the power data.
> Introduce RCU safe mechanism to clean up the old allocated EM data.

"RCU-based" probably and "to clean up the old EM data safely".

> It also adds a mutex for the EM structure to serialize the modifiers.

This part doesn't match the code changes in the patch.

> Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> ---
>  kernel/power/energy_model.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> index 5b40db38b745..2345837bfd2c 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -23,6 +23,9 @@
>   */
>  static DEFINE_MUTEX(em_pd_mutex);
>
> +static void em_cpufreq_update_efficiencies(struct device *dev,
> +                                          struct em_perf_state *table);
> +
>  static bool _is_cpu_device(struct device *dev)
>  {
>         return (dev->bus == &cpu_subsys);
> @@ -104,6 +107,32 @@ static void em_debug_create_pd(struct device *dev) {}
>  static void em_debug_remove_pd(struct device *dev) {}
>  #endif
>
> +static void em_destroy_rt_table_rcu(struct rcu_head *rp)

Adding static functions without callers will obviously cause the
compiler to complain, which is one of the reasons to avoid doing that.
The other is that it is hard to say how these functions are going to
be used without reviewing multiple patches simultaneously, which is a
pain as far as I'm concerned.

> +{
> +       struct em_perf_table *runtime_table;
> +
> +       runtime_table = container_of(rp, struct em_perf_table, rcu);
> +       kfree(runtime_table->state);
> +       kfree(runtime_table);

If runtime_table and its state were allocated in one go, it would be
possible to free them in one go either.

For some reason, you don't seem to want to do that, but why?

> +}
> +
> +static void em_perf_runtime_table_set(struct device *dev,
> +                                     struct em_perf_table *runtime_table)
> +{
> +       struct em_perf_domain *pd = dev->em_pd;
> +       struct em_perf_table *tmp;
> +
> +       tmp = pd->runtime_table;
> +
> +       rcu_assign_pointer(pd->runtime_table, runtime_table);
> +
> +       em_cpufreq_update_efficiencies(dev, runtime_table->state);
> +
> +       /* Don't free default table since it's used by other frameworks. */

Apparently, some frameworks are only going to use the default table
while the runtime-updatable table will be used somewhere else at the
same time.

I'm not really sure if this is a good idea.

> +       if (tmp != pd->default_table)
> +               call_rcu(&tmp->rcu, em_destroy_rt_table_rcu);
> +}
> +
>  static int em_compute_costs(struct device *dev, struct em_perf_state *table,
>                             struct em_data_callback *cb, int nr_states,
>                             unsigned long flags)
> --