On 6/3/20 5:22 PM, Rafael J. Wysocki wrote:
On Wed, Jun 3, 2020 at 6:12 PM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
On 6/3/20 4:40 PM, Rafael J. Wysocki wrote:
On Wed, Jun 3, 2020 at 5:26 PM Lukasz Luba <lukasz.luba@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
---
[ ... ]
}
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.
OK, so that needs to be explained in a comment.
OK, I will add it.
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
Well, fair enough, but I still think that the !dev->em_pd check should
be done under the mutex or it will be confusing.
it just in case and also have the CPU variable sync for free.
I'm not sure what you mean by the last part here?
The mutex_unlock for me also means dmb() took place. ARM has slightly
different memory model than x86 and I just wanted to be sure that
this new values reach memory and become visible to other cores.
mutex_unlock just guaranties this for me.
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?
Yes, please, but see the comments above too.
Saw it
Or wait for you to finish reviewing the other patches and send v9?
That is not necessary, unless you want to make kerneldoc improvements
in the other patches.
I will check them, but if they are OKish then I will just resend this
one.
Thank you for the review.
Regards,
Lukasz
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
