Received-SPF: pass (google.com: domain of linux-kernel+bounces-22323-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <14e5634b-721a-4444-8825-967d807b71b7@arm.com>
Date: Wed, 10 Jan 2024 14:11:57 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 11/23] PM: EM: Add API for updating the runtime
 modifiable EM
Content-Language: en-US
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
 dietmar.eggemann@arm.com, rui.zhang@intel.com, amit.kucheria@verdurent.com,
 amit.kachhap@gmail.com, daniel.lezcano@linaro.org, viresh.kumar@linaro.org,
 len.brown@intel.com, pavel@ucw.cz, mhiramat@kernel.org, qyousef@layalina.io,
 wvw@google.com
References: <20240104171553.2080674-1-lukasz.luba@arm.com>
 <20240104171553.2080674-12-lukasz.luba@arm.com>
 <CAJZ5v0hqRkDmhRBfB4g-2EH2piv-KOQdwad7rVoSK8FzZKg=TA@mail.gmail.com>
From: Lukasz Luba <lukasz.luba@arm.com>
In-Reply-To: <CAJZ5v0hqRkDmhRBfB4g-2EH2piv-KOQdwad7rVoSK8FzZKg=TA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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