Re: [PATCH v6 10/23] PM: EM: Add API for memory allocations for new tables

From: Lukasz Luba
Date: Wed Jan 10 2024 - 09:08:30 EST




On 1/4/24 19:35, Rafael J. Wysocki wrote:
On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:

The runtime modified EM table can be provided from drivers. Create
mechanism which allows safely allocate and free the table for device
drivers. The same table can be used by the EAS in task scheduler code
paths, so make sure the memory is not freed when the device driver module
is unloaded.

Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
---
include/linux/energy_model.h | 11 +++++++++
kernel/power/energy_model.c | 43 ++++++++++++++++++++++++++++++++++--
2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index 5f842da3bb0c..753d70d0ce7e 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -5,6 +5,7 @@
#include <linux/device.h>
#include <linux/jump_label.h>
#include <linux/kobject.h>
+#include <linux/kref.h>
#include <linux/rcupdate.h>
#include <linux/sched/cpufreq.h>
#include <linux/sched/topology.h>
@@ -39,10 +40,12 @@ struct em_perf_state {
/**
* struct em_perf_table - Performance states table
* @rcu: RCU used for safe access and destruction
+ * @refcount: Reference count to track the owners

"Reference counter to track the users"

Also it is not really just a counter - it provides the memory release
mechanism too.

Agree


* @state: List of performance states, in ascending order
*/
struct em_perf_table {
struct rcu_head rcu;
+ struct kref refcount;

So I would just call it kref.

OK


struct em_perf_state state[];
};

@@ -184,6 +187,8 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
struct em_data_callback *cb, cpumask_t *span,
bool microwatts);
void em_dev_unregister_perf_domain(struct device *dev);
+struct em_perf_table __rcu *em_allocate_table(struct em_perf_domain *pd);
+void em_free_table(struct em_perf_table __rcu *table);

/**
* em_pd_get_efficient_state() - Get an efficient performance state from the EM
@@ -365,6 +370,12 @@ static inline int em_pd_nr_perf_states(struct em_perf_domain *pd)
{
return 0;
}
+static inline
+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) {}
#endif

#endif
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index c03010084208..bbc406db0be1 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -114,12 +114,46 @@ static void em_destroy_table_rcu(struct rcu_head *rp)
kfree(table);
}

-static void em_free_table(struct em_perf_table __rcu *table)
+static void em_release_table_kref(struct kref *kref)
{
+ struct em_perf_table __rcu *table;
+
+ /* It was the last owner of this table so we can free */
+ table = container_of(kref, struct em_perf_table, refcount);
+
call_rcu(&table->rcu, em_destroy_table_rcu);
}

-static struct em_perf_table __rcu *
+static inline void em_table_inc(struct em_perf_table __rcu *table)

Why not em_table_get()?

I will change that...


+{
+ kref_get(&table->refcount);
+}
+
+static void em_table_dec(struct em_perf_table __rcu *table)

And em_table_put() here?

and this as well.


Note that I do realize that the "put" and "get" terminology is used in
one of the subsequent patches - I'll get to it later.

+{
+ kref_put(&table->refcount, em_release_table_kref);
+}
+
+/**
+ * em_free_table() - Handles safe free of the EM table when needed
+ * @table : EM memory which is going to be freed
+ *
+ * No return values.
+ */
+void em_free_table(struct em_perf_table __rcu *table)
+{
+ em_table_dec(table);
+}

Why is this necessary? The function called by it could be extern
instead and wrapped into a static inline wrapper in a header (if you
really need the alias).

Sounds good, I'll change it.


+
+/**
+ * em_allocate_table() - Handles safe allocation of the new EM table

" - Allocate a new EM table"

OK


And I would call this em_table_alloc() and (maybe) add an
em_table_free() alias for em_table_put().

OK


+ * @table : EM memory which is going to be freed

So the argument is called "pd" and it is not a table. It is also used
for computing the size of the new table AFAICS.

True, good catch.


+ *
+ * Increments the reference counter to mark that there is an owner of that

"Allocate a new EM table and initialize its kref to indicate that it
has a user."

OK


+ * EM table. That might be a device driver module or EAS.
+ * Returns allocated table or error.
+ */
+struct em_perf_table __rcu *
em_allocate_table(struct em_perf_domain *pd)
{
struct em_perf_table __rcu *table;
@@ -128,6 +162,11 @@ em_allocate_table(struct em_perf_domain *pd)
table_size = sizeof(struct em_perf_state) * pd->nr_perf_states;

table = kzalloc(sizeof(*table) + table_size, GFP_KERNEL);
+ if (!table)
+ return table;

I would return NULL from here explicitly.

OK


+
+ kref_init(&table->refcount);
+
return table;
}

--