* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 8:35 ` Rafael J. Wysocki
@ 2019-06-19 10:33 ` Joel Fernandes
2019-06-19 16:51 ` Sandeep Patil
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: Joel Fernandes @ 2019-06-19 10:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tri Vo, Rafael J. Wysocki, Sandeep Patil, Viresh Kumar,
Hridya Valsaraju, Linux PM, Cc: Android Kernel,
Greg Kroah-Hartman, LKML
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 8:35 ` Rafael J. Wysocki
2019-06-19 10:33 ` Joel Fernandes
@ 2019-06-19 16:51 ` Sandeep Patil
2019-06-19 16:53 ` Joel Fernandes
2019-06-24 1:48 ` Tri Vo
3 siblings, 0 replies; 23+ messages in thread
From: Sandeep Patil @ 2019-06-19 16:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joel Fernandes, Tri Vo, Rafael J. Wysocki, Viresh Kumar,
Hridya Valsaraju, Linux PM, Cc: Android Kernel,
Greg Kroah-Hartman, LKML
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!
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 8:35 ` Rafael J. Wysocki
2019-06-19 10:33 ` Joel Fernandes
2019-06-19 16:51 ` Sandeep Patil
@ 2019-06-19 16:53 ` Joel Fernandes
2019-06-19 17:07 ` Greg Kroah-Hartman
2019-06-24 1:48 ` Tri Vo
3 siblings, 1 reply; 23+ messages in thread
From: Joel Fernandes @ 2019-06-19 16:53 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tri Vo, Rafael J. Wysocki, Sandeep Patil, Viresh Kumar,
Hridya Valsaraju, Linux PM, Cc: Android Kernel,
Greg Kroah-Hartman, LKML, Alexei Starovoitov, Steven Rostedt,
Alexei Starovoitov
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 16:53 ` Joel Fernandes
@ 2019-06-19 17:07 ` Greg Kroah-Hartman
2019-06-19 18:01 ` Joel Fernandes
0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-19 17:07 UTC (permalink / raw)
To: Joel Fernandes
Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Sandeep Patil,
Viresh Kumar, Hridya Valsaraju, Linux PM, Cc: Android Kernel,
LKML, Alexei Starovoitov, Steven Rostedt, Alexei Starovoitov
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 17:07 ` Greg Kroah-Hartman
@ 2019-06-19 18:01 ` Joel Fernandes
2019-06-19 18:31 ` Tri Vo
2019-06-19 18:35 ` Greg Kroah-Hartman
0 siblings, 2 replies; 23+ messages in thread
From: Joel Fernandes @ 2019-06-19 18:01 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Sandeep Patil,
Viresh Kumar, Hridya Valsaraju, Linux PM, Cc: Android Kernel,
LKML, Alexei Starovoitov, Steven Rostedt, Alexei Starovoitov
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 18:01 ` Joel Fernandes
@ 2019-06-19 18:31 ` Tri Vo
2019-06-19 18:35 ` Greg Kroah-Hartman
1 sibling, 0 replies; 23+ messages in thread
From: Tri Vo @ 2019-06-19 18:31 UTC (permalink / raw)
To: Joel Fernandes
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML, Alexei Starovoitov, Steven Rostedt,
Alexei Starovoitov
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 18:01 ` Joel Fernandes
2019-06-19 18:31 ` Tri Vo
@ 2019-06-19 18:35 ` Greg Kroah-Hartman
2019-06-19 18:55 ` Joel Fernandes
1 sibling, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-19 18:35 UTC (permalink / raw)
To: Joel Fernandes
Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Sandeep Patil,
Viresh Kumar, Hridya Valsaraju, Linux PM, Cc: Android Kernel,
LKML, Alexei Starovoitov, Steven Rostedt, Alexei Starovoitov
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 18:35 ` Greg Kroah-Hartman
@ 2019-06-19 18:55 ` Joel Fernandes
[not found] ` <CAGETcx-ZZRc_jtBws2cFTe1wjiWeBowdqfqOhcCJV_7AUyBEVw@mail.gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Joel Fernandes @ 2019-06-19 18:55 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Sandeep Patil,
Viresh Kumar, Hridya Valsaraju, Linux PM, Cc: Android Kernel,
LKML, Alexei Starovoitov, Steven Rostedt, Alexei Starovoitov
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-19 8:35 ` Rafael J. Wysocki
` (2 preceding siblings ...)
2019-06-19 16:53 ` Joel Fernandes
@ 2019-06-24 1:48 ` Tri Vo
2019-06-24 7:36 ` Greg Kroah-Hartman
3 siblings, 1 reply; 23+ messages in thread
From: Tri Vo @ 2019-06-24 1:48 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joel Fernandes, Rafael J. Wysocki, Sandeep Patil, Viresh Kumar,
Hridya Valsaraju, Linux PM, Cc: Android Kernel,
Greg Kroah-Hartman, LKML
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!
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-24 1:48 ` Tri Vo
@ 2019-06-24 7:36 ` Greg Kroah-Hartman
2019-06-24 12:27 ` Joel Fernandes
0 siblings, 1 reply; 23+ messages in thread
From: Greg Kroah-Hartman @ 2019-06-24 7:36 UTC (permalink / raw)
To: Tri Vo
Cc: Rafael J. Wysocki, Joel Fernandes, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-24 7:36 ` Greg Kroah-Hartman
@ 2019-06-24 12:27 ` Joel Fernandes
2019-06-24 21:55 ` Rafael J. Wysocki
0 siblings, 1 reply; 23+ messages in thread
From: Joel Fernandes @ 2019-06-24 12:27 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Tri Vo, Rafael J. Wysocki, Rafael J. Wysocki, Sandeep Patil,
Viresh Kumar, Hridya Valsaraju, Linux PM, Cc: Android Kernel,
LKML
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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-24 12:27 ` Joel Fernandes
@ 2019-06-24 21:55 ` Rafael J. Wysocki
2019-06-24 22:14 ` Tri Vo
0 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2019-06-24 21:55 UTC (permalink / raw)
To: Joel Fernandes
Cc: Greg Kroah-Hartman, Tri Vo, Rafael J. Wysocki, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML
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).
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Alternatives to /sys/kernel/debug/wakeup_sources
2019-06-24 21:55 ` Rafael J. Wysocki
@ 2019-06-24 22:14 ` Tri Vo
0 siblings, 0 replies; 23+ messages in thread
From: Tri Vo @ 2019-06-24 22:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Joel Fernandes, Greg Kroah-Hartman, Rafael J. Wysocki,
Sandeep Patil, Viresh Kumar, Hridya Valsaraju, Linux PM,
Cc: Android Kernel, LKML
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!
^ permalink raw reply [flat|nested] 23+ messages in thread