Re: [PATCH 10/17] PM: EM: Add runtime update interface to modify EM power

[Lukasz Luba]

Hi Pierre,

On 4/11/23 16:40, Pierre Gondois wrote:
> Hello Lukasz,
>
> On 3/14/23 11:33, Lukasz Luba wrote:
> > Add an interface which allows to modify EM power data at runtime.
> > The new power information is populated by the provided callback, which
> > is called for each performance state. The CPU frequencies' efficiency is
> > re-calculated since that might be affected as well. The old EM memory
> > is going to be freed later using RCU mechanism.
> >
> > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > ---
> >   include/linux/energy_model.h |   8 +++
> >   kernel/power/energy_model.c  | 109 +++++++++++++++++++++++++++++++++++
> >   2 files changed, 117 insertions(+)
> >
> > diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> > index a616006a8130..e1772aa6c843 100644
> > --- a/include/linux/energy_model.h
> > +++ b/include/linux/energy_model.h
> > @@ -202,6 +202,8 @@ struct em_data_callback {
> >   struct em_perf_domain *em_cpu_get(int cpu);
> >   struct em_perf_domain *em_pd_get(struct device *dev);
> > +int em_dev_update_perf_domain(struct device *dev, struct 
> > em_data_callback *cb,
> > +                  void *priv);
> >   int em_dev_register_perf_domain(struct device *dev, unsigned int 
> > nr_states,
> >                   struct em_data_callback *cb, cpumask_t *span,
> >                   bool microwatts);
> > @@ -382,6 +384,12 @@ static inline int em_pd_nr_perf_states(struct 
> > em_perf_domain *pd)
> >   {
> >       return 0;
> >   }
> > +static inline
> > +int em_dev_update_perf_domain(struct device *dev, struct 
> > em_data_callback *cb,
> > +                  void *priv)
> > +{
> > +    return -EINVAL;
> > +}
> >   #endif
> >   #endif
> > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
> > index 87962b877376..e0e8fba3d02b 100644
> > --- a/kernel/power/energy_model.c
> > +++ b/kernel/power/energy_model.c
>
> [snip]
>
> > @@ -531,9 +628,21 @@ void em_dev_unregister_perf_domain(struct device 
> > *dev)
> >       tmp = pd->runtime_table;
> > +    /*
> > +     * Safely destroy runtime modifiable EM. By using the call
> > +     * synchronize_rcu() we make sure we don't progress till last user
> > +     * finished the RCU section and our update got applied.
> > +     */
> >       rcu_assign_pointer(pd->runtime_table, NULL);
> >       synchronize_rcu();
> > +    /*
> > +     * After the sync no updates will be in-flight, so free the old
> > +     * memory.
> > +     */
> > +    if (tmp->state != pd->table)
> > +        kfree(tmp->state);
> > +
>
> NIT: I think that the call 'kfree(pd->default_table->state)' which is 
> done in
> the patch:
>    PM: EM: Refactor struct em_perf_domain and add default_table
> should be done here, otherwise this bit of memory is not freed.

In this patch 10/17 there is no 'default_table' field yet, so cannot
be freed in this patch's code.


> Regards,
> Pierre
>
>
> >       kfree(tmp);
> >       kfree(dev->em_pd->table);

^^^^ in this current code we have the clean-up.
Here we clean the dev->em_pd->table, which is our conceptual
'default_table' in current code (before refactoring in 13/17)


In the patch 13/17 that you was referring to, there is also similar
but new cleaning process:
------------------->8---------------------------
-	kfree(dev->em_pd->table);
+	kfree(pd->default_table->state);
+	kfree(pd->default_table);
------------------8<----------------------------

So, it should be good.

Regards,
Lukasz