[PATCH v8 4/8] PM / EM: add support for other devices than CPUs in Energy Model

[Lukasz Luba]

On 6/3/20 4:40 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba <lukasz.luba at arm.com> wrote:
>>
>>
>>
>> On 6/3/20 4:13 PM, Rafael J. Wysocki wrote:
>>> On Tue, Jun 2, 2020 at 1:31 PM Lukasz Luba <lukasz.luba at arm.com> wrote:
>>>>
>>>> Hi Daniel,
>>>>
>>>> On 6/1/20 10:44 PM, Daniel Lezcano wrote:
>>>>> On 27/05/2020 11:58, Lukasz Luba wrote:
>>>>>> Add support for other devices than CPUs. The registration function
>>>>>> does not require a valid cpumask pointer and is ready to handle new
>>>>>> devices. Some of the internal structures has been reorganized in order to
>>>>>> keep consistent view (like removing per_cpu pd pointers).
>>>>>>
>>>>>> Signed-off-by: Lukasz Luba <lukasz.luba at arm.com>
>>>>>> ---
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(em_register_perf_domain);
>>>>>> +
>>>>>> +/**
>>>>>> + * em_dev_unregister_perf_domain() - Unregister Energy Model (EM) for a device
>>>>>> + * @dev             : Device for which the EM is registered
>>>>>> + *
>>>>>> + * Try to unregister the EM for the specified device (but not a CPU).
>>>>>> + */
>>>>>> +void em_dev_unregister_perf_domain(struct device *dev)
>>>>>> +{
>>>>>> +    if (IS_ERR_OR_NULL(dev) || !dev->em_pd)
>>>>>> +            return;
>>>>>> +
>>>>>> +    if (_is_cpu_device(dev))
>>>>>> +            return;
>>>>>> +
>>>>>> +    mutex_lock(&em_pd_mutex);
>>>>>
>>>>> Is the mutex really needed?
>>>>
>>>> I just wanted to align this unregister code with register. Since there
>>>> is debugfs dir lookup and the device's EM existence checks I thought it
>>>> wouldn't harm just to lock for a while and make sure the registration
>>>> path is not used. These two paths shouldn't affect each other, but with
>>>> modules loading/unloading I wanted to play safe.
>>>>
>>>> I can change it maybe to just dmb() and the end of the function if it's
>>>> a big performance problem in this unloading path. What do you think?
>>>
>>> I would rather leave the mutex locking as is.
>>>
>>> However, the question to ask is what exactly may go wrong without that
>>> locking in place?  Is there any specific race condition that you are
>>> concerned about?
>>>
>>
>> I tried to test this with module loading & unloading with panfrost
>> driver and CPU hotplug (which should bail out quickly) and was OK.
>> I don't see any particular race. I don't too much about the
>> debugfs code, though. That's why I tried to protect from some
>> scripts/services which try to re-load the driver.
>>
>> Apart from that, maybe just this dev->em = NULL to be populated to all
>> CPUs, which mutex_unlock synchronizes for free here.
> 
> If it may run concurrently with the registration for the same device,
> the locking is necessary, but in that case the !dev->em_pd check needs
> to go under the mutex too IMO, or you may end up leaking the pd if the
> registration can run between that check and the point at which the
> mutex is taken.

They don't run concurrently for the same device and users of that EM are
already gone.
I just wanted to be sure that everything is cleaned and synced properly.
Here is some example of the directories under
/sys/kernel/debug/energy_model
cpu0, cpu4, gpu, dsp, etc

The only worry that I had was the debugfs dir name, which is a
string from dev_name() and will be the same for the next registration
if module is re-loaded. So the 'name' is reused and debugfs_create_dir()
and debugfs_remove_recursive() uses this fsnotify, but they are
operating under inode_lock/unlock() on the parent dir 'energy_model'.
Then there is also this debugfs_lookup() which is slightly different.

That's why I put a mutex to separate all registration and unregistration
for all devices.
It should work without the mutex in unregister path, but I think it does
not harm to take it just in case and also have the CPU variable sync for
free.


> 
> Apart from this your kerneldoc comments might me improved IMO.
> 
> First of all, you can use @dev inside of a kerneldoc (if @dev
> represents an argument of the documented function) and that will
> produce the right output automatically.

OK

> 
> Second, it is better to avoid saying things like "Try to unregister
> ..." in kerneldoc comments (the "Try to" part is redundant).  Simply
> say "Unregister ..." instead.

Good point, thanks, I will use "Unregister ..." then.

> 
> Thanks!
> 

Shell I send a 'resend patch' which changes these @dev and
'unregister' comments?
Or wait for you to finish reviewing the other patches and send v9?