linux-kernel - Re: [PATCH v5 10/23] PM: EM: Add API for memory allocations for new tables

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9f7ec4e9-b076-4ba2-af27-1ed74e0355a7@arm.com>
Date: Tue, 19 Dec 2023 08:45:04 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: Qais Yousef <qyousef@...alina.io>
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, wvw@...gle.com
Subject: Re: [PATCH v5 10/23] PM: EM: Add API for memory allocations for new
 tables



On 12/17/23 17:59, Qais Yousef wrote:
> On 11/29/23 11:08, Lukasz Luba 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@....com>
>> ---
>>   include/linux/energy_model.h | 11 +++++++++
>>   kernel/power/energy_model.c  | 44 ++++++++++++++++++++++++++++++++++--
>>   2 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index 94a77a813724..e785211828fe 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
>>    * @state:	List of performance states, in ascending order
>>    */
>>   struct em_perf_table {
>>   	struct rcu_head rcu;
>> +	struct kref refcount;
>>   	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
>> @@ -368,6 +373,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 489287666705..489a358b9a00 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(runtime_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_inc_usage(struct em_perf_table __rcu *table)
>> +{
>> +	kref_get(&table->refcount);
>> +}
>> +
>> +static void em_dec_usage(struct em_perf_table __rcu *table)
>> +{
>> +	kref_put(&table->refcount, em_release_table_kref);
>> +}
> 
> nit: em_table_inc/dec() instead? matches general theme elsewhere in the code
> base.

Looks good, I will change it.

> 
>> +
>> +/**
>> + * 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_dec_usage(table);
>> +}
>> +
>> +/**
>> + * em_allocate_table() - Handles safe allocation of the new EM table
>> + * @table : EM memory which is going to be freed
>> + *
>> + * Increments the reference counter to mark that there is an owner of that
>> + * 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,12 @@ 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;
>> +
>> +	kref_init(&table->refcount);
>> +	em_inc_usage(table);
> 
> Doesn't kref_init() initialize to the count to 1 already? Is the em_inc_usage()
> needed here?

Good catch this is not needed here. Thanks!