| Message ID | 20230314103357.26010-11-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Introduce runtime modifiable Energy Model | expand |
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. Regards, Pierre > kfree(tmp); > > kfree(dev->em_pd->table);
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
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
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 @@ -205,6 +205,101 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table, return 0; } +/** + * em_dev_update_perf_domain() - Update run-time EM table for a device + * @dev : Device for which the EM is to be updated + * @cb : Callback function providing the power data for the EM + * @priv : Pointer to private data useful for passing context + * which might be required while calling @cb + * + * Update EM run-time modifiable table for a @dev using the callback + * defined in @cb. The EM new power values are then used for calculating + * the em_perf_state::cost for associated performance state. + * + * This function uses mutex to serialize writers, so it must not be called + * from non-sleeping context. + * + * Return 0 on success or a proper error in case of failure. + */ +int em_dev_update_perf_domain(struct device *dev, struct em_data_callback *cb, + void *priv) +{ + struct em_perf_table *runtime_table; + unsigned long power, freq; + struct em_perf_domain *pd; + int ret, i; + + if (!cb || !cb->update_power) + return -EINVAL; + + /* + * The lock serializes update and unregister code paths. When the + * EM has been unregistered in the meantime, we should capture that + * when entering this critical section. It also makes sure that + * two concurrent updates will be serialized. + */ + mutex_lock(&em_pd_mutex); + + if (!dev || !dev->em_pd) { + ret = -EINVAL; + goto unlock_em; + } + + pd = dev->em_pd; + + runtime_table = kzalloc(sizeof(*runtime_table), GFP_KERNEL); + if (!runtime_table) { + ret = -ENOMEM; + goto unlock_em; + } + + runtime_table->state = kcalloc(pd->nr_perf_states, + sizeof(struct em_perf_state), + GFP_KERNEL); + if (!runtime_table->state) { + ret = -ENOMEM; + goto free_runtime_table; + } + + /* Populate runtime table with updated values using driver callback */ + for (i = 0; i < pd->nr_perf_states; i++) { + freq = pd->table[i].frequency; + runtime_table->state[i].frequency = freq; + + /* + * Call driver callback to get a new power value for + * a given frequency. + */ + ret = cb->update_power(dev, freq, &power, priv); + if (ret) { + dev_dbg(dev, "EM: run-time update error: %d\n", ret); + goto free_runtime_state_table; + } + + runtime_table->state[i].power = power; + } + + ret = em_compute_costs(dev, runtime_table->state, cb, + pd->nr_perf_states, pd->flags); + if (ret) + goto free_runtime_state_table; + + em_perf_runtime_table_set(dev, runtime_table); + + mutex_unlock(&em_pd_mutex); + return 0; + +free_runtime_state_table: + kfree(runtime_table->state); +free_runtime_table: + kfree(runtime_table); +unlock_em: + mutex_unlock(&em_pd_mutex); + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(em_dev_update_perf_domain); + static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, int nr_states, struct em_data_callback *cb, unsigned long flags) @@ -524,6 +619,8 @@ void em_dev_unregister_perf_domain(struct device *dev) * The mutex separates all register/unregister requests and protects * from potential clean-up/setup issues in the debugfs directories. * The debugfs directory name is the same as device's name. + * The lock also protects the updater of the runtime modifiable + * EM and this remover. */ mutex_lock(&em_pd_mutex); @@ -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); + kfree(tmp); kfree(dev->em_pd->table);
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(+)