Re: Alternatives to /sys/kernel/debug/wakeup_sources

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Sandeep Patil]

Hi Rafael, Viresh etc.

On Tue, Jun 11, 2019 at 10:31:16AM -0700, Tri Vo wrote:
> On Tue, Jun 4, 2019 at 5:23 PM Tri Vo <trong@android.com> wrote:
> >
> > Hello Rafael,
> >
> > Currently, Android reads wakeup sources statistics from
> > /sys/kernel/debug/wakeup_sources in production environment. This
> > information is used, for example, to report which wake lock prevents
> > the device from suspending.

Android's usage of the 'wakeup_sources' from debugfs can is linked at[1].
Basically, android's battery stats implementation to plot history for suspend
blocking wakeup sources over device's boot cycle. This is used both for power
specific bug reporting but also is one of the stats that will be used towards
attributing the battery consumption to specific processes over the period of
time.

Android depended on the out-of-tree /proc/wakelocks before and now relies on
wakeup_sources debugfs entry heavily for the aforementioned use cases.

> >
> > Android userspace reading wakeup_sources is not ideal because:
> > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > not guaranteed to be backward/forward compatible.
> > - This file requires debugfs to be mounted, which itself is
> > undesirable for security reasons.
> >
> > To address these problems, we want to contribute a way to expose these
> > statistics that doesn't depend on debugfs.
> >
> > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > Or maybe implement eBPF-based solution? What do you think?

We are going through Android's out-of-tree kernel dependencies along with
userspace APIs that are not necessarily considered "stable and forever
supported" upstream. The debugfs dependencies showed up on our radar as a
result and so we are wondering if we should worry about changes in debugfs
interface and hence the question(s) below.

So, can we rely on /d/wakeup_sources to be considered a userspace API and
hence maintained stable as we do for other /proc and /sys entries?

If yes, then we will go ahead and add tests for this in LTP or
somewhere else suitable.

If no, then we would love to hear suggestions for any changes that need to be
made or we simply just move the debugfs entry into somewhere like
/sys/power/ ?

As a side effect, if the entry moves out of debugfs, Android can run without
mounting debugfs in production that I assume is a good thing.

- ssp

1. https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/java/com/android/internal/os/KernelWakelockReader.java#127

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Rafael J. Wysocki]

On Tuesday, June 18, 2019 10:17:16 PM CEST Sandeep Patil wrote:
> 
> Hi Rafael, Viresh etc.
> 
> On Tue, Jun 11, 2019 at 10:31:16AM -0700, Tri Vo wrote:
> > On Tue, Jun 4, 2019 at 5:23 PM Tri Vo <trong@android.com> wrote:
> > >
> > > Hello Rafael,
> > >
> > > Currently, Android reads wakeup sources statistics from
> > > /sys/kernel/debug/wakeup_sources in production environment. This
> > > information is used, for example, to report which wake lock prevents
> > > the device from suspending.
> 
> Android's usage of the 'wakeup_sources' from debugfs can is linked at[1].
> Basically, android's battery stats implementation to plot history for suspend
> blocking wakeup sources over device's boot cycle. This is used both for power
> specific bug reporting but also is one of the stats that will be used towards
> attributing the battery consumption to specific processes over the period of
> time.
> 
> Android depended on the out-of-tree /proc/wakelocks before and now relies on
> wakeup_sources debugfs entry heavily for the aforementioned use cases.
> 
> > >
> > > Android userspace reading wakeup_sources is not ideal because:
> > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > not guaranteed to be backward/forward compatible.
> > > - This file requires debugfs to be mounted, which itself is
> > > undesirable for security reasons.
> > >
> > > To address these problems, we want to contribute a way to expose these
> > > statistics that doesn't depend on debugfs.
> > >
> > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > Or maybe implement eBPF-based solution? What do you think?
> 
> We are going through Android's out-of-tree kernel dependencies along with
> userspace APIs that are not necessarily considered "stable and forever
> supported" upstream. The debugfs dependencies showed up on our radar as a
> result and so we are wondering if we should worry about changes in debugfs
> interface and hence the question(s) below.
> 
> So, can we rely on /d/wakeup_sources to be considered a userspace API and
> hence maintained stable as we do for other /proc and /sys entries?
> 
> If yes, then we will go ahead and add tests for this in LTP or
> somewhere else suitable.

No, debugfs is not ABI.

> If no, then we would love to hear suggestions for any changes that need to be
> made or we simply just move the debugfs entry into somewhere like
> /sys/power/ ?

No, moving that entire file from debugfs into sysfs is not an option either.

The statistics for the wakeup sources associated with devices are already there
under /sys/devices/.../power/ , but I guess you want all wakeup sources?

That would require adding a kobject to struct wakeup_source and exposing
all of the statistics as separate attributes under it.  In which case it would be
good to replace the existing wakeup statistics under /sys/devices/.../power/
with symbolic links to the attributes under the wakeup_source kobject.

> As a side effect, if the entry moves out of debugfs, Android can run without
> mounting debugfs in production that I assume is a good thing.

And really Android developers might have thought about this a bit earlier.

Thanks!

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Tri Vo]

On Tue, Jun 18, 2019 at 2:23 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Tuesday, June 18, 2019 10:17:16 PM CEST Sandeep Patil wrote:
> >
> > Hi Rafael, Viresh etc.
> >
> > On Tue, Jun 11, 2019 at 10:31:16AM -0700, Tri Vo wrote:
> > > On Tue, Jun 4, 2019 at 5:23 PM Tri Vo <trong@android.com> wrote:
> > > >
> > > > Hello Rafael,
> > > >
> > > > Currently, Android reads wakeup sources statistics from
> > > > /sys/kernel/debug/wakeup_sources in production environment. This
> > > > information is used, for example, to report which wake lock prevents
> > > > the device from suspending.
> >
> > Android's usage of the 'wakeup_sources' from debugfs can is linked at[1].
> > Basically, android's battery stats implementation to plot history for suspend
> > blocking wakeup sources over device's boot cycle. This is used both for power
> > specific bug reporting but also is one of the stats that will be used towards
> > attributing the battery consumption to specific processes over the period of
> > time.
> >
> > Android depended on the out-of-tree /proc/wakelocks before and now relies on
> > wakeup_sources debugfs entry heavily for the aforementioned use cases.
> >
> > > >
> > > > Android userspace reading wakeup_sources is not ideal because:
> > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > not guaranteed to be backward/forward compatible.
> > > > - This file requires debugfs to be mounted, which itself is
> > > > undesirable for security reasons.
> > > >
> > > > To address these problems, we want to contribute a way to expose these
> > > > statistics that doesn't depend on debugfs.
> > > >
> > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > Or maybe implement eBPF-based solution? What do you think?
> >
> > We are going through Android's out-of-tree kernel dependencies along with
> > userspace APIs that are not necessarily considered "stable and forever
> > supported" upstream. The debugfs dependencies showed up on our radar as a
> > result and so we are wondering if we should worry about changes in debugfs
> > interface and hence the question(s) below.
> >
> > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > hence maintained stable as we do for other /proc and /sys entries?
> >
> > If yes, then we will go ahead and add tests for this in LTP or
> > somewhere else suitable.
>
> No, debugfs is not ABI.
>
> > If no, then we would love to hear suggestions for any changes that need to be
> > made or we simply just move the debugfs entry into somewhere like
> > /sys/power/ ?
>
> No, moving that entire file from debugfs into sysfs is not an option either.
>
> The statistics for the wakeup sources associated with devices are already there
> under /sys/devices/.../power/ , but I guess you want all wakeup sources?
>
> That would require adding a kobject to struct wakeup_source and exposing
> all of the statistics as separate attributes under it.  In which case it would be
> good to replace the existing wakeup statistics under /sys/devices/.../power/
> with symbolic links to the attributes under the wakeup_source kobject.

Thanks for your input, Rafael! Your suggestion makes sense. I'll work
on a patch for this.
>
> > As a side effect, if the entry moves out of debugfs, Android can run without
> > mounting debugfs in production that I assume is a good thing.
>
> And really Android developers might have thought about this a bit earlier.

I'm still learning about kernel development. And Android has made
missteps before. So I figured it's a good idea to ask first :)

Thanks!

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
[snip]
> > > > >
> > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > not guaranteed to be backward/forward compatible.
> > > > > - This file requires debugfs to be mounted, which itself is
> > > > > undesirable for security reasons.
> > > > >
> > > > > To address these problems, we want to contribute a way to expose these
> > > > > statistics that doesn't depend on debugfs.
> > > > >
> > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > Or maybe implement eBPF-based solution? What do you think?
> > >
> > > We are going through Android's out-of-tree kernel dependencies along with
> > > userspace APIs that are not necessarily considered "stable and forever
> > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > result and so we are wondering if we should worry about changes in debugfs
> > > interface and hence the question(s) below.
> > >
> > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > hence maintained stable as we do for other /proc and /sys entries?
> > >
> > > If yes, then we will go ahead and add tests for this in LTP or
> > > somewhere else suitable.
> >
> > No, debugfs is not ABI.
> >
> > > If no, then we would love to hear suggestions for any changes that need to be
> > > made or we simply just move the debugfs entry into somewhere like
> > > /sys/power/ ?
> >
> > No, moving that entire file from debugfs into sysfs is not an option either.
> >
> > The statistics for the wakeup sources associated with devices are already there
> > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> >
> > That would require adding a kobject to struct wakeup_source and exposing
> > all of the statistics as separate attributes under it.  In which case it would be
> > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > with symbolic links to the attributes under the wakeup_source kobject.
>
> Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> on a patch for this.

Does that entail making each wake up source, a new sysfs node under a
particular device, and then adding stats under that new node?

thanks,

- Joel

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Rafael J. Wysocki]

On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
>
> On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> [snip]
> > > > > >
> > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > not guaranteed to be backward/forward compatible.
> > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > undesirable for security reasons.
> > > > > >
> > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > statistics that doesn't depend on debugfs.
> > > > > >
> > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > >
> > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > userspace APIs that are not necessarily considered "stable and forever
> > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > result and so we are wondering if we should worry about changes in debugfs
> > > > interface and hence the question(s) below.
> > > >
> > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > hence maintained stable as we do for other /proc and /sys entries?
> > > >
> > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > somewhere else suitable.
> > >
> > > No, debugfs is not ABI.
> > >
> > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > made or we simply just move the debugfs entry into somewhere like
> > > > /sys/power/ ?
> > >
> > > No, moving that entire file from debugfs into sysfs is not an option either.
> > >
> > > The statistics for the wakeup sources associated with devices are already there
> > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > >
> > > That would require adding a kobject to struct wakeup_source and exposing
> > > all of the statistics as separate attributes under it.  In which case it would be
> > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > with symbolic links to the attributes under the wakeup_source kobject.
> >
> > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > on a patch for this.
>
> Does that entail making each wake up source, a new sysfs node under a
> particular device, and then adding stats under that new node?

Not under a device, because there are wakeup source objects without
associated devices.

It is conceivable to have a "wakeup_sources" directory under
/sys/power/ and sysfs nodes for all wakeup sources in there.

Then, instead of exposing wakeup statistics directly under
/sys/devices/.../power/, there can be symbolic links from there to the
new wakeup source nodes under "wakeup_sources" (so as to avoid
exposing the same data in two different places in sysfs, which may be
confusing).

Cheers!

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Wed, Jun 19, 2019 at 4:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > [snip]
> > > > > > >
> > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > undesirable for security reasons.
> > > > > > >
> > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > statistics that doesn't depend on debugfs.
> > > > > > >
> > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > >
> > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > interface and hence the question(s) below.
> > > > >
> > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > >
> > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > somewhere else suitable.
> > > >
> > > > No, debugfs is not ABI.
> > > >
> > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > /sys/power/ ?
> > > >
> > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > >
> > > > The statistics for the wakeup sources associated with devices are already there
> > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > >
> > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > with symbolic links to the attributes under the wakeup_source kobject.
> > >
> > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > on a patch for this.
> >
> > Does that entail making each wake up source, a new sysfs node under a
> > particular device, and then adding stats under that new node?
>
> Not under a device, because there are wakeup source objects without
> associated devices.
>
> It is conceivable to have a "wakeup_sources" directory under
> /sys/power/ and sysfs nodes for all wakeup sources in there.
>
> Then, instead of exposing wakeup statistics directly under
> /sys/devices/.../power/, there can be symbolic links from there to the
> new wakeup source nodes under "wakeup_sources" (so as to avoid
> exposing the same data in two different places in sysfs, which may be
> confusing).

Makes sense to me, thanks!

 - Joel

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Sandeep Patil]

On Wed, Jun 19, 2019 at 10:35:17AM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > [snip]
> > > > > > >
> > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > undesirable for security reasons.
> > > > > > >
> > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > statistics that doesn't depend on debugfs.
> > > > > > >
> > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > >
> > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > interface and hence the question(s) below.
> > > > >
> > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > >
> > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > somewhere else suitable.
> > > >
> > > > No, debugfs is not ABI.
> > > >
> > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > /sys/power/ ?
> > > >
> > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > >
> > > > The statistics for the wakeup sources associated with devices are already there
> > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > >
> > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > with symbolic links to the attributes under the wakeup_source kobject.
> > >
> > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > on a patch for this.
> >
> > Does that entail making each wake up source, a new sysfs node under a
> > particular device, and then adding stats under that new node?
> 
> Not under a device, because there are wakeup source objects without
> associated devices.
> 
> It is conceivable to have a "wakeup_sources" directory under
> /sys/power/ and sysfs nodes for all wakeup sources in there.


This is what I understood from your initial reply and I think it makes sense.
Thanks again, Rafael.

- ssp

> 
> Then, instead of exposing wakeup statistics directly under
> /sys/devices/.../power/, there can be symbolic links from there to the
> new wakeup source nodes under "wakeup_sources" (so as to avoid
> exposing the same data in two different places in sysfs, which may be
> confusing).
> 
> Cheers!

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Wed, Jun 19, 2019 at 4:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
[snip]
> > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > /sys/power/ ?
> > > >
> > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > >
> > > > The statistics for the wakeup sources associated with devices are already there
> > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > >
> > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > with symbolic links to the attributes under the wakeup_source kobject.
> > >
> > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > on a patch for this.
> >
> > Does that entail making each wake up source, a new sysfs node under a
> > particular device, and then adding stats under that new node?
>
> Not under a device, because there are wakeup source objects without
> associated devices.
>
> It is conceivable to have a "wakeup_sources" directory under
> /sys/power/ and sysfs nodes for all wakeup sources in there.

One of the "issues" with this is, now if you have say 100 wake up
sources, with 10 entries each, then we're talking about a 1000 sysfs
files. Each one has to be opened, and read individually. This adds
overhead and it is more convenient to read from a single file. The
problem is this single file is not ABI. So the question I guess is,
how do we solve this in both an ABI friendly way while keeping the
overhead low.

Thanks,

 - Joel

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Greg Kroah-Hartman]

On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > It is conceivable to have a "wakeup_sources" directory under
> > /sys/power/ and sysfs nodes for all wakeup sources in there.
> 
> One of the "issues" with this is, now if you have say 100 wake up
> sources, with 10 entries each, then we're talking about a 1000 sysfs
> files. Each one has to be opened, and read individually. This adds
> overhead and it is more convenient to read from a single file. The
> problem is this single file is not ABI. So the question I guess is,
> how do we solve this in both an ABI friendly way while keeping the
> overhead low.

How much overhead?  Have you measured it, reading from virtual files is
fast :)

And how often does this happen?  Does it _need_ to happen?

Parsing files is also hard, and not for sysfs files, you can't have it
both ways.

So try it this way, and if there really is a performance issue, we can
then talk about it...

thanks,

greg k-h

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > > It is conceivable to have a "wakeup_sources" directory under
> > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> >
> > One of the "issues" with this is, now if you have say 100 wake up
> > sources, with 10 entries each, then we're talking about a 1000 sysfs
> > files. Each one has to be opened, and read individually. This adds
> > overhead and it is more convenient to read from a single file. The
> > problem is this single file is not ABI. So the question I guess is,
> > how do we solve this in both an ABI friendly way while keeping the
> > overhead low.
>
> How much overhead?  Have you measured it, reading from virtual files is
> fast :)

I measured, and it is definitely not free. If you create and read a
1000 files and just return a string back, it can take up to 11-13
milliseconds (did not lock CPU frequencies, was just looking for
average ball park). This is assuming that the counter reading is just
doing that, and nothing else is being done to return the sysfs data
which is probably not always true in practice.

Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
CPU scheduling competion reading sysfs can hurt the deadline. There's
also the question of power - we definitely have spent time in the past
optimizing other virtual files such as /proc/pid/smaps for this reason
where it spent lots of CPU time.

> And how often does this happen?  Does it _need_ to happen?

These are good questions and we should find out. I am not saying that
sysfs will not work, I am just saying we need to consider all the
things. I will let Tri look into this since he is working on it, I
don't know off the top.

> Parsing files is also hard, and not for sysfs files, you can't have it
> both ways.

I don't think we are concerned with a parsing issue here, or are
discussing it in this thread.

> So try it this way, and if there really is a performance issue, we can
> then talk about it...

Sure that sounds good to me, again I am not saying we should do sysfs.
But we should consider all the issues and chose the right solution.

thanks!

 - Joel

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Tri Vo]

On Wed, Jun 19, 2019 at 11:01 AM Joel Fernandes <joelaf@google.com> wrote:
>
> On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > > > It is conceivable to have a "wakeup_sources" directory under
> > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > >
> > > One of the "issues" with this is, now if you have say 100 wake up
> > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> > > files. Each one has to be opened, and read individually. This adds
> > > overhead and it is more convenient to read from a single file. The
> > > problem is this single file is not ABI. So the question I guess is,
> > > how do we solve this in both an ABI friendly way while keeping the
> > > overhead low.
> >
> > How much overhead?  Have you measured it, reading from virtual files is
> > fast :)
>
> I measured, and it is definitely not free. If you create and read a
> 1000 files and just return a string back, it can take up to 11-13
> milliseconds (did not lock CPU frequencies, was just looking for
> average ball park). This is assuming that the counter reading is just
> doing that, and nothing else is being done to return the sysfs data
> which is probably not always true in practice.
>
> Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> CPU scheduling competion reading sysfs can hurt the deadline. There's
> also the question of power - we definitely have spent time in the past
> optimizing other virtual files such as /proc/pid/smaps for this reason
> where it spent lots of CPU time.
>
> > And how often does this happen?  Does it _need_ to happen?
>
> These are good questions and we should find out. I am not saying that
> sysfs will not work, I am just saying we need to consider all the
> things. I will let Tri look into this since he is working on it, I
> don't know off the top.

There are really two use cases for wakeup_sources in Android:
(1) As Sandeep pointed out, it's used to plot history of wakeup
sources. This use case might actually be performance sensitive. I'll
check with our internal Android teams and loop back here.
(2) wakeup_sources information is included in bugreports.
Reading/parsing wakeup_sources in this case only happens when
generating a bugreport, so not particularly sensitive to overhead.

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Greg Kroah-Hartman]

On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
> On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > > > It is conceivable to have a "wakeup_sources" directory under
> > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > >
> > > One of the "issues" with this is, now if you have say 100 wake up
> > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> > > files. Each one has to be opened, and read individually. This adds
> > > overhead and it is more convenient to read from a single file. The
> > > problem is this single file is not ABI. So the question I guess is,
> > > how do we solve this in both an ABI friendly way while keeping the
> > > overhead low.
> >
> > How much overhead?  Have you measured it, reading from virtual files is
> > fast :)
> 
> I measured, and it is definitely not free. If you create and read a
> 1000 files and just return a string back, it can take up to 11-13
> milliseconds (did not lock CPU frequencies, was just looking for
> average ball park). This is assuming that the counter reading is just
> doing that, and nothing else is being done to return the sysfs data
> which is probably not always true in practice.
> 
> Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> CPU scheduling competion reading sysfs can hurt the deadline. There's
> also the question of power - we definitely have spent time in the past
> optimizing other virtual files such as /proc/pid/smaps for this reason
> where it spent lots of CPU time.

smaps was "odd", but that was done after measurements were actually made
to prove it was needed.  That hasn't happened yet :)

And is there a reason you have to do this every 16ms?

> > And how often does this happen?  Does it _need_ to happen?
> 
> These are good questions and we should find out. I am not saying that
> sysfs will not work, I am just saying we need to consider all the
> things. I will let Tri look into this since he is working on it, I
> don't know off the top.
> 
> > Parsing files is also hard, and not for sysfs files, you can't have it
> > both ways.
> 
> I don't think we are concerned with a parsing issue here, or are
> discussing it in this thread.
> 
> > So try it this way, and if there really is a performance issue, we can
> > then talk about it...
> 
> Sure that sounds good to me, again I am not saying we should do sysfs.
> But we should consider all the issues and chose the right solution.

Figure out who needs this, how often it needs it, and how many files
really are going to be involved before we try to optimize anything.

thanks,

greg k-h

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
> > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > > > > It is conceivable to have a "wakeup_sources" directory under
> > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > > >
> > > > One of the "issues" with this is, now if you have say 100 wake up
> > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> > > > files. Each one has to be opened, and read individually. This adds
> > > > overhead and it is more convenient to read from a single file. The
> > > > problem is this single file is not ABI. So the question I guess is,
> > > > how do we solve this in both an ABI friendly way while keeping the
> > > > overhead low.
> > >
> > > How much overhead?  Have you measured it, reading from virtual files is
> > > fast :)
> >
> > I measured, and it is definitely not free. If you create and read a
> > 1000 files and just return a string back, it can take up to 11-13
> > milliseconds (did not lock CPU frequencies, was just looking for
> > average ball park). This is assuming that the counter reading is just
> > doing that, and nothing else is being done to return the sysfs data
> > which is probably not always true in practice.
> >
> > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> > CPU scheduling competion reading sysfs can hurt the deadline. There's
> > also the question of power - we definitely have spent time in the past
> > optimizing other virtual files such as /proc/pid/smaps for this reason
> > where it spent lots of CPU time.
>
> smaps was "odd", but that was done after measurements were actually made
> to prove it was needed.  That hasn't happened yet :)
>
> And is there a reason you have to do this every 16ms?

Not every, I was just saying whenever it happens and a frame delivery
deadline is missed, then a frame drop can occur which can result in a
poor user experience.

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Wed, Jun 19, 2019 at 3:59 PM 'Saravana Kannan' via kernel-team
<kernel-team@android.com> wrote:
>
>
>
> On Wed, Jun 19, 2019, 11:55 AM 'Joel Fernandes' via kernel-team <kernel-team@android.com> wrote:
>>
>> On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> >
>> > On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
>> > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
>> > > <gregkh@linuxfoundation.org> wrote:
>> > > >
>> > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
>> > > > > > It is conceivable to have a "wakeup_sources" directory under
>> > > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
>> > > > >
>> > > > > One of the "issues" with this is, now if you have say 100 wake up
>> > > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
>> > > > > files. Each one has to be opened, and read individually. This adds
>> > > > > overhead and it is more convenient to read from a single file. The
>> > > > > problem is this single file is not ABI. So the question I guess is,
>> > > > > how do we solve this in both an ABI friendly way while keeping the
>> > > > > overhead low.
>> > > >
>> > > > How much overhead?  Have you measured it, reading from virtual files is
>> > > > fast :)
>> > >
>> > > I measured, and it is definitely not free. If you create and read a
>> > > 1000 files and just return a string back, it can take up to 11-13
>> > > milliseconds (did not lock CPU frequencies, was just looking for
>> > > average ball park). This is assuming that the counter reading is just
>> > > doing that, and nothing else is being done to return the sysfs data
>> > > which is probably not always true in practice.
>> > >
>> > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
>> > > CPU scheduling competion reading sysfs can hurt the deadline. There's
>> > > also the question of power - we definitely have spent time in the past
>> > > optimizing other virtual files such as /proc/pid/smaps for this reason
>> > > where it spent lots of CPU time.
>> >
>> > smaps was "odd", but that was done after measurements were actually made
>> > to prove it was needed.  That hasn't happened yet :)
>> >
>> > And is there a reason you have to do this every 16ms?
>>
>> Not every, I was just saying whenever it happens and a frame delivery
>> deadline is missed, then a frame drop can occur which can result in a
>> poor user experience.
>
>
> But this is not done in the UI thread context. So some thread running for more than 16ms shouldn't cause a frame drop. If it does, we have bigger problems.
>

Not really. That depends on the priority of the other thread and other
things. It can obviously time share the same CPU as the UI thread if
it is not configured correctly. Even with CFS it can reduce the time
consumed by other "real-time" CFS threads. I am not sure what you are
proposing, there are also (obviously) power issues with things running
for long times pointlessly. We should try to do better if we can. As
Greg said, some study/research can be done on the use case before
settling for a solution (sysfs or other).

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Saravana Kannan]

On Wed, Jun 19, 2019 at 1:09 PM 'Joel Fernandes' via kernel-team
<kernel-team@android.com> wrote:
>
> On Wed, Jun 19, 2019 at 3:59 PM 'Saravana Kannan' via kernel-team
> <kernel-team@android.com> wrote:
> >
> >
> >
> > On Wed, Jun 19, 2019, 11:55 AM 'Joel Fernandes' via kernel-team <kernel-team@android.com> wrote:
> >>
> >> On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> >
> >> > On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
> >> > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> >> > > <gregkh@linuxfoundation.org> wrote:
> >> > > >
> >> > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> >> > > > > > It is conceivable to have a "wakeup_sources" directory under
> >> > > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> >> > > > >
> >> > > > > One of the "issues" with this is, now if you have say 100 wake up
> >> > > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> >> > > > > files. Each one has to be opened, and read individually. This adds
> >> > > > > overhead and it is more convenient to read from a single file. The
> >> > > > > problem is this single file is not ABI. So the question I guess is,
> >> > > > > how do we solve this in both an ABI friendly way while keeping the
> >> > > > > overhead low.
> >> > > >
> >> > > > How much overhead?  Have you measured it, reading from virtual files is
> >> > > > fast :)
> >> > >
> >> > > I measured, and it is definitely not free. If you create and read a
> >> > > 1000 files and just return a string back, it can take up to 11-13
> >> > > milliseconds (did not lock CPU frequencies, was just looking for
> >> > > average ball park). This is assuming that the counter reading is just
> >> > > doing that, and nothing else is being done to return the sysfs data
> >> > > which is probably not always true in practice.
> >> > >
> >> > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> >> > > CPU scheduling competion reading sysfs can hurt the deadline. There's
> >> > > also the question of power - we definitely have spent time in the past
> >> > > optimizing other virtual files such as /proc/pid/smaps for this reason
> >> > > where it spent lots of CPU time.
> >> >
> >> > smaps was "odd", but that was done after measurements were actually made
> >> > to prove it was needed.  That hasn't happened yet :)
> >> >
> >> > And is there a reason you have to do this every 16ms?
> >>
> >> Not every, I was just saying whenever it happens and a frame delivery
> >> deadline is missed, then a frame drop can occur which can result in a
> >> poor user experience.
> >
> >
> > But this is not done in the UI thread context. So some thread running for more than 16ms shouldn't cause a frame drop. If it does, we have bigger problems.
> >
>
> Not really. That depends on the priority of the other thread and other
> things. It can obviously time share the same CPU as the UI thread if
> it is not configured correctly. Even with CFS it can reduce the time
> consumed by other "real-time" CFS threads. I am not sure what you are
> proposing, there are also (obviously) power issues with things running
> for long times pointlessly. We should try to do better if we can. As
> Greg said, some study/research can be done on the use case before
> settling for a solution (sysfs or other).
>

Agree, power and optimization is good. Just saying that the UI example
is not a real one. If the UI thread is that poorly configured that
some thread running for a second can cause frame drops in a multicore
system, that's a problem with the UI framework design.

-Saravana

> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Wed, Jun 19, 2019 at 4:41 PM 'Saravana Kannan' via kernel-team
<kernel-team@android.com> wrote:
> > > On Wed, Jun 19, 2019, 11:55 AM 'Joel Fernandes' via kernel-team <kernel-team@android.com> wrote:
> > >>
> > >> On Wed, Jun 19, 2019 at 2:35 PM Greg Kroah-Hartman
> > >> <gregkh@linuxfoundation.org> wrote:
> > >> >
> > >> > On Wed, Jun 19, 2019 at 02:01:36PM -0400, Joel Fernandes wrote:
> > >> > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman
> > >> > > <gregkh@linuxfoundation.org> wrote:
> > >> > > >
> > >> > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote:
> > >> > > > > > It is conceivable to have a "wakeup_sources" directory under
> > >> > > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > >> > > > >
> > >> > > > > One of the "issues" with this is, now if you have say 100 wake up
> > >> > > > > sources, with 10 entries each, then we're talking about a 1000 sysfs
> > >> > > > > files. Each one has to be opened, and read individually. This adds
> > >> > > > > overhead and it is more convenient to read from a single file. The
> > >> > > > > problem is this single file is not ABI. So the question I guess is,
> > >> > > > > how do we solve this in both an ABI friendly way while keeping the
> > >> > > > > overhead low.
> > >> > > >
> > >> > > > How much overhead?  Have you measured it, reading from virtual files is
> > >> > > > fast :)
> > >> > >
> > >> > > I measured, and it is definitely not free. If you create and read a
> > >> > > 1000 files and just return a string back, it can take up to 11-13
> > >> > > milliseconds (did not lock CPU frequencies, was just looking for
> > >> > > average ball park). This is assuming that the counter reading is just
> > >> > > doing that, and nothing else is being done to return the sysfs data
> > >> > > which is probably not always true in practice.
> > >> > >
> > >> > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any
> > >> > > CPU scheduling competion reading sysfs can hurt the deadline. There's
> > >> > > also the question of power - we definitely have spent time in the past
> > >> > > optimizing other virtual files such as /proc/pid/smaps for this reason
> > >> > > where it spent lots of CPU time.
> > >> >
> > >> > smaps was "odd", but that was done after measurements were actually made
> > >> > to prove it was needed.  That hasn't happened yet :)
> > >> >
> > >> > And is there a reason you have to do this every 16ms?
> > >>
> > >> Not every, I was just saying whenever it happens and a frame delivery
> > >> deadline is missed, then a frame drop can occur which can result in a
> > >> poor user experience.
> > >
> > >
> > > But this is not done in the UI thread context. So some thread running for more than 16ms shouldn't cause a frame drop. If it does, we have bigger problems.
> > >
> >
> > Not really. That depends on the priority of the other thread and other
> > things. It can obviously time share the same CPU as the UI thread if
> > it is not configured correctly. Even with CFS it can reduce the time
> > consumed by other "real-time" CFS threads. I am not sure what you are
> > proposing, there are also (obviously) power issues with things running
> > for long times pointlessly. We should try to do better if we can. As
> > Greg said, some study/research can be done on the use case before
> > settling for a solution (sysfs or other).
> >
>
> Agree, power and optimization is good. Just saying that the UI example
> is not a real one. If the UI thread is that poorly configured that
> some thread running for a second can cause frame drops in a multicore
> system, that's a problem with the UI framework design.

We do know that historically there are problems with the UI thread's
scheduling and folks are looking into DL scheduling for that. I was
just giving UI thread as an example, there are also other low latency
threads (audio etc). Anyway, I think we know the next steps here so we
can park this discussion for now.

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Tri Vo]

On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > [snip]
> > > > > > >
> > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > undesirable for security reasons.
> > > > > > >
> > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > statistics that doesn't depend on debugfs.
> > > > > > >
> > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > >
> > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > interface and hence the question(s) below.
> > > > >
> > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > >
> > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > somewhere else suitable.
> > > >
> > > > No, debugfs is not ABI.
> > > >
> > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > /sys/power/ ?
> > > >
> > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > >
> > > > The statistics for the wakeup sources associated with devices are already there
> > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > >
> > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > with symbolic links to the attributes under the wakeup_source kobject.
> > >
> > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > on a patch for this.
> >
> > Does that entail making each wake up source, a new sysfs node under a
> > particular device, and then adding stats under that new node?
>
> Not under a device, because there are wakeup source objects without
> associated devices.
>
> It is conceivable to have a "wakeup_sources" directory under
> /sys/power/ and sysfs nodes for all wakeup sources in there.
>
> Then, instead of exposing wakeup statistics directly under
> /sys/devices/.../power/, there can be symbolic links from there to the
> new wakeup source nodes under "wakeup_sources" (so as to avoid
> exposing the same data in two different places in sysfs, which may be
> confusing).

This may be a dumb question. Is it appropriate to make symbolic links
in sysfs from one attribute to another attribute? For example,
/sys/devices/.../power/wakeup_count ->
/sys/power/wakeup_sources/.../wakeup_count.

I only see kobject to kobject symlinks around. And I don't think we
can make /sys/devices/.../power/ directory a symlink to where our new
wakeup stats will be, since the former contains attributes other than
wakeup ones.

Thanks!

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Greg Kroah-Hartman]

On Sun, Jun 23, 2019 at 06:48:43PM -0700, Tri Vo wrote:
> On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > > [snip]
> > > > > > > >
> > > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > > undesirable for security reasons.
> > > > > > > >
> > > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > > statistics that doesn't depend on debugfs.
> > > > > > > >
> > > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > > >
> > > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > > interface and hence the question(s) below.
> > > > > >
> > > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > > >
> > > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > > somewhere else suitable.
> > > > >
> > > > > No, debugfs is not ABI.
> > > > >
> > > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > > /sys/power/ ?
> > > > >
> > > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > > >
> > > > > The statistics for the wakeup sources associated with devices are already there
> > > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > > >
> > > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > > with symbolic links to the attributes under the wakeup_source kobject.
> > > >
> > > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > > on a patch for this.
> > >
> > > Does that entail making each wake up source, a new sysfs node under a
> > > particular device, and then adding stats under that new node?
> >
> > Not under a device, because there are wakeup source objects without
> > associated devices.
> >
> > It is conceivable to have a "wakeup_sources" directory under
> > /sys/power/ and sysfs nodes for all wakeup sources in there.
> >
> > Then, instead of exposing wakeup statistics directly under
> > /sys/devices/.../power/, there can be symbolic links from there to the
> > new wakeup source nodes under "wakeup_sources" (so as to avoid
> > exposing the same data in two different places in sysfs, which may be
> > confusing).
> 
> This may be a dumb question. Is it appropriate to make symbolic links
> in sysfs from one attribute to another attribute? For example,
> /sys/devices/.../power/wakeup_count ->
> /sys/power/wakeup_sources/.../wakeup_count.

Why? would you want that?

> I only see kobject to kobject symlinks around. And I don't think we
> can make /sys/devices/.../power/ directory a symlink to where our new
> wakeup stats will be, since the former contains attributes other than
> wakeup ones.

No, don't link attributes, they refer to the kobject that created them.
I really doubt that this is the same kobject in both places :)

thanks,

greg k-h

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Joel Fernandes]

On Mon, Jun 24, 2019 at 3:37 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, Jun 23, 2019 at 06:48:43PM -0700, Tri Vo wrote:
> > On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > > > [snip]
> > > > > > > > >
> > > > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > > > undesirable for security reasons.
> > > > > > > > >
> > > > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > > > statistics that doesn't depend on debugfs.
> > > > > > > > >
> > > > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > > > >
> > > > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > > > interface and hence the question(s) below.
> > > > > > >
> > > > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > > > >
> > > > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > > > somewhere else suitable.
> > > > > >
> > > > > > No, debugfs is not ABI.
> > > > > >
> > > > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > > > /sys/power/ ?
> > > > > >
> > > > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > > > >
> > > > > > The statistics for the wakeup sources associated with devices are already there
> > > > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > > > >
> > > > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > > > with symbolic links to the attributes under the wakeup_source kobject.
> > > > >
> > > > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > > > on a patch for this.
> > > >
> > > > Does that entail making each wake up source, a new sysfs node under a
> > > > particular device, and then adding stats under that new node?
> > >
> > > Not under a device, because there are wakeup source objects without
> > > associated devices.
> > >
> > > It is conceivable to have a "wakeup_sources" directory under
> > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > >
> > > Then, instead of exposing wakeup statistics directly under
> > > /sys/devices/.../power/, there can be symbolic links from there to the
> > > new wakeup source nodes under "wakeup_sources" (so as to avoid
> > > exposing the same data in two different places in sysfs, which may be
> > > confusing).
> >
> > This may be a dumb question. Is it appropriate to make symbolic links
> > in sysfs from one attribute to another attribute? For example,
> > /sys/devices/.../power/wakeup_count ->
> > /sys/power/wakeup_sources/.../wakeup_count.
>
> Why? would you want that?

This sounds like what Rafael suggested (quoted above), right?

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Rafael J. Wysocki]

On Mon, Jun 24, 2019 at 2:27 PM Joel Fernandes <joelaf@google.com> wrote:
>
> On Mon, Jun 24, 2019 at 3:37 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Jun 23, 2019 at 06:48:43PM -0700, Tri Vo wrote:
> > > On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> > > > >
> > > > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > > > > [snip]
> > > > > > > > > >
> > > > > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > > > > undesirable for security reasons.
> > > > > > > > > >
> > > > > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > > > > statistics that doesn't depend on debugfs.
> > > > > > > > > >
> > > > > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > > > > >
> > > > > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > > > > interface and hence the question(s) below.
> > > > > > > >
> > > > > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > > > > >
> > > > > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > > > > somewhere else suitable.
> > > > > > >
> > > > > > > No, debugfs is not ABI.
> > > > > > >
> > > > > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > > > > /sys/power/ ?
> > > > > > >
> > > > > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > > > > >
> > > > > > > The statistics for the wakeup sources associated with devices are already there
> > > > > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > > > > >
> > > > > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > > > > with symbolic links to the attributes under the wakeup_source kobject.
> > > > > >
> > > > > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > > > > on a patch for this.
> > > > >
> > > > > Does that entail making each wake up source, a new sysfs node under a
> > > > > particular device, and then adding stats under that new node?
> > > >
> > > > Not under a device, because there are wakeup source objects without
> > > > associated devices.
> > > >
> > > > It is conceivable to have a "wakeup_sources" directory under
> > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > > >
> > > > Then, instead of exposing wakeup statistics directly under
> > > > /sys/devices/.../power/, there can be symbolic links from there to the
> > > > new wakeup source nodes under "wakeup_sources" (so as to avoid
> > > > exposing the same data in two different places in sysfs, which may be
> > > > confusing).
> > >
> > > This may be a dumb question. Is it appropriate to make symbolic links
> > > in sysfs from one attribute to another attribute? For example,
> > > /sys/devices/.../power/wakeup_count ->
> > > /sys/power/wakeup_sources/.../wakeup_count.
> >
> > Why? would you want that?
>
> This sounds like what Rafael suggested (quoted above), right?

I did, basically to avoid exposing the same information in two places
via different code paths.

I tend to forget about this limitation, sorry for the confusion.

That's not a big deal, though, the attributes under
/sys/devices/.../power/ just need to stay the way they are (for
backwards compatibility).

Re: Alternatives to /sys/kernel/debug/wakeup_sources

[Tri Vo]

On Mon, Jun 24, 2019 at 2:55 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jun 24, 2019 at 2:27 PM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Mon, Jun 24, 2019 at 3:37 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Jun 23, 2019 at 06:48:43PM -0700, Tri Vo wrote:
> > > > On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes <joelaf@google.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo <trong@android.com> wrote:
> > > > > > [snip]
> > > > > > > > > > >
> > > > > > > > > > > Android userspace reading wakeup_sources is not ideal because:
> > > > > > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are
> > > > > > > > > > > not guaranteed to be backward/forward compatible.
> > > > > > > > > > > - This file requires debugfs to be mounted, which itself is
> > > > > > > > > > > undesirable for security reasons.
> > > > > > > > > > >
> > > > > > > > > > > To address these problems, we want to contribute a way to expose these
> > > > > > > > > > > statistics that doesn't depend on debugfs.
> > > > > > > > > > >
> > > > > > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs?
> > > > > > > > > > > Or maybe implement eBPF-based solution? What do you think?
> > > > > > > > >
> > > > > > > > > We are going through Android's out-of-tree kernel dependencies along with
> > > > > > > > > userspace APIs that are not necessarily considered "stable and forever
> > > > > > > > > supported" upstream. The debugfs dependencies showed up on our radar as a
> > > > > > > > > result and so we are wondering if we should worry about changes in debugfs
> > > > > > > > > interface and hence the question(s) below.
> > > > > > > > >
> > > > > > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and
> > > > > > > > > hence maintained stable as we do for other /proc and /sys entries?
> > > > > > > > >
> > > > > > > > > If yes, then we will go ahead and add tests for this in LTP or
> > > > > > > > > somewhere else suitable.
> > > > > > > >
> > > > > > > > No, debugfs is not ABI.
> > > > > > > >
> > > > > > > > > If no, then we would love to hear suggestions for any changes that need to be
> > > > > > > > > made or we simply just move the debugfs entry into somewhere like
> > > > > > > > > /sys/power/ ?
> > > > > > > >
> > > > > > > > No, moving that entire file from debugfs into sysfs is not an option either.
> > > > > > > >
> > > > > > > > The statistics for the wakeup sources associated with devices are already there
> > > > > > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources?
> > > > > > > >
> > > > > > > > That would require adding a kobject to struct wakeup_source and exposing
> > > > > > > > all of the statistics as separate attributes under it.  In which case it would be
> > > > > > > > good to replace the existing wakeup statistics under /sys/devices/.../power/
> > > > > > > > with symbolic links to the attributes under the wakeup_source kobject.
> > > > > > >
> > > > > > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work
> > > > > > > on a patch for this.
> > > > > >
> > > > > > Does that entail making each wake up source, a new sysfs node under a
> > > > > > particular device, and then adding stats under that new node?
> > > > >
> > > > > Not under a device, because there are wakeup source objects without
> > > > > associated devices.
> > > > >
> > > > > It is conceivable to have a "wakeup_sources" directory under
> > > > > /sys/power/ and sysfs nodes for all wakeup sources in there.
> > > > >
> > > > > Then, instead of exposing wakeup statistics directly under
> > > > > /sys/devices/.../power/, there can be symbolic links from there to the
> > > > > new wakeup source nodes under "wakeup_sources" (so as to avoid
> > > > > exposing the same data in two different places in sysfs, which may be
> > > > > confusing).
> > > >
> > > > This may be a dumb question. Is it appropriate to make symbolic links
> > > > in sysfs from one attribute to another attribute? For example,
> > > > /sys/devices/.../power/wakeup_count ->
> > > > /sys/power/wakeup_sources/.../wakeup_count.
> > >
> > > Why? would you want that?
> >
> > This sounds like what Rafael suggested (quoted above), right?
>
> I did, basically to avoid exposing the same information in two places
> via different code paths.
>
> I tend to forget about this limitation, sorry for the confusion.
>
> That's not a big deal, though, the attributes under
> /sys/devices/.../power/ just need to stay the way they are (for
> backwards compatibility).

Thanks for the clarification, Rafael!