linux-kernel - Re: [PATCH v6 12/23] PM: EM: Add helpers to read under RCU lock the EM table

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 for Android: free password hash cracker in your pocket

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

Date: Wed, 10 Jan 2024 14:06:25 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.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, qyousef@...alina.io,
 wvw@...gle.com
Subject: Re: [PATCH v6 12/23] PM: EM: Add helpers to read under RCU lock the
 EM table



On 1/4/24 19:55, Rafael J. Wysocki wrote:
> On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@....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@....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
>> --
>