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

[Pierre Gondois <pierre.gondois@arm.com>]

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@arm.com>
>>> ---
>>>    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