[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <27520bc9-21f4-b4d6-3159-39542a93cfca@arm.com>
Date: Mon, 15 May 2023 10:48:03 +0200
From: Pierre Gondois <pierre.gondois@....com>
To: Lukasz Luba <lukasz.luba@....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 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@....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
Powered by blists - more mailing lists