diff mbox series

[v6,12/23] PM: EM: Add helpers to read under RCU lock the EM table

Message ID 20240104171553.2080674-13-lukasz.luba@arm.com
State New
Headers show
Series Introduce runtime modifiable Energy Model | expand

Commit Message

Lukasz Luba Jan. 4, 2024, 5:15 p.m. UTC
To use the runtime modifiable EM table there is a need to use RCU
read locking properly. Add helper functions for the device drivers and
frameworks to make sure it's done properly.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
 include/linux/energy_model.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Lukasz Luba Jan. 10, 2024, 2:06 p.m. UTC | #1
On 1/4/24 19:55, Rafael J. Wysocki wrote:
> On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> To use the runtime modifiable EM table there is a need to use RCU
>> read locking properly. Add helper functions for the device drivers and
>> frameworks to make sure it's done properly.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   include/linux/energy_model.h | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index f33257ed83fd..cfaf5d8b1aad 100644
>> --- a/include/linux/energy_model.h
>> +++ b/include/linux/energy_model.h
>> @@ -338,6 +338,20 @@ static inline int em_pd_nr_perf_states(struct em_perf_domain *pd)
>>          return pd->nr_perf_states;
>>   }
>>
>> +static inline struct em_perf_state *em_get_table(struct em_perf_domain *pd)
>> +{
>> +       struct em_perf_table __rcu *table;
>> +
>> +       rcu_read_lock();
>> +       table = rcu_dereference(pd->em_table);
>> +       return table->state;
>> +}
>> +
>> +static inline void em_put_table(void)
>> +{
>> +       rcu_read_unlock();
>> +}
> 
> The lack of symmetry between em_get_table() and em_put_table() is kind
> of confusing.
> 
> I don't really like these wrappers.
> 
> IMO it would be better to use rcu_read_lock()/rcu_read_unlock()
> directly everywhere they are needed and there can be a wrapper around
> rcu_dereference(pd->em_table), something like
> 
> static inline struct em_perf_state *em_perf_state_from_pd(struct
> em_perf_domain *pd)
> {
>          return rcu_dereference(pd->em_table)->state;
> }

Fair enough, I will change this and use explicit rcu_read_lock/unlock()
in the thermal/DTPM code together with this above function.
I will add comment to it that it needs to be called under the RCU read
section locked.

Then also it would be easier to handle the function names in patch 10/23
that you have also commented.

> 
>> +
>>   #else
>>   struct em_data_callback {};
>>   #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb) { }
>> @@ -384,6 +398,11 @@ int em_dev_update_perf_domain(struct device *dev,
>>   {
>>          return -EINVAL;
>>   }
>> +static inline struct em_perf_state *em_get_table(struct em_perf_domain *pd)
>> +{
>> +       return NULL;
>> +}
>> +static inline void em_put_table(void) {}
>>   #endif
>>
>>   #endif
>> --
>
diff mbox series

Patch

diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index f33257ed83fd..cfaf5d8b1aad 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -338,6 +338,20 @@  static inline int em_pd_nr_perf_states(struct em_perf_domain *pd)
 	return pd->nr_perf_states;
 }
 
+static inline struct em_perf_state *em_get_table(struct em_perf_domain *pd)
+{
+	struct em_perf_table __rcu *table;
+
+	rcu_read_lock();
+	table = rcu_dereference(pd->em_table);
+	return table->state;
+}
+
+static inline void em_put_table(void)
+{
+	rcu_read_unlock();
+}
+
 #else
 struct em_data_callback {};
 #define EM_ADV_DATA_CB(_active_power_cb, _cost_cb) { }
@@ -384,6 +398,11 @@  int em_dev_update_perf_domain(struct device *dev,
 {
 	return -EINVAL;
 }
+static inline struct em_perf_state *em_get_table(struct em_perf_domain *pd)
+{
+	return NULL;
+}
+static inline void em_put_table(void) {}
 #endif
 
 #endif