Use another file and field for wakeups count

Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=717863
Review-Url: https://codereview.chromium.org/2859763002
Cr-Commit-Position: refs/heads/master@{#469270}
Committed: https://chromium.googlesource.com/chromium/src/+/93c0968c8188befdaeaf1d49a98ad9086fa4a0f3

[sabbakumov, 2017-05-03 02:55:17]

Description was changed from

==========
Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=636823
==========

to

==========
Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=636823
==========

[sabbakumov, 2017-05-03 02:55:17]

sabbakumov@yandex-team.ru changed reviewers:
+ mark@chromium.org

[sabbakumov, 2017-05-03 03:02:07]

The CQ bit was checked by sabbakumov@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 03:02:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 03:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 03:02:42]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Mark Mentovai, 2017-05-03 03:17:17]

mark@chromium.org changed reviewers:
+ thestig@chromium.org

[Mark Mentovai, 2017-05-03 03:17:18]

thestig wrote this code and is also a base OWNER, so I’m adding him.

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
base/process/process_metrics_linux.cc:102: std::string stat_data;
/proc/pid/stat and /proc/pid/status are distinct. This reads status, so all of
the variables that deal with it should be named for that, not stat.

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
base/process/process_metrics_linux.cc:963: static const char kSwitchStat[] =
"voluntary_ctxt_switches";
I haven’t (yet?) verified that this is equivalent or roughly equivalent to
se.statistics.nr_wakeups.

[sabbakumov, 2017-05-03 03:21:02]

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
base/process/process_metrics_linux.cc:102: std::string stat_data;
On 2017/05/03 03:17:17, Mark Mentovai wrote:
> /proc/pid/stat and /proc/pid/status are distinct. This reads status, so all of
> the variables that deal with it should be named for that, not stat.

I used the convention from the ReadProcStatusAndGetFieldAsSizeT function in this
file.

[Mark Mentovai, 2017-05-03 03:30:12]

sabbakumov wrote:
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> File base/process/process_metrics_linux.cc (right):
> 
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> base/process/process_metrics_linux.cc:102: std::string stat_data;
> On 2017/05/03 03:17:17, Mark Mentovai wrote:
> > /proc/pid/stat and /proc/pid/status are distinct. This reads status, so all
of
> > the variables that deal with it should be named for that, not stat.
> 
> I used the convention from the ReadProcStatusAndGetFieldAsSizeT function in
this
> file.

So what? It’s wrong. Don’t perpetuate the error. In fact, since you noticed it,
it’d be nice if you fixed it.

[Mark Mentovai, 2017-05-03 03:31:56]

On 2017/05/03 03:17:18, Mark Mentovai wrote:
> thestig wrote this code and is also a base OWNER, so I’m adding him.
> 
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> File base/process/process_metrics_linux.cc (right):
> 
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> base/process/process_metrics_linux.cc:102: std::string stat_data;
> /proc/pid/stat and /proc/pid/status are distinct. This reads status, so all of
> the variables that deal with it should be named for that, not stat.
> 
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> base/process/process_metrics_linux.cc:963: static const char kSwitchStat[] =
> "voluntary_ctxt_switches";
> I haven’t (yet?) verified that this is equivalent or roughly equivalent to
> se.statistics.nr_wakeups.

I’ve done a small investigation and I don’t believe that voluntary_ctxt_switches
tracks what we’re interested in. Your change description says that you did some
research. Can you share your findings so that we can evaluate whether
voluntary_ctxt_switches is appropriate for
ProcessMetrics::GetIdleWakeupsPerSecond()?

[sabbakumov, 2017-05-03 05:19:48]

On 2017/05/03 03:31:56, Mark Mentovai wrote:
> On 2017/05/03 03:17:18, Mark Mentovai wrote:
> > thestig wrote this code and is also a base OWNER, so I’m adding him.
> > 
> >
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> > File base/process/process_metrics_linux.cc (right):
> > 
> >
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> > base/process/process_metrics_linux.cc:102: std::string stat_data;
> > /proc/pid/stat and /proc/pid/status are distinct. This reads status, so all
of
> > the variables that deal with it should be named for that, not stat.
> > 
> >
>
https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
> > base/process/process_metrics_linux.cc:963: static const char kSwitchStat[] =
> > "voluntary_ctxt_switches";
> > I haven’t (yet?) verified that this is equivalent or roughly equivalent to
> > se.statistics.nr_wakeups.
> 
> I’ve done a small investigation and I don’t believe that
voluntary_ctxt_switches
> tracks what we’re interested in. Your change description says that you did
some
> research. Can you share your findings so that we can evaluate whether
> voluntary_ctxt_switches is appropriate for
> ProcessMetrics::GetIdleWakeupsPerSecond()?

First of all, file /proc/<pid>/sched doesn't appear on man pages (man proc).
That
means we can't rely even on it's existence.
For investigation I used the following commands running on different terminals
on the system where /proc/<pid>/sched is available:

watch -n 1 'cat /proc/<pid>/sched | grep "se.statistics.nr_wakeups\s"'
this on the first one.

watch -n 1 'cat /proc/<pid>/status | grep "^voluntary_ctxt_switches"'
this on the second one.

I chose Chrome's main process to watch for.
Every second I got about the same numbers.

[sabbakumov, 2017-05-03 05:20:05]

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2859763002/diff/1/base/process/process_metric...
base/process/process_metrics_linux.cc:102: std::string stat_data;
On 2017/05/03 03:17:17, Mark Mentovai wrote:
> /proc/pid/stat and /proc/pid/status are distinct. This reads status, so all of
> the variables that deal with it should be named for that, not stat.

Done.

[Lei Zhang, 2017-05-03 05:47:03]

I only took a quick look, and I have some questions below. I can take a deeper
look at se.statistics.nr_wakeups vs voluntary_ctxt_switches tomorrow.

1) Why did you associate this CL with https://crbug.com/636823 ? That is a Mac
bug and this CL touches Linux code.

2) Who built this 4.8.0-49-generic kernel of yours? Is it a stock Ubuntu kernel?

a)
http://packages.ubuntu.com/search?suite=trusty-updates&searchon=names&keyword...
shows no 4.8 kernel for Ubuntu 14.04
b)
http://packages.ubuntu.com/search?suite=xenial-updates&searchon=names&keyword...
shows Ubuntu 16.04 has a 4.8 kernel, but the latest package is
linux-image-4.8.0-45. Where did you get -49?

[sabbakumov, 2017-05-03 05:56:07]

Description was changed from

==========
Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=636823
==========

to

==========
Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=717863
==========

[sabbakumov, 2017-05-03 06:01:07]

On 2017/05/03 05:47:03, Lei Zhang wrote:
> I only took a quick look, and I have some questions below. I can take a deeper
> look at se.statistics.nr_wakeups vs voluntary_ctxt_switches tomorrow.
> 
> 1) Why did you associate this CL with https://crbug.com/636823 ? That is a Mac
> bug and this CL touches Linux code.
> 
> 2) Who built this 4.8.0-49-generic kernel of yours? Is it a stock Ubuntu
kernel?
> 
> a)
>
http://packages.ubuntu.com/search?suite=trusty-updates&searchon=names&keyword...
> shows no 4.8 kernel for Ubuntu 14.04
> b)
>
http://packages.ubuntu.com/search?suite=xenial-updates&searchon=names&keyword...
> shows Ubuntu 16.04 has a 4.8 kernel, but the latest package is
> linux-image-4.8.0-45. Where did you get -49?

1) I changed the issue.
2) Yes, this is the Ubuntu stock kernel. You can check it out using this
command:
apt-cache search linux-image-4.8

[Lei Zhang, 2017-05-03 06:11:18]

On 2017/05/03 06:01:07, sabbakumov wrote:
> 2) Yes, this is the Ubuntu stock kernel. You can check it out using this
> command:
> apt-cache search linux-image-4.8

Thanks. I guess packages.ubuntu.com is slightly out of date.

[Mark Mentovai, 2017-05-03 16:41:50]

I misread this the old code last night and thought that we were using
se.statistics.nr_wakeups.idle. se.statistics.nr_wakeups should indeed be close
enough to voluntary_ctxt_switches for our purposes. LGTM, and thanks for the
patch.

[Mark Mentovai, 2017-05-03 19:44:07]

Here’s what happened to se.statistics.nr_wakeups effective in Linux 4.6:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id...

[Lei Zhang, 2017-05-03 21:05:26]

lgtm

Thanks Mark for looking up the kernel change.

https://codereview.chromium.org/2859763002/diff/20001/base/process/process_me...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2859763002/diff/20001/base/process/process_me...
base/process/process_metrics_linux.cc:96: // Read /proc/<pid>/status and look
for |field|. On succes, return true and
BTW, there's a typo: "succes". Do you mind fixing it while we are here?

[sabbakumov, 2017-05-04 04:10:52]

https://codereview.chromium.org/2859763002/diff/20001/base/process/process_me...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2859763002/diff/20001/base/process/process_me...
base/process/process_metrics_linux.cc:96: // Read /proc/<pid>/status and look
for |field|. On succes, return true and
On 2017/05/03 21:05:26, Lei Zhang wrote:
> BTW, there's a typo: "succes". Do you mind fixing it while we are here?

Done.

[sabbakumov, 2017-05-04 04:11:31]

The CQ bit was checked by sabbakumov@yandex-team.ru

[sabbakumov, 2017-05-04 04:11:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2859763002/#ps40001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-05-04 04:11:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 05:10:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493871091592430,
"parent_rev": "a3874ecb64ef003b3ec76acd0ea53baae8eb73d2", "commit_rev":
"93c0968c8188befdaeaf1d49a98ad9086fa4a0f3"}

[commit-bot: I haz the power, 2017-05-04 05:11:48]

Description was changed from

==========
Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=717863
==========

to

==========
Use another file and field for wakeups count

On some Linux systems (like Ubuntu 14.04.5 or Ubuntu 16.04) with
4.8.0-49-generic kernel there's no /proc/<pid>/sched file. Or it exists
but doesn't have the required se.statistics.nr_wakeups field.
This causes TaskManagerBrowserTest.IdleWakeups test to fail and in
Chrome Task Manager all the Idle Wakeups counts are 0.

After some research I found out that the voluntary_ctxt_switches field
in file /proc/<pid>/status is practically the same as the previous
one. Using that field helps the test to pass.

BUG=717863

Review-Url: https://codereview.chromium.org/2859763002
Cr-Commit-Position: refs/heads/master@{#469270}
Committed:
https://chromium.googlesource.com/chromium/src/+/93c0968c8188befdaeaf1d49a98a...
==========

[commit-bot: I haz the power, 2017-05-04 05:11:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/93c0968c8188befdaeaf1d49a98a...