linux-kernel - Re: [PATCH v6 11/23] PM: EM: Add API for updating the runtime modifiable EM

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] [day] [month] [year] [list]

Date: Wed, 10 Jan 2024 14:11:57 +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 11/23] PM: EM: Add API for updating the runtime
 modifiable EM



On 1/4/24 19:47, Rafael J. Wysocki wrote:
> I don't really like using the API TLA in patch subjects, because it
> does not really say much.  IMO a subject like this would be better:
> 
> "PM: EM: Introduce em_dev_update_perf_domain() for EM updates"

Fair enough

> 
> On Thu, Jan 4, 2024 at 6:15 PM Lukasz Luba <lukasz.luba@....com> wrote:
>>
>> Add API function em_dev_update_perf_domain() which allows to safely
>> change the EM.
> 
> "... which allows the EM to be changed safely."

OK

> 
> New paragraph:
> 
>> The concurrent modifiers are protected by the mutex
>> to serialize them. Removal of the old memory is asynchronous and
>> handled by the RCU mechanisms.
> 
> "Concurrent updaters are serialized with a mutex and the removal of
> memory that will not be used any more is carried out with the help of
> RCU."

OK

> 
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
>> ---
>>   include/linux/energy_model.h |  8 +++++++
>>   kernel/power/energy_model.c  | 41 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 49 insertions(+)
>>
>> diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
>> index 753d70d0ce7e..f33257ed83fd 100644
>> --- a/include/linux/energy_model.h
>> +++ b/include/linux/energy_model.h
>> @@ -183,6 +183,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_perf_table __rcu *new_table);
>>   int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>>                                  struct em_data_callback *cb, cpumask_t *span,
>>                                  bool microwatts);
>> @@ -376,6 +378,12 @@ 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) {}
>> +static inline
>> +int em_dev_update_perf_domain(struct device *dev,
>> +                             struct em_perf_table __rcu *new_table)
>> +{
>> +       return -EINVAL;
>> +}
>>   #endif
>>
>>   #endif
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index bbc406db0be1..496dc00835c6 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -220,6 +220,47 @@ static int em_allocate_perf_table(struct em_perf_domain *pd,
>>          return 0;
>>   }
>>
>> +/**
>> + * em_dev_update_perf_domain() - Update runtime EM table for a device
>> + * @dev                : Device for which the EM is to be updated
>> + * @table      : The new EM table that is going to be used from now
> 
> This is called "new_table" below.

good catch

> 
>> + *
>> + * Update EM runtime modifiable table for the @dev using the provided @table.
>> + *
>> + * This function uses mutex to serialize writers, so it must not be called
> 
> "uses a mutex"

OK

> 
>> + * from non-sleeping context.
> 
> "a non-sleeping context".

OK

> 
>> + *
>> + * Return 0 on success or a proper error in case of failure.
> 
> It is not clear what "a proper error" means.  It would be better to
> simply say "or an error code on failure" IMO.

Agree, I'll change it.

> 
>> + */
>> +int em_dev_update_perf_domain(struct device *dev,
>> +                             struct em_perf_table __rcu *new_table)
>> +{
>> +       struct em_perf_table __rcu *old_table;
>> +       struct em_perf_domain *pd;
>> +
>> +       /* Serialize update/unregister or concurrent updates */
>> +       mutex_lock(&em_pd_mutex);
>> +
>> +       if (!dev || !dev->em_pd) {
> 
> dev need not be checked under the lock.

True, I will put it about the lock.

> 
>> +               mutex_unlock(&em_pd_mutex);
>> +               return -EINVAL;
>> +       }
>> +       pd = dev->em_pd;
>> +
>> +       em_table_inc(new_table);
>> +
>> +       old_table = pd->em_table;
>> +       rcu_assign_pointer(pd->em_table, new_table);
>> +
>> +       em_cpufreq_update_efficiencies(dev, new_table->state);
>> +
>> +       em_table_dec(old_table);
>> +
>> +       mutex_unlock(&em_pd_mutex);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(em_dev_update_perf_domain);
>> +
>>   static int em_create_runtime_table(struct em_perf_domain *pd)
>>   {
>>          struct em_perf_table __rcu *table;
>> --


Thank you for the review!

Regards,
Lukasz