Re: [PATCH v4 09/18] PM: EM: Introduce runtime modifiable table

[Lukasz Luba <lukasz.luba@arm.com>]

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/