diff mbox series

[10/17] PM: EM: Add runtime update interface to modify EM power

Message ID 20230314103357.26010-11-lukasz.luba@arm.com
State Superseded
Headers show
Series Introduce runtime modifiable Energy Model | expand

Commit Message

Lukasz Luba March 14, 2023, 10:33 a.m. UTC
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. 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.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 include/linux/energy_model.h |   8 +++
 kernel/power/energy_model.c  | 109 +++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

Comments

Pierre Gondois April 11, 2023, 3:40 p.m. UTC | #1
Hello Lukasz,

On 3/14/23 11:33, Lukasz Luba wrote:
> 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. 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.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>   include/linux/energy_model.h |   8 +++
>   kernel/power/energy_model.c  | 109 +++++++++++++++++++++++++++++++++++
>   2 files changed, 117 insertions(+)
> 
> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
> index a616006a8130..e1772aa6c843 100644
> --- a/include/linux/energy_model.h
> +++ b/include/linux/energy_model.h
> @@ -202,6 +202,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);
> @@ -382,6 +384,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 87962b877376..e0e8fba3d02b 100644
> --- a/kernel/power/energy_model.c
> +++ b/kernel/power/energy_model.c

[snip]

> @@ -531,9 +628,21 @@ void em_dev_unregister_perf_domain(struct device *dev)
>   
>   	tmp = 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 old
> +	 * memory.
> +	 */
> +	if (tmp->state != pd->table)
> +		kfree(tmp->state);
> +

NIT: I think that the call 'kfree(pd->default_table->state)' which is done in
the patch:
   PM: EM: Refactor struct em_perf_domain and add default_table
should be done here, otherwise this bit of memory is not freed.

Regards,
Pierre


>   	kfree(tmp);
>   
>   	kfree(dev->em_pd->table);
Lukasz Luba May 10, 2023, 6:55 a.m. UTC | #2
Hi Pierre,

On 4/11/23 16:40, Pierre Gondois wrote:
> Hello Lukasz,
> 
> On 3/14/23 11:33, Lukasz Luba wrote:
>> 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. 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.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   include/linux/energy_model.h |   8 +++
>>   kernel/power/energy_model.c  | 109 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 117 insertions(+)
>>
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index a616006a8130..e1772aa6c843 100644
>> --- a/include/linux/energy_model.h
>> +++ b/include/linux/energy_model.h
>> @@ -202,6 +202,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);
>> @@ -382,6 +384,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 87962b877376..e0e8fba3d02b 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
> 
> [snip]
> 
>> @@ -531,9 +628,21 @@ void em_dev_unregister_perf_domain(struct device 
>> *dev)
>>       tmp = 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 old
>> +     * memory.
>> +     */
>> +    if (tmp->state != pd->table)
>> +        kfree(tmp->state);
>> +
> 
> NIT: I think that the call 'kfree(pd->default_table->state)' which is 
> done in
> the patch:
>    PM: EM: Refactor struct em_perf_domain and add default_table
> should be done here, otherwise this bit of memory is not freed.

In this patch 10/17 there is no 'default_table' field yet, so cannot
be freed in this patch's code.


> 
> Regards,
> Pierre
> 
> 
>>       kfree(tmp);
>>       kfree(dev->em_pd->table);

^^^^ in this current code we have the clean-up.
Here we clean the dev->em_pd->table, which is our conceptual
'default_table' in current code (before refactoring in 13/17)


In the patch 13/17 that you was referring to, there is also similar
but new cleaning process:
------------------->8---------------------------
-	kfree(dev->em_pd->table);
+	kfree(pd->default_table->state);
+	kfree(pd->default_table);
------------------8<----------------------------

So, it should be good.

Regards,
Lukasz
Pierre Gondois May 15, 2023, 8:48 a.m. UTC | #3
Hi Lukasz,

On 5/10/23 08:55, Lukasz Luba wrote:
> Hi Pierre,
> 
> On 4/11/23 16:40, Pierre Gondois wrote:
>> Hello Lukasz,
>>
>> On 3/14/23 11:33, Lukasz Luba wrote:
>>> 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. 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.
>>>
>>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>>> ---
>>>    include/linux/energy_model.h |   8 +++
>>>    kernel/power/energy_model.c  | 109 +++++++++++++++++++++++++++++++++++
>>>    2 files changed, 117 insertions(+)
>>>
>>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>>> index a616006a8130..e1772aa6c843 100644
>>> --- a/include/linux/energy_model.h
>>> +++ b/include/linux/energy_model.h
>>> @@ -202,6 +202,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);
>>> @@ -382,6 +384,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 87962b877376..e0e8fba3d02b 100644
>>> --- a/kernel/power/energy_model.c
>>> +++ b/kernel/power/energy_model.c
>>
>> [snip]
>>
>>> @@ -531,9 +628,21 @@ void em_dev_unregister_perf_domain(struct device
>>> *dev)
>>>        tmp = 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 old
>>> +     * memory.
>>> +     */
>>> +    if (tmp->state != pd->table)
>>> +        kfree(tmp->state);
>>> +
>>
>> NIT: I think that the call 'kfree(pd->default_table->state)' which is
>> done in
>> the patch:
>>     PM: EM: Refactor struct em_perf_domain and add default_table
>> should be done here, otherwise this bit of memory is not freed.
> 
> In this patch 10/17 there is no 'default_table' field yet, so cannot
> be freed in this patch's code.

I copy/pasted the statement:
    'kfree(pd->default_table->state)'
but I meant that the dynamic/runtime 'state' structure is freed, but the
'state' structure belonging to the default table is not freed. I.e. there
should be the following call:
    'kfree(pd->table->state)'
in this patch, which would be updated to
    'kfree(pd->default_table->state)'
in the patch:
    PM: EM: Refactor struct em_perf_domain and add default_table

Ultimately, all the memory is freed with all the patches applied, so this
is just a NIT about re-ordering (if this comment is indeed accurate).

> 
> 
>>>        kfree(tmp);
>>>        kfree(dev->em_pd->table);
> 
> ^^^^ in this current code we have the clean-up.
> Here we clean the dev->em_pd->table, which is our conceptual
> 'default_table' in current code (before refactoring in 13/17)
> 
> 
> In the patch 13/17 that you was referring to, there is also similar
> but new cleaning process:
> ------------------->8---------------------------
> -	kfree(dev->em_pd->table);
> +	kfree(pd->default_table->state);
> +	kfree(pd->default_table);
> ------------------8<----------------------------
> 
> So, it should be good.
> 
> Regards,
> Lukasz

Regards,
Pierre
diff mbox series

Patch

diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index a616006a8130..e1772aa6c843 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -202,6 +202,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);
@@ -382,6 +384,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 87962b877376..e0e8fba3d02b 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -205,6 +205,101 @@  static int em_compute_costs(struct device *dev, struct em_perf_state *table,
 	return 0;
 }
 
+/**
+ * em_dev_update_perf_domain() - Update run-time 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
+ *
+ * Update EM run-time 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.
+ *
+ * This function uses mutex to serialize writers, so it must not be called
+ * 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) {
+		ret = -EINVAL;
+		goto unlock_em;
+	}
+
+	pd = dev->em_pd;
+
+	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;
+	}
+
+	/* Populate runtime table with updated values using driver callback */
+	for (i = 0; i < pd->nr_perf_states; i++) {
+		freq = pd->table[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: run-time 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)
@@ -524,6 +619,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);
 
@@ -531,9 +628,21 @@  void em_dev_unregister_perf_domain(struct device *dev)
 
 	tmp = 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 old
+	 * memory.
+	 */
+	if (tmp->state != pd->table)
+		kfree(tmp->state);
+
 	kfree(tmp);
 
 	kfree(dev->em_pd->table);