Messages in this thread | | | Date | Sat, 6 Jun 2020 15:14:59 +0800 | From | Chen Yu <> | Subject | Re: [PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all usage_count changes |
| |
Hi, On Fri, Jun 05, 2020 at 09:33:11PM +0200, Michal Miroslaw wrote: > On Sat, Jun 06, 2020 at 03:05:52AM +0800, Chen Yu wrote: > > Commit d229290689ae ("PM-runtime: add tracepoints for usage_count changes") > > has added some tracepoints to monitor the change of runtime usage, and > > there is something to improve: > > 1. There are some places that adjust the usage count have not > > been traced yet. For example, pm_runtime_get_noresume() and > > pm_runtime_put_noidle() > > 2. The change of the usage count will not be tracked if decreased > > from 1 to 0. > [...] > > @@ -1448,16 +1453,17 @@ EXPORT_SYMBOL_GPL(pm_runtime_forbid); > > */ > > void pm_runtime_allow(struct device *dev) > > { > > + bool is_zero; > > + > > spin_lock_irq(&dev->power.lock); > > if (dev->power.runtime_auto) > > goto out; > > > > dev->power.runtime_auto = true; > > - if (atomic_dec_and_test(&dev->power.usage_count)) > > + is_zero = atomic_dec_and_test(&dev->power.usage_count); > > + trace_rpm_usage_rcuidle(dev, RPM_AUTO | RPM_ASYNC); > > + if (is_zero) > > rpm_idle(dev, RPM_AUTO | RPM_ASYNC); > > - else > > - trace_rpm_usage_rcuidle(dev, RPM_AUTO | RPM_ASYNC); > > - > [...] > > IIRC, rpm_idle() has a tracepoint already. > Yes, this is what I concerned previously. If someone want to track the change of usage_count, then he might have to enable both trace rpm_usage and rpm_idle so as to track the moment when the counter drops from 1 to 0. It might be more consistent if we only enable trace rpm_usage to track the whole process. > > @@ -1523,9 +1529,8 @@ static void update_autosuspend(struct device *dev, int old_delay, int old_use) > > /* If it used to be allowed then prevent it. */ > > if (!old_use || old_delay >= 0) { > > atomic_inc(&dev->power.usage_count); > > - rpm_resume(dev, 0); > > - } else { > > trace_rpm_usage_rcuidle(dev, 0); > > + rpm_resume(dev, 0); > > } > > } > [...] > > This actually changes logic, so it doesn't match the patch description. > This patch intends to adjust the logic to be consistent with the change of usage_counter, that is to say, only after the counter has been possibly modified, we record it. In current logic above, it tracks the usage count where the latter does not change.
Thanks, Chenyu > Best Regards > Michał Mirosław
| |