[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a3907ec0-7e20-e3a5-3814-476a25e1efaa@arm.com>
Date: Fri, 29 Sep 2023 11:00:24 +0100
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 v4 11/18] PM: EM: Add runtime update interface to modify
EM power
On 9/26/23 20:48, Rafael J. Wysocki wrote:
> On Mon, Sep 25, 2023 at 10:11 AM Lukasz Luba <lukasz.luba@....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@....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.
Powered by blists - more mailing lists