lkml.org 
[lkml]   [2023]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v4 11/18] PM: EM: Add runtime update interface to modify EM power
From


On 9/26/23 20:48, Rafael J. Wysocki wrote:
> On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> First off, I would merge this with the previous patch, as the changes
> would be much clearer then IMO.

I was trying to avoid a big patch ~150 lines. I will do that merge.

>
>> 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.
>
> But it all starts with copying the frequencies from the default table.

Yes, I can add that to the description.

>
>> 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.
>
> Not all of it, but the old runtime table that is not going to be used any more.

True, I will rephrase that, to make it more precised.

>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>

[snip]

>>
>> +/**
>> + * em_dev_update_perf_domain() - Update runtime 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
>
> It is still unclear to me who is going to use this priv pointer and how.

I have explained that in some previous patch response. A driver or
kernel module which monitors the thermal situation and has context.

>
>> + *
>> + * Update EM runtime 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.
>
> It actually allocates a new runtime table and populates it from
> scratch, using the frequencies from the default table and the
> callback.

Yes, it allocated new table and put the updated power values there.
I can add that to the comment.

>
>> + *
>> + * This function uses mutex to serialize writers, so it must not be called
>
> "a mutex"

ACK

>
>> + * from non-sleeping context.

[snip]

>> +
>> + if (!dev || !dev->em_pd) {
>
> Checking dev against NULL under the mutex is pointless (either it is
> NULL or it isn't, so check it earlier).

ACK

>
>> + ret = -EINVAL;
>> + goto unlock_em;
>> + }
>> +
>> + pd = dev->em_pd;
>
> And I would check pd against NULL here.

It's done above, next to '!dev || !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;
>> + }
>
> The above allocations can be merged into one and allocating memory
> under the mutex is questionable.

So how to make sure that there is no 2 callers trying to update the
same EM or unregistration is not in the background?

[snip]

>>
>> @@ -501,9 +598,23 @@ void em_dev_unregister_perf_domain(struct device *dev)
>>
>> runtime_table = 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
>> + * memory allocated for runtime table (if there was such).
>> + */
>> + if (runtime_table != pd->default_table) {
>> + kfree(runtime_table->state);
>> + kfree(runtime_table);
>> + }
>
> Can't this race with the RCU callback freeing the runtime table?

That's why there is this 'synchronize_rcu()' above and the mutex. The
updating caller if finished the update, would unlock the mutex and this
unregister code can go. Here we call the synchronize_rcu() so we assure
the callback has finished for the update path and than we explicitly
free the saved 'runtime_table' here. So all data should be freed and
code serialized in those two paths.

\
 
 \ /
  Last update: 2023-09-29 12:00    [W:0.103 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site