Messages in this thread | | | Date | Wed, 10 May 2023 07:55:23 +0100 | Subject | Re: [PATCH 10/17] PM: EM: Add runtime update interface to modify EM power | From | Lukasz Luba <> |
| |
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.
> > 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
| |