Re: [PATCH v3 09/12] PM: EM: Add RCU mechanism which safely cleans the old data

[Lukasz Luba]

On 8/16/23 14:06, Dietmar Eggemann wrote:
> On 21/07/2023 17:50, Lukasz Luba 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.
> > It also adds a mutex for the EM structure to serialize the modifiers.
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   kernel/power/energy_model.c | 42 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >
> > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> > index c2f8a0046f8a..4596bfe7398e 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,45 @@ 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)
> > +{
> > +	struct em_perf_table *runtime_table;
> > +
> > +	runtime_table = container_of(rp, struct em_perf_table, rcu);
> > +	kfree(runtime_table->state);
> > +	kfree(runtime_table);
> > +}
> > +
> > +static void em_destroy_tmp_setup_rcu(struct rcu_head *rp)
> > +{
> > +	struct em_perf_table *runtime_table;
> > +
> > +	runtime_table = container_of(rp, struct em_perf_table, rcu);
> > +	kfree(runtime_table);
> > +}
>
> Still don't like that we have to have 2 rcu callbacks here. In case we
> could assign default_table to runtime_table in em_create_pd() (and not
> just default_table->state to runtime_table->state) IMHO we would only
> need one rcu callback?

You have convinced me. I'll change that code.

> -->8--
>
> -static void em_destroy_tmp_setup_rcu(struct rcu_head *rp)
> -{
> -       struct em_perf_table *runtime_table;
> -
> -       runtime_table = container_of(rp, struct em_perf_table, rcu);
> -       kfree(runtime_table);
> -}
> -
>   static void em_perf_runtime_table_set(struct device *dev,
>                                        struct em_perf_table *runtime_table)
>   {
> @@ -136,13 +128,8 @@ static void em_perf_runtime_table_set(struct device *dev,
>   
>          em_cpufreq_update_efficiencies(dev, runtime_table->state);
>   
> -       /*
> -        * Check if the 'state' array is not actually the one from setup.
> -        * If it is then don't free it.
> -        */
> -       if (tmp->state == pd->default_table->state)
> -               call_rcu(&tmp->rcu, em_destroy_tmp_setup_rcu);
> -       else
> +       /* Don't free default table (inital value of runtime table) */
> +       if (tmp != pd->default_table)
>                  call_rcu(&tmp->rcu, em_destroy_rt_table_rcu);
>   }
>   
> @@ -349,7 +336,6 @@ static int em_create_pd(struct device *dev, int nr_states,
>                          unsigned long flags)
>   {
>          struct em_perf_table *default_table;
> -       struct em_perf_table *runtime_table;
>          struct em_perf_domain *pd;
>          struct device *cpu_dev;
>          int cpu, ret, num_cpus;
> @@ -382,24 +368,15 @@ static int em_create_pd(struct device *dev, int nr_states,
>   
>          pd->default_table = default_table;
>   
> -       runtime_table = kzalloc(sizeof(*runtime_table), GFP_KERNEL);
> -       if (!runtime_table) {
> -               kfree(default_table);
> -               kfree(pd);
> -               return -ENOMEM;
> -       }
> -
>          ret = em_create_perf_table(dev, pd, nr_states, cb, flags);
>          if (ret) {
>                  kfree(default_table);
> -               kfree(runtime_table);
>                  kfree(pd);
>                  return ret;
>          }
>   
> -       /* Re-use temporally (till 1st modification) the memory */
> -       runtime_table->state = default_table->state;
> -       rcu_assign_pointer(pd->runtime_table, runtime_table);
> +       /* Initialize runtime table as default table */
> +       rcu_assign_pointer(pd->runtime_table, default_table);
>   
>          if (_is_cpu_device(dev))
>                  for_each_cpu(cpu, cpus) {

I'll have to modify the unregister function to be aligned with this
approach as well.