Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs

[Tri Vo]

On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo wrote:
> > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > blocking wakeup sources over device's boot cycle. This information can
> > then be used (1) for power-specific bug reporting and (2) towards
> > attributing battery consumption to specific processes over a period of
> > time.
> >
> > However, debugfs doesn't have stable ABI. For this reason, expose wakeup
> > sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> >
> > Add following attributes to each wakeup source. These attributes match
> > the columns of /sys/kernel/debug/wakeup_sources.
> >
> >   active_count
> >   event_count
> >   wakeup_count
> >   expire_count
> >   active_time (named "active_since" in debugfs)
> >   total_time
> >   max_time
> >   last_change
> >   prevent_suspend_time
>
> Can you also add a Documentation/ABI/ update for your new sysfs files so
> that we can properly review this?
>
> > Embedding a struct kobject into struct wakeup_source changes lifetime
> > requirements on the latter. To that end, change deallocation of struct
> > wakeup_source using kfree to kobject_put().

Will do.
>
> Ick, are you sure you need a new kobject here?  Why wouldn't a named
> attribute group work instead?  That should keep this patch much smaller
> and simpler.

Yeah, named attribute groups might be a much cleaner way to do this.
Let me investigate.
>
> > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > +                                     struct wakeup_source_attribute *attr,
> > +                                     char *buf)
> > +{
> > +     unsigned long flags;
> > +     unsigned long var;
> > +
> > +     spin_lock_irqsave(&ws->lock, flags);
> > +     if (strcmp(attr->attr.name, "active_count") == 0)
> > +             var = ws->active_count;
> > +     else if (strcmp(attr->attr.name, "event_count") == 0)
> > +             var = ws->event_count;
> > +     else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > +             var = ws->wakeup_count;
> > +     else
> > +             var = ws->expire_count;
> > +     spin_unlock_irqrestore(&ws->lock, flags);
> > +
> > +     return sprintf(buf, "%lu\n", var);
> > +}
>
> Why is this lock always needed to be grabbed?  You are just reading a
> value, who cares if it changes inbetween reading it and returning the
> buffer string as it can change at that point in time anyway?

Right, we don't care if the value changes in between us reading and
printing it. However, IIUC not grabbing this lock results in a data
race, which is undefined behavior.