diff mbox series

[RFC,v3,1/4] PM: Add a sysfs file to represent the total sleep duration

Message ID 20221115200156.12218-2-mario.limonciello@amd.com
State New
Headers
Series Make it easier to measure % in HW sleep state |

Commit Message

Mario Limonciello Nov. 15, 2022, 8:01 p.m. UTC 
  For userspace to be able to analyze how much of a suspend cycle was spent
in the hardware sleep states userspace software has to use kernel trace
points paired with the file `low_power_idle_system_residency_us` on
supported systems.

To make this information more discoverable, introduce a new sysfs file
to represent the duration spent in a sleep state.
This file will be present and updated during resume for all suspend
types.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
RFC v2->v3
 * Drop one of the sysfs files
 * Use sysfs_emit instead
 * Fix symbol name (s/type/time/)
 * Drop is_visible
 * Use timespec64 type for suspend stats
 * Update documentation
 * Update sysfs file name
---
 Documentation/ABI/testing/sysfs-power |  8 ++++++++
 include/linux/suspend.h               |  2 ++
 kernel/power/main.c                   | 15 +++++++++++++++
 kernel/power/suspend.c                |  2 ++
 kernel/time/timekeeping.c             |  2 ++
 5 files changed, 29 insertions(+)

Comments

John Stultz Nov. 15, 2022, 8:44 p.m. UTC | #1 
On Tue, Nov 15, 2022 at 12:02 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> For userspace to be able to analyze how much of a suspend cycle was spent
> in the hardware sleep states userspace software has to use kernel trace
> points paired with the file `low_power_idle_system_residency_us` on
> supported systems.
>
> To make this information more discoverable, introduce a new sysfs file
> to represent the duration spent in a sleep state.
> This file will be present and updated during resume for all suspend
> types.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> RFC v2->v3
>  * Drop one of the sysfs files
>  * Use sysfs_emit instead
>  * Fix symbol name (s/type/time/)
>  * Drop is_visible
>  * Use timespec64 type for suspend stats
>  * Update documentation
>  * Update sysfs file name
> ---
>  Documentation/ABI/testing/sysfs-power |  8 ++++++++
>  include/linux/suspend.h               |  2 ++
>  kernel/power/main.c                   | 15 +++++++++++++++
>  kernel/power/suspend.c                |  2 ++
>  kernel/time/timekeeping.c             |  2 ++
>  5 files changed, 29 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index f99d433ff311..3abe20c47e08 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -413,6 +413,14 @@ Description:
>                 The /sys/power/suspend_stats/last_failed_step file contains
>                 the last failed step in the suspend/resume path.
>
> +What:          /sys/power/suspend_stats/last_total
> +Date:          December 2022
> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
> +Description:
> +               The /sys/power/suspend_stats/last_total file contains
> +               the total duration of the sleep cycle.
> +               This is measured in microseconds.
> +

Nit/bikeshed: "last_total" seems less straightforward then it should
be? Maybe "total_suspend_time" instead?

> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 31ec4a9b9d70..f33012860699 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2003 Open Source Development Lab
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/export.h>
>  #include <linux/kobject.h>
>  #include <linux/string.h>
> @@ -54,6 +55,11 @@ void unlock_system_sleep(unsigned int flags)
>  }
>  EXPORT_SYMBOL_GPL(unlock_system_sleep);
>
> +void pm_account_suspend_time(const struct timespec64 t)
> +{
> +       suspend_stats.last_total = timespec64_add(suspend_stats.last_total, t);
> +}
> +
>  void ksys_sync_helper(void)
>  {
>         ktime_t start;
> @@ -377,6 +383,14 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
>  }
>  static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
>
> +static ssize_t last_total_show(struct kobject *kobj,
> +               struct kobj_attribute *attr, char *buf)
> +{
> +       return sysfs_emit(buf, "%llu\n",
> +                         timespec64_to_ns(&suspend_stats.last_total) / NSEC_PER_USEC);
> +}
> +static struct kobj_attribute last_total = __ATTR_RO(last_total);
> +
>  static struct attribute *suspend_attrs[] = {
>         &success.attr,
>         &fail.attr,
> @@ -391,6 +405,7 @@ static struct attribute *suspend_attrs[] = {
>         &last_failed_dev.attr,
>         &last_failed_errno.attr,
>         &last_failed_step.attr,
> +       &last_total.attr,
>         NULL,
>  };

While not identical, this has some overlap with the logic in
kernel/time/timekeeping_debug.c
I wonder if it would make sense to consolidate some of this accounting?

thanks
-john
Mario Limonciello Nov. 16, 2022, 2:40 a.m. UTC | #2 
On 11/15/2022 14:44, John Stultz wrote:
> On Tue, Nov 15, 2022 at 12:02 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> For userspace to be able to analyze how much of a suspend cycle was spent
>> in the hardware sleep states userspace software has to use kernel trace
>> points paired with the file `low_power_idle_system_residency_us` on
>> supported systems.
>>
>> To make this information more discoverable, introduce a new sysfs file
>> to represent the duration spent in a sleep state.
>> This file will be present and updated during resume for all suspend
>> types.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> RFC v2->v3
>>   * Drop one of the sysfs files
>>   * Use sysfs_emit instead
>>   * Fix symbol name (s/type/time/)
>>   * Drop is_visible
>>   * Use timespec64 type for suspend stats
>>   * Update documentation
>>   * Update sysfs file name
>> ---
>>   Documentation/ABI/testing/sysfs-power |  8 ++++++++
>>   include/linux/suspend.h               |  2 ++
>>   kernel/power/main.c                   | 15 +++++++++++++++
>>   kernel/power/suspend.c                |  2 ++
>>   kernel/time/timekeeping.c             |  2 ++
>>   5 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index f99d433ff311..3abe20c47e08 100644
>> --- a/Documentation/ABI/testing/sysfs-power
>> +++ b/Documentation/ABI/testing/sysfs-power
>> @@ -413,6 +413,14 @@ Description:
>>                  The /sys/power/suspend_stats/last_failed_step file contains
>>                  the last failed step in the suspend/resume path.
>>
>> +What:          /sys/power/suspend_stats/last_total
>> +Date:          December 2022
>> +Contact:       Mario Limonciello <mario.limonciello@amd.com>
>> +Description:
>> +               The /sys/power/suspend_stats/last_total file contains
>> +               the total duration of the sleep cycle.
>> +               This is measured in microseconds.
>> +
> 
> Nit/bikeshed: "last_total" seems less straightforward then it should
> be? Maybe "total_suspend_time" instead?

I initially thought the same thing but I feel like you can make the same 
argument about "success" or other files in the "suspend_stats" directory.

> 
>> diff --git a/kernel/power/main.c b/kernel/power/main.c
>> index 31ec4a9b9d70..f33012860699 100644
>> --- a/kernel/power/main.c
>> +++ b/kernel/power/main.c
>> @@ -6,6 +6,7 @@
>>    * Copyright (c) 2003 Open Source Development Lab
>>    */
>>
>> +#include <linux/acpi.h>
>>   #include <linux/export.h>
>>   #include <linux/kobject.h>
>>   #include <linux/string.h>
>> @@ -54,6 +55,11 @@ void unlock_system_sleep(unsigned int flags)
>>   }
>>   EXPORT_SYMBOL_GPL(unlock_system_sleep);
>>
>> +void pm_account_suspend_time(const struct timespec64 t)
>> +{
>> +       suspend_stats.last_total = timespec64_add(suspend_stats.last_total, t);
>> +}
>> +
>>   void ksys_sync_helper(void)
>>   {
>>          ktime_t start;
>> @@ -377,6 +383,14 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
>>   }
>>   static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
>>
>> +static ssize_t last_total_show(struct kobject *kobj,
>> +               struct kobj_attribute *attr, char *buf)
>> +{
>> +       return sysfs_emit(buf, "%llu\n",
>> +                         timespec64_to_ns(&suspend_stats.last_total) / NSEC_PER_USEC);
>> +}
>> +static struct kobj_attribute last_total = __ATTR_RO(last_total);
>> +
>>   static struct attribute *suspend_attrs[] = {
>>          &success.attr,
>>          &fail.attr,
>> @@ -391,6 +405,7 @@ static struct attribute *suspend_attrs[] = {
>>          &last_failed_dev.attr,
>>          &last_failed_errno.attr,
>>          &last_failed_step.attr,
>> +       &last_total.attr,
>>          NULL,
>>   };
> 
> While not identical, this has some overlap with the logic in
> kernel/time/timekeeping_debug.c
> I wonder if it would make sense to consolidate some of this accounting?

Sure.  If the consensus is to stick to this approach of exporting the 
total time I'll look into that.

> 
> thanks
> -john
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index f99d433ff311..3abe20c47e08 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -413,6 +413,14 @@  Description:
 		The /sys/power/suspend_stats/last_failed_step file contains
 		the last failed step in the suspend/resume path.
 
+What:		/sys/power/suspend_stats/last_total
+Date:		December 2022
+Contact:	Mario Limonciello <mario.limonciello@amd.com>
+Description:
+		The /sys/power/suspend_stats/last_total file contains
+		the total duration of the sleep cycle.
+		This is measured in microseconds.
+
 What:		/sys/power/sync_on_suspend
 Date:		October 2019
 Contact:	Jonas Meurer <jonas@freesources.org>
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index cfe19a028918..57f29ab4176c 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -68,6 +68,7 @@  struct suspend_stats {
 	int	last_failed_errno;
 	int	errno[REC_FAILED_NUM];
 	int	last_failed_step;
+	struct timespec64	last_total;
 	enum suspend_stat_step	failed_steps[REC_FAILED_NUM];
 };
 
@@ -489,6 +490,7 @@  void restore_processor_state(void);
 extern int register_pm_notifier(struct notifier_block *nb);
 extern int unregister_pm_notifier(struct notifier_block *nb);
 extern void ksys_sync_helper(void);
+extern void pm_account_suspend_time(const struct timespec64 t);
 
 #define pm_notifier(fn, pri) {				\
 	static struct notifier_block fn##_nb =			\
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 31ec4a9b9d70..f33012860699 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -6,6 +6,7 @@ 
  * Copyright (c) 2003 Open Source Development Lab
  */
 
+#include <linux/acpi.h>
 #include <linux/export.h>
 #include <linux/kobject.h>
 #include <linux/string.h>
@@ -54,6 +55,11 @@  void unlock_system_sleep(unsigned int flags)
 }
 EXPORT_SYMBOL_GPL(unlock_system_sleep);
 
+void pm_account_suspend_time(const struct timespec64 t)
+{
+	suspend_stats.last_total = timespec64_add(suspend_stats.last_total, t);
+}
+
 void ksys_sync_helper(void)
 {
 	ktime_t start;
@@ -377,6 +383,14 @@  static ssize_t last_failed_step_show(struct kobject *kobj,
 }
 static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
 
+static ssize_t last_total_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%llu\n",
+			  timespec64_to_ns(&suspend_stats.last_total) / NSEC_PER_USEC);
+}
+static struct kobj_attribute last_total = __ATTR_RO(last_total);
+
 static struct attribute *suspend_attrs[] = {
 	&success.attr,
 	&fail.attr,
@@ -391,6 +405,7 @@  static struct attribute *suspend_attrs[] = {
 	&last_failed_dev.attr,
 	&last_failed_errno.attr,
 	&last_failed_step.attr,
+	&last_total.attr,
 	NULL,
 };
 
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index fa3bf161d13f..cb0f72f52528 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -423,6 +423,8 @@  static int suspend_enter(suspend_state_t state, bool *wakeup)
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
+	suspend_stats.last_total = (struct timespec64) { 0, 0 };
+
 	if (state == PM_SUSPEND_TO_IDLE) {
 		s2idle_loop();
 		goto Platform_wake;
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index f72b9f1de178..f9c8cfef8fc5 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -24,6 +24,7 @@ 
 #include <linux/compiler.h>
 #include <linux/audit.h>
 #include <linux/random.h>
+#include <linux/suspend.h>
 
 #include "tick-internal.h"
 #include "ntp_internal.h"
@@ -1698,6 +1699,7 @@  static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
 	tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
 	tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
 	tk_debug_account_sleep_time(delta);
+	pm_account_suspend_time(*delta);
 }
 
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_RTC_HCTOSYS_DEVICE)