Messages in this thread |  | | From | "Rafael J. Wysocki" <> | | Date | Thu, 4 Jan 2024 20:47:19 +0100 | | Subject | Re: [PATCH v6 11/23] PM: EM: Add API for updating the runtime modifiable EM |
| |
I don't really like using the API TLA in patch subjects, because it does not really say much. IMO a subject like this would be better:
"PM: EM: Introduce em_dev_update_perf_domain() for EM updates"
On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > Add API function em_dev_update_perf_domain() which allows to safely > change the EM.
"... which allows the EM to be changed safely."
New paragraph:
> The concurrent modifiers are protected by the mutex > to serialize them. Removal of the old memory is asynchronous and > handled by the RCU mechanisms.
"Concurrent updaters are serialized with a mutex and the removal of memory that will not be used any more is carried out with the help of RCU."
> > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > include/linux/energy_model.h | 8 +++++++ > kernel/power/energy_model.c | 41 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h > index 753d70d0ce7e..f33257ed83fd 100644 > --- a/include/linux/energy_model.h > +++ b/include/linux/energy_model.h > @@ -183,6 +183,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_perf_table __rcu *new_table); > int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states, > struct em_data_callback *cb, cpumask_t *span, > bool microwatts); > @@ -376,6 +378,12 @@ struct em_perf_table __rcu *em_allocate_table(struct em_perf_domain *pd) > return NULL; > } > static inline void em_free_table(struct em_perf_table __rcu *table) {} > +static inline > +int em_dev_update_perf_domain(struct device *dev, > + struct em_perf_table __rcu *new_table) > +{ > + return -EINVAL; > +} > #endif > > #endif > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c > index bbc406db0be1..496dc00835c6 100644 > --- a/kernel/power/energy_model.c > +++ b/kernel/power/energy_model.c > @@ -220,6 +220,47 @@ static int em_allocate_perf_table(struct em_perf_domain *pd, > return 0; > } > > +/** > + * em_dev_update_perf_domain() - Update runtime EM table for a device > + * @dev : Device for which the EM is to be updated > + * @table : The new EM table that is going to be used from now
This is called "new_table" below.
> + * > + * Update EM runtime modifiable table for the @dev using the provided @table. > + * > + * This function uses mutex to serialize writers, so it must not be called
"uses a mutex"
> + * from non-sleeping context.
"a non-sleeping context".
> + * > + * Return 0 on success or a proper error in case of failure.
It is not clear what "a proper error" means. It would be better to simply say "or an error code on failure" IMO.
> + */ > +int em_dev_update_perf_domain(struct device *dev, > + struct em_perf_table __rcu *new_table) > +{ > + struct em_perf_table __rcu *old_table; > + struct em_perf_domain *pd; > + > + /* Serialize update/unregister or concurrent updates */ > + mutex_lock(&em_pd_mutex); > + > + if (!dev || !dev->em_pd) {
dev need not be checked under the lock.
> + mutex_unlock(&em_pd_mutex); > + return -EINVAL; > + } > + pd = dev->em_pd; > + > + em_table_inc(new_table); > + > + old_table = pd->em_table; > + rcu_assign_pointer(pd->em_table, new_table); > + > + em_cpufreq_update_efficiencies(dev, new_table->state); > + > + em_table_dec(old_table); > + > + mutex_unlock(&em_pd_mutex); > + return 0; > +} > +EXPORT_SYMBOL_GPL(em_dev_update_perf_domain); > + > static int em_create_runtime_table(struct em_perf_domain *pd) > { > struct em_perf_table __rcu *table; > --
|  |