lkml.org 
[lkml]   [2023]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 09/18] PM: EM: Introduce runtime modifiable table
From


On 9/29/23 13:27, Rafael J. Wysocki wrote:
> On Fri, Sep 29, 2023 at 11:15 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>>

[snip]

>>>> em_debug_remove_pd(dev);
>>>>
>>>> + runtime_table = pd->runtime_table;
>>>> +
>>>> + rcu_assign_pointer(pd->runtime_table, NULL);
>>>> + synchronize_rcu();
>>>
>>> Is it really a good idea to call this under a mutex?
>>
>> This is the unregistration of the EM code path, so a thermal
>> driver which gets some IRQs might not be aware that the EM
>> is going to vanish. That's why those two code paths: update
>> & unregister are protected with the same lock.
>>
>> This synchronize_rcu() won't be long,
>
> Are you sure? This potentially waits for all of the CPUs in the
> system to go through a quiescent state which may take a while in
> principle.
>
> In any case, though, this effectively makes everyone waiting for the
> mutex also wait for the grace period to elapse and they may not care
> about it.

My apologies for the delay, I had to check this. Yes, should be possible
and safe to not wait here as you described on this synchronize_rcu().

What I have drawn in other response to patch 11/18 [1] should still be
true.

Thanks, I will remove this sync call from here.

>
>> but makes sure that when we free(dev->em_pd) we don't leak runtime_table.
>>
>>>
>>>> +
>>>> kfree(pd->default_table->state);
>>>> kfree(pd->default_table);
>>>> kfree(dev->em_pd);
>>>> +
>>>
>>> Unrelated change.
>>
>> ACK
>>
>>>
>>>> dev->em_pd = NULL;
>>>> mutex_unlock(&em_pd_mutex);
>>>> }
>>>> --
>>>
>>> So this really adds a pointer to a table that can be dynamically
>>> updated to struct em_perf_domain without any users so far. It is not
>>> used anywhere as of this patch AFAICS, which is not what the changelog
>>> is saying.
>>
>> Good catch. I will adjust the changlog in header and say:
>>
>> 'Add infrastructure and mechanisms for the new runtime table.
>> The runtime modifiable EM data is used by the Energy Aware Scheduler
>> (EAS)for the task placement.
>
> I would make it more clear that this is going to happen after some
> other subsequent changes.
>

OK, I will add that information too.

[1]
https://lore.kernel.org/lkml/91d6e9be-d50c-d157-55a0-79134cbd01fb@arm.com/

\
 
 \ /
  Last update: 2023-10-06 10:03    [W:0.103 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site