| Message ID | 20230512095743.3393563-10-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Introduce runtime modifiable Energy Model | expand |
On 12/05/2023 11:57, Lukasz Luba wrote: > The EM is going to support runtime modifications of the power data. > In order to achieve that prepare the internal mechanism. This patch > introduces RCU safe mechanism to clean up the old allocated EM data. s/In order to achieve that prepare the internal mechanism. This patch introduces/Introduce ... much shorter, same information. [...] > +static void em_perf_runtime_table_set(struct device *dev, > + struct em_perf_table *runtime_table) > +{ > + struct em_perf_domain *pd = dev->em_pd; > + struct em_perf_table *tmp; > + > + tmp = pd->runtime_table; > + > + rcu_assign_pointer(pd->runtime_table, runtime_table); > + > + em_cpufreq_update_efficiencies(dev, runtime_table->state); > + > + if (trace_em_perf_state_enabled()) { > + unsigned long freq, power, cost, flags; > + int i; > + > + for (i = 0; i < pd->nr_perf_states; i++) { > + freq = runtime_table->state[i].frequency; > + power = runtime_table->state[i].power; > + cost = runtime_table->state[i].cost; > + flags = runtime_table->state[i].flags; > + > + trace_em_perf_state(dev_name(dev), pd->nr_perf_states, > + i, freq, power, cost, flags); > + } > + } > + > + /* > + * Check if the 'state' array is not actually the one from setup. > + * If it is then don't free it. > + */ > + if (tmp->state == pd->table) It's unfortunate that you have the refactoring in 13/17 which would lead to: if (pd->runtime_table>state == pd->default_table->state) ^^^^^^^^^^^^^ ^^^^^^^^^^^^^ so people would immediately grasp one of the core concepts of the design: non-modifiable default_table and modifiable runtime_table. I still belief that it would be the better idea to do the refactoring first. [...]
On 5/30/23 11:02, Dietmar Eggemann wrote: > On 12/05/2023 11:57, Lukasz Luba wrote: >> The EM is going to support runtime modifications of the power data. >> In order to achieve that prepare the internal mechanism. This patch >> introduces RCU safe mechanism to clean up the old allocated EM data. > > s/In order to achieve that prepare the internal mechanism. This patch > introduces/Introduce ... much shorter, same information. OK > > [...] > >> +static void em_perf_runtime_table_set(struct device *dev, >> + struct em_perf_table *runtime_table) >> +{ >> + struct em_perf_domain *pd = dev->em_pd; >> + struct em_perf_table *tmp; >> + >> + tmp = pd->runtime_table; >> + >> + rcu_assign_pointer(pd->runtime_table, runtime_table); >> + >> + em_cpufreq_update_efficiencies(dev, runtime_table->state); >> + >> + if (trace_em_perf_state_enabled()) { >> + unsigned long freq, power, cost, flags; >> + int i; >> + >> + for (i = 0; i < pd->nr_perf_states; i++) { >> + freq = runtime_table->state[i].frequency; >> + power = runtime_table->state[i].power; >> + cost = runtime_table->state[i].cost; >> + flags = runtime_table->state[i].flags; >> + >> + trace_em_perf_state(dev_name(dev), pd->nr_perf_states, >> + i, freq, power, cost, flags); >> + } >> + } >> + >> + /* >> + * Check if the 'state' array is not actually the one from setup. >> + * If it is then don't free it. >> + */ >> + if (tmp->state == pd->table) > > It's unfortunate that you have the refactoring in 13/17 which would lead to: > > if (pd->runtime_table>state == pd->default_table->state) > ^^^^^^^^^^^^^ ^^^^^^^^^^^^^ > > so people would immediately grasp one of the core concepts of the > design: non-modifiable default_table and modifiable runtime_table. > > I still belief that it would be the better idea to do the refactoring first. > > [...] That make sense. Let me try that approach as see how the patch set would look like.
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c index 39d47028ef3d..bc9b7dec0763 100644 --- a/kernel/power/energy_model.c +++ b/kernel/power/energy_model.c @@ -26,6 +26,9 @@ */ static DEFINE_MUTEX(em_pd_mutex); +static void em_cpufreq_update_efficiencies(struct device *dev, + struct em_perf_state *table); + static bool _is_cpu_device(struct device *dev) { return (dev->bus == &cpu_subsys); @@ -106,6 +109,60 @@ static void em_debug_create_pd(struct device *dev) {} static void em_debug_remove_pd(struct device *dev) {} #endif +static void em_destroy_rt_table_rcu(struct rcu_head *rp) +{ + struct em_perf_table *runtime_table; + + runtime_table = container_of(rp, struct em_perf_table, rcu); + kfree(runtime_table->state); + kfree(runtime_table); +} + +static void em_destroy_tmp_setup_rcu(struct rcu_head *rp) +{ + struct em_perf_table *runtime_table; + + runtime_table = container_of(rp, struct em_perf_table, rcu); + kfree(runtime_table); +} + +static void em_perf_runtime_table_set(struct device *dev, + struct em_perf_table *runtime_table) +{ + struct em_perf_domain *pd = dev->em_pd; + struct em_perf_table *tmp; + + tmp = pd->runtime_table; + + rcu_assign_pointer(pd->runtime_table, runtime_table); + + em_cpufreq_update_efficiencies(dev, runtime_table->state); + + if (trace_em_perf_state_enabled()) { + unsigned long freq, power, cost, flags; + int i; + + for (i = 0; i < pd->nr_perf_states; i++) { + freq = runtime_table->state[i].frequency; + power = runtime_table->state[i].power; + cost = runtime_table->state[i].cost; + flags = runtime_table->state[i].flags; + + trace_em_perf_state(dev_name(dev), pd->nr_perf_states, + i, freq, power, cost, flags); + } + } + + /* + * Check if the 'state' array is not actually the one from setup. + * If it is then don't free it. + */ + if (tmp->state == pd->table) + call_rcu(&tmp->rcu, em_destroy_tmp_setup_rcu); + else + call_rcu(&tmp->rcu, em_destroy_rt_table_rcu); +} + static int em_compute_costs(struct device *dev, struct em_perf_state *table, struct em_data_callback *cb, int nr_states, unsigned long flags)
The EM is going to support runtime modifications of the power data. In order to achieve that prepare the internal mechanism. This patch introduces RCU safe mechanism to clean up the old allocated EM data. It also adds a mutex for the EM structure to serialize the modifiers. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- kernel/power/energy_model.c | 57 +++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)