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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <94e7bbaa-3a25-6634-f5af-fff4932a44b9@arm.com>
Date:   Wed, 10 May 2023 07:55:23 +0100
From:   Lukasz Luba <lukasz.luba@....com>
To:     Pierre Gondois <pierre.gondois@....com>
Cc:     dietmar.eggemann@....com, rui.zhang@...el.com,
        amit.kucheria@...durent.com, amit.kachhap@...il.com,
        daniel.lezcano@...aro.org, viresh.kumar@...aro.org,
        len.brown@...el.com, pavel@....cz, ionela.voinescu@....com,
        linux-pm@...r.kernel.org, rostedt@...dmis.org,
        linux-kernel@...r.kernel.org, rafael@...nel.org,
        mhiramat@...nel.org
Subject: Re: [PATCH 10/17] PM: EM: Add runtime update interface to modify EM
 power

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


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