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

[Limonciello, Mario]

On 11/15/2022 14:44, John Stultz wrote:
> On Tue, Nov 15, 2022 at 12:02 PM Mario Limonciello
> <mario.limonciello@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxx>
> > +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