Extend trace point to cover all runtime usage count

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 changes intended.

Signed-off-by Chen Yu <[email protected]>

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

drivers/base/power/runtime.c +1 -0

index 9f62790f644c..85a248e196ca 100644
@@ -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)

drivers/base/power/runtime.c +1 -0

+
 static bool pm_runtime_need_not_resume(struct device *dev)
 {
        return atomic_read(&dev->power.usage_count) <= 1 &&

include/linux/pm_runtime.h +2 -11

index 3dbc207bff53..26510fef2acd 100644
@@ -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
--

2.17.1

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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644

Quoted 28 lines · 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 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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644
@@ -1004,10 +1004,11 @@ int __pm_runtime_idle(struct device *dev, int rpmflags)
 	int retval;
 

drivers/base/power/runtime.c int __pm_runtime_idle(struct device *dev, int rpmflags) · line 1009

Quoted 6 lines

drivers/base/power/runtime.c +3 -2

 	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?

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · 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 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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644
@@ -1004,10 +1004,11 @@ int __pm_runtime_idle(struct device *dev, int rpmflags)
 	int retval;
 

Quoted 9 lines

drivers/base/power/runtime.c int __pm_runtime_idle(struct device *dev, int rpmflags) · line 1009

Quoted 6 lines

drivers/base/power/runtime.c +3 -2

 	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?

I would argue that this is easier mentally: We trace what state the device is in from now on (a "current state" for the time being) instead of tracing what it was before (an information that has just expired).

Quoted 28 lines · 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 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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644
@@ -1004,10 +1004,11 @@ int __pm_runtime_idle(struct device *dev, int rpmflags)
 	int retval;
 

drivers/base/power/runtime.c int __pm_runtime_idle(struct device *dev, int rpmflags) · line 1009

Quoted 6 lines

drivers/base/power/runtime.c +3 -2

 	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?

thanks,

greg k-h

Hi Greg,
thanks very much for taking a look,

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · 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 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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644
@@ -1004,10 +1004,11 @@ int __pm_runtime_idle(struct device *dev, int rpmflags)
 	int retval;
 

Quoted 10 lines

drivers/base/power/runtime.c int __pm_runtime_idle(struct device *dev, int rpmflags) · line 1009

Quoted 6 lines

drivers/base/power/runtime.c +3 -2

 	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.

Quoted 42 lines · Chen Yu

Hi Greg,
thanks very much for taking a look,

Quoted 29 lines · Greg Kroah-Hartman

Quoted 28 lines · 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 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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644
@@ -1004,10 +1004,11 @@ int __pm_runtime_idle(struct device *dev, int rpmflags)
 	int retval;
 

Quoted 10 lines

drivers/base/power/runtime.c int __pm_runtime_idle(struct device *dev, int rpmflags) · line 1009

Quoted 6 lines

drivers/base/power/runtime.c +3 -2

 	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?

Quoted 4 lines

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.

thanks,

greg k-h

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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644

drivers/base/power/runtime.c +1 -1

+               if (non_zero)
                        return 0;
-               }
        }
 
        might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe);

drivers/base/power/runtime.c +2 -1

+               if (non_zero)
                        return 0;
-               }
+
        }
 
        might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe);

drivers/base/power/runtime.c +3 -0

+       trace_rpm_usage_rcuidle(dev, 0);
        rpm_resume(dev, 0);
 
  out:

@@ -1448,16 +1454,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)

drivers/base/power/runtime.c +4 -4

-
  out:
        spin_unlock_irq(&dev->power.lock);
 }

@@ -1523,9 +1530,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 +1539,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);

drivers/base/power/runtime.c +1 -1

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

Quoted 27 lines · 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 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 <[email protected]>

Signed-off-by Chen Yu <[email protected]>

---
 drivers/base/power/runtime.c | 38 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

drivers/base/power/runtime.c +0 -0

index 85a248e196ca..5789d2624513 100644

drivers/base/power/runtime.c int __pm_runtime_idle(struct device *dev, int rpmflags) · line 1009

Quoted 8 lines

drivers/base/power/runtime.c +3 -2

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

It looks like you could move the trace event before the atomic variable check.

The ordering between the two doesn't matter, because usage_count may change between the check and the trace event anyway.

But then what is the trace event useful for in the first place?

Quoted 7 lines

drivers/base/power/runtime.c +1 -1

+               if (non_zero)
                        return 0;
-               }
        }
 
        might_sleep_if(!(rpmflags & RPM_ASYNC) && !dev->power.irq_safe);

drivers/base/power/runtime.c int __pm_runtime_suspend(struct device *dev, int rpmflags) · line 1044

Quoted 8 lines

drivers/base/power/runtime.c +3 -2

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

And the same comments apply here.

Quoted 11 lines

drivers/base/power/runtime.c +2 -1

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

drivers/base/power/runtime.c int __pm_runtime_resume(struct device *dev, int rpmflags) · line 1080

Quoted 3 lines

drivers/base/power/runtime.c +1 -1

-       if (rpmflags & RPM_GET_PUT)
+       if (rpmflags & RPM_GET_PUT) {
                atomic_inc(&dev->power.usage_count);

So the reason why things like that don't work is because the atomic variable can change again between the inc and the trace event.

drivers/base/power/runtime.c void pm_runtime_forbid(struct device *dev) · line 1440

Quoted 9 lines

drivers/base/power/runtime.c +2 -0

+               trace_rpm_usage_rcuidle(dev, rpmflags);
+       }
 
        spin_lock_irqsave(&dev->power.lock, flags);
        retval = rpm_resume(dev, rpmflags);

@@ -1433,6 +1438,7 @@ void pm_runtime_forbid(struct device *dev)
 
        dev->power.runtime_auto = false;
        atomic_inc(&dev->power.usage_count);

Analogously here.

Quoted 12 lines

drivers/base/power/runtime.c +3 -0

+       trace_rpm_usage_rcuidle(dev, 0);
        rpm_resume(dev, 0);
 
  out:

@@ -1448,16 +1454,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)

drivers/base/power/runtime.c EXPORT_SYMBOL_GPL(pm_runtime_forbid); · line 1459

Quoted 10 lines

drivers/base/power/runtime.c +3 -3

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

The change of ordering is pointless for the reasons outlined above.

And so on.

Quoted 25 lines

drivers/base/power/runtime.c +4 -4

-
  out:
        spin_unlock_irq(&dev->power.lock);
 }

@@ -1523,9 +1530,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 +1539,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);
+               }
 

drivers/base/power/runtime.c void pm_runtime_drop_link(struct device *dev) · line 1753

Quoted 8 lines

drivers/base/power/runtime.c +1 -0

                /* Maybe we can autosuspend now. */
                rpm_idle(dev, RPM_AUTO);

@@ -1741,12 +1749,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);
 }

This actually kind of makes sense, as a matter of tracing the pm_runtime_get_noresume() usage, but not as a matter of tracing the atomic variable value.

Quoted 10 lines

drivers/base/power/runtime.c +1 -1

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