Telemetry: Implement idle wakeups counting on Android.

Telemetry: Implement idle wakeups counting on Android.

BUG=422170

[Lei Zhang, 2014-10-24 03:00:37]

Patchset #2 (id:20001) has been deleted

[Lei Zhang, 2014-10-24 03:00:41]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2014-10-24 03:03:25]

thestig@chromium.org changed reviewers:
+ dtu@chromium.org, tonyg@chromium.org

[Lei Zhang, 2014-10-24 03:03:26]

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:10:
import logging
I'm not sure how much of the code below should go here versus
linux_platform_backend.py.

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:54:
results.update({'IdleWakeupCount': self._GetIdleWakeupCount(pid)})
Just following the mac version, hope that's ok.

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
I really just need to do: "echo 1 > /proc/timer_stats" as root. Is there a
better way?

Should I call python /path/to/linux_helper.py below, or just make
linux_helper.py executable and run it directly?

On the bots, how do they handle the interactive sudo session?

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:161:
logging.error('Failed to set /proc/timer_stats')
Is this ok or should we raise an Exception?

[jeremy, 2014-10-27 06:04:16]

jeremy@chromium.org changed reviewers:
+ jeremy@chromium.org

[jeremy, 2014-10-27 06:04:17]

Very very nice!!! Thanks for implementing this!

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:53:
results.update({'CpuProcessTime': cpu_process_jiffies})
On Mac this is returned in seconds, any way of converting this?

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:123: def
_GetIdleWakeupCount(self, pid):
Can we add a unit test for this, esp. the parsing.

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
By interactive sudo sessions, do you mean when credentials are requested
interactively? In this case execution will freeze :(

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:161:
logging.error('Failed to set /proc/timer_stats')
IMHO it should raise an exception, silent failures on the bots are bad.

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_helper.py (right):

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_helper.py:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
Might need to change this to a compiled binary so we can setuid it...
Tony might have an idea for a better way...

[sullivan, 2014-10-27 14:43:32]

sullivan@chromium.org changed reviewers:
+ sullivan@chromium.org

[sullivan, 2014-10-27 14:43:32]

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:10:
import logging
On 2014/10/24 03:03:26, Lei Zhang wrote:
> I'm not sure how much of the code below should go here versus
> linux_platform_backend.py.

This code is shared with android and chrome OS. Does it work on those platforms?

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
I think we want to factor this so we can call LaunchApplication with
elevate_privilege (would require setuid):
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:161:
logging.error('Failed to set /proc/timer_stats')
On 2014/10/27 06:04:16, jeremy wrote:
> IMHO it should raise an exception, silent failures on the bots are bad.

I'd really love to see the answer to this written down in documentation
somewhere!

I noticed that for example when we fail to read MSRs, we raise OSError:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

[Lei Zhang, 2014-10-28 08:12:19]

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:10:
import logging
On 2014/10/27 14:43:32, sullivan wrote:
> On 2014/10/24 03:03:26, Lei Zhang wrote:
> > I'm not sure how much of the code below should go here versus
> > linux_platform_backend.py.
> 
> This code is shared with android and chrome OS. Does it work on those
platforms?

I'll have to check and make sure.

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:53:
results.update({'CpuProcessTime': cpu_process_jiffies})
On 2014/10/27 06:04:17, jeremy wrote:
> On Mac this is returned in seconds, any way of converting this?

Doh, that means this has been wrong the entire time. I'll fix it in a separate
CL: https://codereview.chromium.org/684713003/

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:123: def
_GetIdleWakeupCount(self, pid):
On 2014/10/27 06:04:16, jeremy wrote:
> Can we add a unit test for this, esp. the parsing.

Done.

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
On 2014/10/27 14:43:32, sullivan wrote:
> I think we want to factor this so we can call LaunchApplication with
> elevate_privilege (would require setuid):
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

Well, I am doing just that below. But without making the binary setuid, if I run
this code as is in a terminal, it asks me for my password via sudo. That's what
I mean by the "interactive sudo session."

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:161:
logging.error('Failed to set /proc/timer_stats')
On 2014/10/27 14:43:32, sullivan wrote:
> On 2014/10/27 06:04:16, jeremy wrote:
> > IMHO it should raise an exception, silent failures on the bots are bad.
> 
> I'd really love to see the answer to this written down in documentation
> somewhere!
> 
> I noticed that for example when we fail to read MSRs, we raise OSError:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

OSError it is.

[jeremy, 2014-10-28 11:38:04]

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
As mentioned above, an interactive prompt will stall and eventually timeout on
the bots.

[Lei Zhang, 2014-10-28 23:35:42]

On 2014/10/28 11:38:04, jeremy wrote:
>
https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
> (right):
> 
>
https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
> os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
> As mentioned above, an interactive prompt will stall and eventually timeout on
> the bots.

So maybe we want a setuid binary then? I haven't tried CrOS yet, but on Android,
it doesn't like linux_helper.py since there's no Python interpreter, and Android
doesn't support elevated_privileges. I'll have to play around and see what's the
best way to do this across all Linux-based platforms. Suggestions welcome.

[Lei Zhang, 2014-10-29 00:28:16]

On 2014/10/28 23:35:42, Lei Zhang wrote:
> On 2014/10/28 11:38:04, jeremy wrote:
> >
>
https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/675843004/diff/40001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/core/platform/linux_based_platform_backend.py:150:
> > os.path.join(os.path.dirname(__file__), 'linux_helper.py'),
> > As mentioned above, an interactive prompt will stall and eventually timeout
on
> > the bots.
> 
> So maybe we want a setuid binary then? I haven't tried CrOS yet, but on
Android,
> it doesn't like linux_helper.py since there's no Python interpreter, and
Android
> doesn't support elevated_privileges. I'll have to play around and see what's
the
> best way to do this across all Linux-based platforms. Suggestions welcome.

Actually, I fixed Android.

[jeremy, 2014-10-29 06:03:14]

On 2014/10/28 23:35:42, Lei Zhang wrote:
> So maybe we want a setuid binary then? I haven't tried CrOS yet, but on
Android,
> it doesn't like linux_helper.py since there's no Python interpreter, and
Android
> doesn't support elevated_privileges. I'll have to play around and see what's
the
> best way to do this across all Linux-based platforms. Suggestions welcome.

Only idea I have is a binary :(

[Lei Zhang, 2014-10-30 08:50:43]

I'm scaling back the scope of this CL to just Android. A lot of the CL is to set
up infrastructure for future Linux / CrOS implementations.

[Lei Zhang, 2014-11-05 07:07:10]

On 2014/10/30 08:50:43, Lei Zhang wrote:
> I'm scaling back the scope of this CL to just Android. A lot of the CL is to
set
> up infrastructure for future Linux / CrOS implementations.

All fixed up, PTAL.

[jeremy, 2014-11-05 07:28:11]

LGTM
Could you please manually check this on a Mac to make sure idle wakeup numbers
are still getting reported with this patch?

https://codereview.chromium.org/675843004/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:484: def
_RunTimerStatsUtility(self, start):
_EnableTimerStatsCollection ?
s/start/enable/

[Lei Zhang, 2014-11-06 04:52:49]

Looking for an OWNER to review at this point.

Yes, I tested manually on a Mac and needed to go to patch set 11 to make it
work.

The wakeups reporting is currently tied to the ability to monitor power. I
happen to also have an old Core 2 Duo Mac, where I should be able to get wakeup
numbers, but can't. We can work on that in a separate CL if we care to.

https://codereview.chromium.org/675843004/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/675843004/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:484: def
_RunTimerStatsUtility(self, start):
On 2014/11/05 07:28:11, jeremy wrote:
> _EnableTimerStatsCollection ?
> s/start/enable/

Done.

[Lei Zhang, 2014-11-13 21:47:58]

ping?

[tonyg, 2014-11-14 21:34:03]

https://codereview.chromium.org/675843004/diff/260001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/675843004/diff/260001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:331: def
StartMeasuringIdleWakeUps(self):
The platform API needs to report idle wakeups to the rest of telemetry in a
consistent way regardless of platform. We shouldn't do this scheme where they're
included in cpu stats on mac and here on other platforms. The whole point of the
platform API is to conceal these sorts of details.

Beyond consistency, I don't have super strong opinions about which way to go.
But idle wakeups sure does feel like a cpu stat.

[tonyg, 2015-01-14 18:10:45]

tonyg@chromium.org changed reviewers:
- tonyg@chromium.org

[jdduke (slow), 2015-07-09 21:00:18]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-07-09 21:00:19]

https://codereview.chromium.org/675843004/diff/260001/tools/perf/metrics/powe...
File tools/perf/metrics/power.py (right):

https://codereview.chromium.org/675843004/diff/260001/tools/perf/metrics/powe...
tools/perf/metrics/power.py:173: """Computes number of idle wakeups that
occurred over measurement period.
Hmm, the way /proc/timer_stats collection works I'm not sure we can subtract the
before/after values (as the values are really only valid within the scope of
enabling collection).

Perhaps we could have |StopMeasuringIdleWakeUps| return the total wakeups scoped
by |StartMeasuringIdleWakeups|? So Mac would do subtraction, and Android (Linux)
would just return the raw data from /proc/timer_stats? If we do that, I wonder
if it makes sense (or would be less confusing) to remove IdleWakeupCount as a
member of CPU stats.