| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0iebSOT--AiP-9-CYwqtTe7+kRddryJ3DdvFb3WUeji7w@mail.gmail.com>
Date: Tue, 26 Sep 2023 21:48:20 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Lukasz Luba <lukasz.luba@....com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
rafael@...nel.org, dietmar.eggemann@....com, rui.zhang@...el.com,
amit.kucheria@...durent.com, amit.kachhap@...il.com,
daniel.lezcano@...aro.org, viresh.kumar@...aro.org,
len.brown@...el.com, pavel@....cz, mhiramat@...nel.org,
qyousef@...alina.io, wvw@...gle.com
Subject: Re: [PATCH v4 11/18] PM: EM: Add runtime update interface to modify
EM power
On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@....com> wrote:
First off, I would merge this with the previous patch, as the changes
would be much clearer then IMO.
> 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.
But it all starts with copying the frequencies from the default table.
> 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.
Not all of it, but the old runtime table that is not going to be used any more.
> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
> ---
> include/linux/energy_model.h | 8 +++
> kernel/power/energy_model.c | 111 +++++++++++++++++++++++++++++++++++
> 2 files changed, 119 insertions(+)
>
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index 740e7c25cfff..8f055ab356ed 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -201,6 +201,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);
> @@ -384,6 +386,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 2345837bfd2c..78e1495dc87e 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c
> @@ -172,6 +172,101 @@ static int em_compute_costs(struct device *dev, struct em_perf_state *table,
> return 0;
> }
>
> +/**
> + * em_dev_update_perf_domain() - Update runtime 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
It is still unclear to me who is going to use this priv pointer and how.
> + *
> + * Update EM runtime 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.
It actually allocates a new runtime table and populates it from
scratch, using the frequencies from the default table and the
callback.
> + *
> + * This function uses mutex to serialize writers, so it must not be called
"a mutex"
> + * 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) {
Checking dev against NULL under the mutex is pointless (either it is
NULL or it isn't, so check it earlier).
> + ret = -EINVAL;
> + goto unlock_em;
> + }
> +
> + pd = dev->em_pd;
And I would check pd against NULL here.
> +
> + 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;
> + }
The above allocations can be merged into one and allocating memory
under the mutex is questionable.
> +
> + /* Populate runtime table with updated values using driver callback */
> + for (i = 0; i < pd->nr_perf_states; i++) {
> + freq = pd->default_table->state[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: runtime 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)
> @@ -494,6 +589,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);
>
> @@ -501,9 +598,23 @@ void em_dev_unregister_perf_domain(struct device *dev)
>
> runtime_table = 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
> + * memory allocated for runtime table (if there was such).
> + */
> + if (runtime_table != pd->default_table) {
> + kfree(runtime_table->state);
> + kfree(runtime_table);
> + }
Can't this race with the RCU callback freeing the runtime table?
> +
> kfree(pd->default_table->state);
> kfree(pd->default_table);
> kfree(dev->em_pd);
> --
Powered by blists - more mailing lists