[PATCH 0/2][RFC] Add more trace point for runtime usage count

[PATCH 0/2][RFC] Add more trace point for runtime usage count

[Chen Yu]

Currentlt some code flow of runtime usage count changes is not covered by 
the tracepoints. Add corresponding tracepoints to monitor all the usage_count
changes.


Chen Yu (2):
  PM-runtime: Move all runtime usage related function to runtime.c
  PM-runtime: add more tracepoints for usage_count changes

 drivers/base/power/runtime.c | 49 +++++++++++++++++++++++++-----------
 include/linux/pm_runtime.h   | 14 +++--------
 2 files changed, 39 insertions(+), 24 deletions(-)

-- 
2.20.1

[PATCH 1/2][RFC] PM-runtime: Move all runtime usage related function to runtime.c

[Chen Yu]

In order to track all the runtime usage count change, move the code
related to runtime usage count change from pm_runtime.h to runtime.c,
so that in runtime.c we can leverage trace event to do the tracking.
Meanwhile export pm_runtime_get_noresume() and pm_runtime_put_noidle()
so the module can use them.

No functional change.

Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/base/power/runtime.c | 12 ++++++++++++
 include/linux/pm_runtime.h   | 14 ++++----------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 9f62790f644c..85a248e196ca 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1738,6 +1738,18 @@ void pm_runtime_drop_link(struct device *dev)
 	spin_unlock_irq(&dev->power.lock);
 }
 
+void pm_runtime_get_noresume(struct device *dev)
+{
+	atomic_inc(&dev->power.usage_count);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_noresume);
+
+void pm_runtime_put_noidle(struct device *dev)
+{
+	atomic_add_unless(&dev->power.usage_count, -1, 0);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_put_noidle);
+
 static bool pm_runtime_need_not_resume(struct device *dev)
 {
 	return atomic_read(&dev->power.usage_count) <= 1 &&
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 3dbc207bff53..57afdb122d8a 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -59,6 +59,8 @@ extern void pm_runtime_get_suppliers(struct device *dev);
 extern void pm_runtime_put_suppliers(struct device *dev);
 extern void pm_runtime_new_link(struct device *dev);
 extern void pm_runtime_drop_link(struct device *dev);
+extern void pm_runtime_get_noresume(struct device *dev);
+extern void pm_runtime_put_noidle(struct device *dev);
 
 static inline int pm_runtime_get_if_in_use(struct device *dev)
 {
@@ -70,16 +72,6 @@ static inline void pm_suspend_ignore_children(struct device *dev, bool enable)
 	dev->power.ignore_children = enable;
 }
 
-static inline void pm_runtime_get_noresume(struct device *dev)
-{
-	atomic_inc(&dev->power.usage_count);
-}
-
-static inline void pm_runtime_put_noidle(struct device *dev)
-{
-	atomic_add_unless(&dev->power.usage_count, -1, 0);
-}
-
 static inline bool pm_runtime_suspended(struct device *dev)
 {
 	return dev->power.runtime_status == RPM_SUSPENDED
@@ -188,6 +180,8 @@ static inline void pm_runtime_get_suppliers(struct device *dev) {}
 static inline void pm_runtime_put_suppliers(struct device *dev) {}
 static inline void pm_runtime_new_link(struct device *dev) {}
 static inline void pm_runtime_drop_link(struct device *dev) {}
+static inline void pm_runtime_get_noresume(struct device *dev) {}
+static inline void pm_runtime_put_noidle(struct device *dev) {}
 
 #endif /* !CONFIG_PM */
 
-- 
2.20.1

Re: [PATCH 1/2][RFC] PM-runtime: Move all runtime usage related function to runtime.c

[Chen Yu]

On Sat, Jun 06, 2020 at 03:05:35AM +0800, Chen Yu wrote:
> In order to track all the runtime usage count change, move the code
> related to runtime usage count change from pm_runtime.h to runtime.c,
> so that in runtime.c we can leverage trace event to do the tracking.
> Meanwhile export pm_runtime_get_noresume() and pm_runtime_put_noidle()
> so the module can use them.
> 
> No functional change.
>
There is a compile issue found by lkp, will send a
new version out.

Thanks,
Chenyu

[PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all usage_count changes

[Chen Yu]

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(-)

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);
 
-- 
2.20.1

Re: [PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all usage_count changes

[Michal Miroslaw]

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

Re: [PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all usage_count changes

[Chen Yu]

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

Re: [PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all usage_count changes

[Michal Miroslaw]

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

Re: [PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all usage_count changes

[Chen Yu]

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