Messages in this thread | | | Date | Mon, 15 May 2023 10:48:03 +0200 | From | Pierre Gondois <> | 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@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
| |