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

[Qais Yousef <qyousef@layalina.io>]

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@arm.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.

> +
> +/**
> + * 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?


Cheers

--
Qais Yousef

> +
>  	return table;
>  }
>  
> -- 
> 2.25.1
>