Messages in this thread | | | Date | Thu, 16 Jul 2020 09:36:50 +0800 | From | Chen Yu <> | Subject | Re: [PATCH 2/2][RESEND v3] PM-runtime: change the tracepoints to cover all usage_count |
| |
On Wed, Jul 15, 2020 at 10:33:22AM +0200, Greg Kroah-Hartman wrote: > On Wed, Jul 15, 2020 at 04:18:38PM +0800, Chen Yu wrote: > > Hi Greg, > > thanks very much for taking a look, > > On Wed, Jul 15, 2020 at 09:06:14AM +0200, Greg Kroah-Hartman wrote: > > > On Wed, Jul 15, 2020 at 02:28:03PM +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 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 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. Besides, all usage changes will > > > > be shown using rpm_usage even if included by other trace points. > > > > And these changes has helped track down the e1000e runtime issue. > > > > > > > > Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > > > --- > > > > drivers/base/power/runtime.c | 38 +++++++++++++++++++++++------------- > > > > 1 file changed, 24 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > > > index 85a248e196ca..5789d2624513 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); > > > > > > Why not just call trace everywhere before you do the atomic operations? > > > Why does the trace need to be called after the operation everywhere? > > > > > If I understand correctly, besides Michal's comments, if we put the trace > > before the atomic operation, we might be unable to judge whether the counter > > is going to increase or decrease from rpmflags: it is RPM_GET_PUT which combine > > the get() and put() together, then it is a little inconvenient for tracking IMO. > > A trace can never know the exact value of an atomic value as it could > change right before or after the trace function is called, right? > > So why are you caring about that? Care about the functionality that is > happening, not a reference count that you do not control at all. > Ah I see, thanks for the explanation, I'll re-think about the scenaio.
Thanks, Chenyu > thanks, > > greg k-h
| |