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

[Pierre Gondois]

Hi Lukasz,

On 5/10/23 08:55, Lukasz Luba wrote:
> 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.

I copy/pasted the statement:
   'kfree(pd->default_table->state)'
but I meant that the dynamic/runtime 'state' structure is freed, but the
'state' structure belonging to the default table is not freed. I.e. there
should be the following call:
   'kfree(pd->table->state)'
in this patch, which would be updated to
   'kfree(pd->default_table->state)'
in the patch:
   PM: EM: Refactor struct em_perf_domain and add default_table

Ultimately, all the memory is freed with all the patches applied, so this
is just a NIT about re-ordering (if this comment is indeed accurate).

> > >        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

Regards,
Pierre