| Message ID | 6ce5c2d21758363b7c9a31187eda1787bc4a6160.1591380524.git.yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add more trace point for runtime usage count | expand |
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. > @@ -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. Best Regards Michał Mirosław
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
On Sat, Jun 06, 2020 at 03:14:59PM +0800, Chen Yu wrote: > 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. I see now what you intended. I think it would be nice to put the idea (that all usage changes be shown using rpm_usage even if included in other trace points) into the commit message. Otherwise, looks ok. Best Regards Michał Mirosław
On Sun, Jun 07, 2020 at 06:55:35AM +0200, Michal Miroslaw wrote: > On Sat, Jun 06, 2020 at 03:14:59PM +0800, Chen Yu wrote: > > 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. > > I see now what you intended. I think it would be nice to put the idea > (that all usage changes be shown using rpm_usage even if included in > other trace points) into the commit message. Otherwise, looks ok. > Okay, will do in next version, thanks! Chenyu
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 85a248e196ca..4aa2b5aa0bb8 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1004,10 +1004,11 @@ int __pm_runtime_idle(struct device *dev, int rpmflags) int retval; if (rpmflags & RPM_GET_PUT) { - if (!atomic_dec_and_test(&dev->power.usage_count)) { - trace_rpm_usage_rcuidle(dev, rpmflags); + bool non_zero = !atomic_dec_and_test(&dev->power.usage_count); + + trace_rpm_usage_rcuidle(dev, rpmflags); + if (non_zero) return 0; - } } might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); @@ -1038,10 +1039,12 @@ int __pm_runtime_suspend(struct device *dev, int rpmflags) int retval; if (rpmflags & RPM_GET_PUT) { - if (!atomic_dec_and_test(&dev->power.usage_count)) { - trace_rpm_usage_rcuidle(dev, rpmflags); + bool non_zero = !atomic_dec_and_test(&dev->power.usage_count); + + trace_rpm_usage_rcuidle(dev, rpmflags); + if (non_zero) return 0; - } + } might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe); @@ -1073,8 +1076,10 @@ int __pm_runtime_resume(struct device *dev, int rpmflags) might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe && dev->power.runtime_status != RPM_ACTIVE); - if (rpmflags & RPM_GET_PUT) + if (rpmflags & RPM_GET_PUT) { atomic_inc(&dev->power.usage_count); + trace_rpm_usage_rcuidle(dev, rpmflags); + } spin_lock_irqsave(&dev->power.lock, flags); retval = rpm_resume(dev, rpmflags); @@ -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); - out: spin_unlock_irq(&dev->power.lock); } @@ -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); } } @@ -1533,8 +1538,10 @@ static void update_autosuspend(struct device *dev, int old_delay, int old_use) else { /* If it used to be prevented then allow it. */ - if (old_use && old_delay < 0) + if (old_use && old_delay < 0) { atomic_dec(&dev->power.usage_count); + trace_rpm_usage_rcuidle(dev, 0); + } /* Maybe we can autosuspend now. */ rpm_idle(dev, RPM_AUTO); @@ -1741,12 +1748,14 @@ void pm_runtime_drop_link(struct device *dev) void pm_runtime_get_noresume(struct device *dev) { atomic_inc(&dev->power.usage_count); + trace_rpm_usage_rcuidle(dev, 0); } EXPORT_SYMBOL_GPL(pm_runtime_get_noresume); void pm_runtime_put_noidle(struct device *dev) { atomic_add_unless(&dev->power.usage_count, -1, 0); + trace_rpm_usage_rcuidle(dev, 0); } EXPORT_SYMBOL_GPL(pm_runtime_put_noidle);
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. This patch address above issues by tracking the usage count whenever it has been modified. And these changes has helped track down the e1000e runtime issue. Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- drivers/base/power/runtime.c | 37 ++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)