You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Cong Wang <cw...@twopensource.com> on 2015/09/03 00:16:47 UTC
Re: Review Request 37540: Add perf event API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/
-----------------------------------------------------------
(Updated Sept. 2, 2015, 10:16 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Summary (updated)
-----------------
Add perf event API
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Abstract Linux kernel perf event API and provide API to collect schedule events.
Diffs
-----
src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
Diff: https://reviews.apache.org/r/37540/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 15, 2015, 12:31 a.m., Vinod Kone wrote:
> > my main question is regarding division of responsibilities between the wrapper class and the process class. Looks like opening the fd and mmaping is done by the wrapper and the rest by the process. Can everything be done by the process?
Hmm, but why should we expose the process class to user? With current code, user only sees the wrapper. Also, there are many similar code in src/linux/cgroup.cpp, pretty much because of the async IO interface in Mesos code base.
> On Sept. 15, 2015, 12:31 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 747
> > <https://reviews.apache.org/r/37540/diff/6/?file=1064305#file1064305line747>
> >
> > is there any reason why we want close() as part of the public API? sounds like users can simply call delete on the PerfEvent object? i would recommend removing this method or making it private.
> >
> > more importantly, what happens if close() is called twice? is that safe?
Good point. Fixed, just FYI it is a smart pointer, user should call reset() not delete.
> On Sept. 15, 2015, 12:31 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, lines 749-752
> > <https://reviews.apache.org/r/37540/diff/6/?file=1064305#file1064305line749>
> >
> > can this be done by the process instead?
Probably we can do it in finialize() too, not sure what is the benifit over this?
> On Sept. 15, 2015, 12:31 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 563
> > <https://reviews.apache.org/r/37540/diff/6/?file=1064305#file1064305line563>
> >
> > sharing the map between this class and the constituent process class is unfortunate. what happens if the process class deletes the pointer?
> >
> > is it possible to have the process class create and own the map?
The map is created when user calls create(), then it is passed to the process class, so the process class only reads it when user calls read().
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review98924
-----------------------------------------------------------
On Sept. 4, 2015, 11:13 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724
> src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review98924
-----------------------------------------------------------
my main question is regarding division of responsibilities between the wrapper class and the process class. Looks like opening the fd and mmaping is done by the wrapper and the rest by the process. Can everything be done by the process?
src/linux/perf.hpp (line 92)
<https://reviews.apache.org/r/37540/#comment155724>
const &
src/linux/perf.hpp (line 95)
<https://reviews.apache.org/r/37540/#comment155649>
s/, returns/. Returns/
src/linux/perf.hpp (line 96)
<https://reviews.apache.org/r/37540/#comment155650>
s/,/;/
s/cgroups/cgroup names/ ?
src/linux/perf.hpp (line 99)
<https://reviews.apache.org/r/37540/#comment155725>
const
src/linux/perf.hpp (line 126)
<https://reviews.apache.org/r/37540/#comment155726>
const &
src/linux/perf.cpp (line 508)
<https://reviews.apache.org/r/37540/#comment155701>
s/, after/; after/
src/linux/perf.cpp (line 562)
<https://reviews.apache.org/r/37540/#comment155710>
sharing the map between this class and the constituent process class is unfortunate. what happens if the process class deletes the pointer?
is it possible to have the process class create and own the map?
src/linux/perf.cpp (line 564)
<https://reviews.apache.org/r/37540/#comment155702>
don't think you need CHECK_NOTNULL here because you are setting 'process' variable right above.
also indent by 2 spaces instead of 4.
src/linux/perf.cpp (lines 605 - 610)
<https://reviews.apache.org/r/37540/#comment155735>
indent these by 4 spaces from the beginning of the column (i.e., 4 spaces from 'void').
src/linux/perf.cpp (line 628)
<https://reviews.apache.org/r/37540/#comment155730>
CHECK_NOTNULL(attr); ?
src/linux/perf.cpp (line 660)
<https://reviews.apache.org/r/37540/#comment155729>
CHECK_NOTNULL(attr)?
src/linux/perf.cpp (line 699)
<https://reviews.apache.org/r/37540/#comment155713>
The fd returned by os::open() above is the pid of the cgroup? It's worth adding a comment.
src/linux/perf.cpp (line 746)
<https://reviews.apache.org/r/37540/#comment155709>
is there any reason why we want close() as part of the public API? sounds like users can simply call delete on the PerfEvent object? i would recommend removing this method or making it private.
more importantly, what happens if close() is called twice? is that safe?
src/linux/perf.cpp (lines 748 - 751)
<https://reviews.apache.org/r/37540/#comment155721>
can this be done by the process instead?
src/linux/perf.cpp (line 754)
<https://reviews.apache.org/r/37540/#comment155708>
default is 'true'. so no need to explicitly specify it.
- Vinod Kone
On Sept. 4, 2015, 11:13 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724
> src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 22, 2015, 12:37 a.m., Jie Yu wrote:
> > src/linux/perf.cpp, lines 516-529
> > <https://reviews.apache.org/r/37540/diff/7/?file=1077025#file1077025line516>
> >
> > Should we use `__sync_synchronize` instead?
Hmm, __sync_synchronize is documented as a full memory barrier, while we only need a read memory barrier here... but this isn't a very hot path, so I can replace it.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review99882
-----------------------------------------------------------
On Sept. 18, 2015, 10:11 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review99882
-----------------------------------------------------------
src/linux/perf.cpp (lines 515 - 528)
<https://reviews.apache.org/r/37540/#comment156898>
Should we use `__sync_synchronize` instead?
- Jie Yu
On Sept. 18, 2015, 10:11 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review106926
-----------------------------------------------------------
src/linux/perf.cpp (line 514)
<https://reviews.apache.org/r/37540/#comment165731>
s/Gcc/GCC/
src/linux/perf.cpp (line 516)
<https://reviews.apache.org/r/37540/#comment165732>
s/use/Use/
s/gcc/GCC/
src/linux/perf.cpp (line 590)
<https://reviews.apache.org/r/37540/#comment165742>
Lets add a comment to PerfEvent::read() (the interface) about what the semantics are for simultaneous read.
src/linux/perf.cpp (line 593)
<https://reviews.apache.org/r/37540/#comment165744>
i think you need a .onDiscard() handler here.
src/linux/perf.cpp (line 597)
<https://reviews.apache.org/r/37540/#comment165745>
so if one of the fds in the map is ready `_read()` is called? what happens when another fd in the map becomes ready?
src/linux/perf.cpp (line 602)
<https://reviews.apache.org/r/37540/#comment165746>
if io:poll() fails on an fd, you terminate the process but if it gets discarded (who discards it?), the process continues?
src/linux/perf.cpp (line 871)
<https://reviews.apache.org/r/37540/#comment165734>
instead of two colons in the error message, do
return Error("Failed to create PerfEvent for cgroup '" + cgroup + "': " + event.error());
- Vinod Kone
On Sept. 30, 2015, 12:12 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 12:12 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Dec. 2, 2015, 10:13 a.m., Niklas Nielsen wrote:
> > Ping - Cong, is there anything you need to close the last issues? :)
>
> Cong Wang wrote:
> I was blocked at the second to the last issue above and switched to something else, now I can think more about it. Thanks!
Sweet! Let us know when you are ready for new review rounds :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review108686
-----------------------------------------------------------
On Sept. 29, 2015, 5:12 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 5:12 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Dec. 2, 2015, 6:13 p.m., Niklas Nielsen wrote:
> > Ping - Cong, is there anything you need to close the last issues? :)
I was blocked at the second to the last issue above and switched to something else, now I can think more about it. Thanks!
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review108686
-----------------------------------------------------------
On Sept. 30, 2015, 12:12 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 12:12 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review108686
-----------------------------------------------------------
Ping - Cong, is there anything you need to close the last issues? :)
- Niklas Nielsen
On Sept. 29, 2015, 5:12 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 5:12 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/
-----------------------------------------------------------
(Updated Sept. 30, 2015, 12:12 a.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Address review comments
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Abstract Linux kernel perf event API and provide API to collect schedule events.
Diffs (updated)
-----
src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
Diff: https://reviews.apache.org/r/37540/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 25, 2015, 9:53 p.m., Vinod Kone wrote:
> > src/linux/perf.hpp, line 110
> > <https://reviews.apache.org/r/37540/diff/7/?file=1077024#file1077024line110>
> >
> > put the braces on the same line. more importantly, do you actually need a default constructor?
> >
> > also, do you want this class to be copyable or assignable? if not, make those constructors private too.
I don't need a default constructor, just need to prevent users using it like "PerfEvent event;", that is calling create() is the only way to construct it. Not sure if I need to make more constructors here since create() returns a (smart) pointer.
> On Sept. 25, 2015, 9:53 p.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 532
> > <https://reviews.apache.org/r/37540/diff/7/?file=1077025#file1077025line532>
> >
> > i would move this function close to _create(), where it is used.
It would be used by other patch (for MESOS-3399) too. Also, not sure if we gain more readability by moving it, since that would be put it between two PerfEvent member function definitions.
> On Sept. 25, 2015, 9:53 p.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 570
> > <https://reviews.apache.org/r/37540/diff/7/?file=1077025#file1077025line570>
> >
> > 1 blank line.
Fixed. And opened https://issues.apache.org/jira/browse/MESOS-3523 for future.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review100656
-----------------------------------------------------------
On Sept. 18, 2015, 10:11 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 25, 2015, 9:53 p.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 792
> > <https://reviews.apache.org/r/37540/diff/7/?file=1077025#file1077025line792>
> >
> > so the semantics of this read() are a bit confusing.
> >
> > if the caller calls the read() back to back, they will get the same event if the first io::poll() is still running? but they will get a different event if io::poll() has finished before the 2nd read() is called?
That is correct. I want to ensure two different threads can get the same result when they read from the same perf event handler.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review100656
-----------------------------------------------------------
On Sept. 18, 2015, 10:11 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review100656
-----------------------------------------------------------
src/linux/perf.hpp (line 96)
<https://reviews.apache.org/r/37540/#comment157920>
s/pathes/paths/
src/linux/perf.hpp (line 107)
<https://reviews.apache.org/r/37540/#comment157886>
one blank line.
src/linux/perf.hpp (line 110)
<https://reviews.apache.org/r/37540/#comment157890>
put the braces on the same line. more importantly, do you actually need a default constructor?
also, do you want this class to be copyable or assignable? if not, make those constructors private too.
src/linux/perf.cpp (line 531)
<https://reviews.apache.org/r/37540/#comment157928>
i would move this function close to _create(), where it is used.
src/linux/perf.cpp (lines 564 - 565)
<https://reviews.apache.org/r/37540/#comment157926>
please define this constructor outside the class as you did with the rest of the methods.
src/linux/perf.cpp (line 569)
<https://reviews.apache.org/r/37540/#comment157911>
1 blank line.
src/linux/perf.cpp (line 573)
<https://reviews.apache.org/r/37540/#comment157912>
1 blank line.
src/linux/perf.cpp (line 579)
<https://reviews.apache.org/r/37540/#comment157932>
no need for underscore.
src/linux/perf.cpp (line 581)
<https://reviews.apache.org/r/37540/#comment157931>
no need for underscore.
src/linux/perf.cpp (line 590)
<https://reviews.apache.org/r/37540/#comment157909>
I would define PerfEvent class methods after defining PerfEventProcess class methods.
Also, please order the method definitions in the same order as their declarations.
src/linux/perf.cpp (line 591)
<https://reviews.apache.org/r/37540/#comment157902>
indent by two spaces.
actually, you can even put this right above spawn if you like. that's what we do in most of our wrappers, afaict.
src/linux/perf.cpp (line 604)
<https://reviews.apache.org/r/37540/#comment157927>
Add a comment?
src/linux/perf.cpp (line 633)
<https://reviews.apache.org/r/37540/#comment157917>
instead of CHECK return an Error here?
src/linux/perf.cpp (line 667)
<https://reviews.apache.org/r/37540/#comment157918>
ditto. return Error.
src/linux/perf.cpp (line 679)
<https://reviews.apache.org/r/37540/#comment157916>
new line here.
src/linux/perf.cpp (line 717)
<https://reviews.apache.org/r/37540/#comment157919>
new line.
src/linux/perf.cpp (line 750)
<https://reviews.apache.org/r/37540/#comment157924>
The caller of this method doesn't know which cgroup failed, so how about including that info in the error?
return Error("Failed to create PerfEvent for cgroup '" + cgroup + "': " + event.error());
src/linux/perf.cpp (line 782)
<https://reviews.apache.org/r/37540/#comment157954>
do you want an onDiscard() handler for this future? what should happen if the caller does a futuer.discard()?
src/linux/perf.cpp (line 791)
<https://reviews.apache.org/r/37540/#comment157956>
so the semantics of this read() are a bit confusing.
if the caller calls the read() back to back, they will get the same event if the first io::poll() is still running? but they will get a different event if io::poll() has finished before the 2nd read() is called?
src/linux/perf.cpp (line 827)
<https://reviews.apache.org/r/37540/#comment157934>
s/reads,/reads./
src/linux/perf.cpp (line 871)
<https://reviews.apache.org/r/37540/#comment157955>
can we get the failure reason from io::poll?
- Vinod Kone
On Sept. 18, 2015, 10:11 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 10:11 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
> src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
> src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/
-----------------------------------------------------------
(Updated Sept. 18, 2015, 10:11 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Address review comments, rebase and cleanup
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Abstract Linux kernel perf event API and provide API to collect schedule events.
Diffs (updated)
-----
src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a
src/tests/containerizer/perf_tests.cpp ed5212ee31b8aa47339b8b8fab184bbdf85be82a
Diff: https://reviews.apache.org/r/37540/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/
-----------------------------------------------------------
(Updated Sept. 4, 2015, 11:13 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Address review comments
Bugs: MESOS-2769
https://issues.apache.org/jira/browse/MESOS-2769
Repository: mesos
Description
-------
Abstract Linux kernel perf event API and provide API to collect schedule events.
Diffs (updated)
-----
src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5
src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724
src/tests/containerizer/perf_tests.cpp bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38
Diff: https://reviews.apache.org/r/37540/diff/
Testing
-------
make check
Thanks,
Cong Wang
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 3, 2015, 12:46 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 819
> > <https://reviews.apache.org/r/37540/diff/5/?file=1062757#file1062757line819>
> >
> > who can discard this future?
finalize().
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review97548
-----------------------------------------------------------
On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 10:16 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
> src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 3, 2015, 12:46 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, line 791
> > <https://reviews.apache.org/r/37540/diff/5/?file=1062757#file1062757line791>
> >
> > why is __read() a separate function? can you just pull the logic down here?
__read() is used by the next patch.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review97548
-----------------------------------------------------------
On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 10:16 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
> src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 3, 2015, 12:46 a.m., Vinod Kone wrote:
> > src/linux/perf.hpp, line 119
> > <https://reviews.apache.org/r/37540/diff/5/?file=1062756#file1062756line119>
> >
> > We use "_" prefix for continuation. Since this is just a private overload, I would remove "_".
Compile failed after removing the "_":
../../3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp: In instantiation of 'Option<T>::Option(_Some<U>&&) [with U = Try<process::Owned<perf::PerfEvent> >; T = hashmap<int, perf_event_mmap_page*>]':
../../3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp:53:19: required from 'Try<T>::Try(const U&) [with U = Try<process::Owned<perf::PerfEvent> >; T = hashmap<int, perf_event_mmap_page*>]'
../../src/linux/perf.cpp:660:12: required from here
../../3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp:53:61: error: no matching function for call to 'hashmap<int, perf_event_mmap_page*>::hashmap(std::remove_reference<Try<process::Owned<perf::PerfEvent> >&>::type)'
Option(_Some<U>&& some) : state(SOME), t(std::move(some.t)) {}
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review97548
-----------------------------------------------------------
On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 10:16 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
> src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Cong Wang <cw...@twopensource.com>.
> On Sept. 3, 2015, 12:46 a.m., Vinod Kone wrote:
> > src/linux/perf.hpp, lines 84-89
> > <https://reviews.apache.org/r/37540/diff/5/?file=1062756#file1062756line84>
> >
> > Do we need this overload considering users can call the overload below?
I think it is still useful, because the return value of the other overload is a hashmap, not convenient to use for a single cgroup.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review97548
-----------------------------------------------------------
On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 10:16 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
> src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 37540: Add perf event API
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37540/#review97548
-----------------------------------------------------------
src/linux/perf.hpp (line 71)
<https://reviews.apache.org/r/37540/#comment153551>
we typically name the internal process with a process suffix. s/Listener/Process/
src/linux/perf.hpp (line 77)
<https://reviews.apache.org/r/37540/#comment153530>
s/.././
src/linux/perf.hpp (line 78)
<https://reviews.apache.org/r/37540/#comment153531>
put the argument on the next line.
src/linux/perf.hpp (lines 84 - 89)
<https://reviews.apache.org/r/37540/#comment153532>
Do we need this overload considering users can call the overload below?
src/linux/perf.hpp (lines 88 - 89)
<https://reviews.apache.org/r/37540/#comment153535>
swap the arguments.
src/linux/perf.hpp (lines 95 - 96)
<https://reviews.apache.org/r/37540/#comment153534>
Swap the arguments.
src/linux/perf.hpp (line 111)
<https://reviews.apache.org/r/37540/#comment153536>
argument on next line.
src/linux/perf.hpp (line 119)
<https://reviews.apache.org/r/37540/#comment153552>
We use "_" prefix for continuation. Since this is just a private overload, I would remove "_".
src/linux/perf.cpp (lines 471 - 484)
<https://reviews.apache.org/r/37540/#comment153575>
What is this for? Comments?
src/linux/perf.cpp (lines 496 - 497)
<https://reviews.apache.org/r/37540/#comment153577>
kill this?
src/linux/perf.cpp (lines 501 - 503)
<https://reviews.apache.org/r/37540/#comment153576>
kill this?
src/linux/perf.cpp (lines 514 - 516)
<https://reviews.apache.org/r/37540/#comment153580>
swap the order.
src/linux/perf.cpp (line 522)
<https://reviews.apache.org/r/37540/#comment153579>
why inline?
should this be static?
also, why is this defined here instead of outside the class as you did for other methods?
src/linux/perf.cpp (line 541)
<https://reviews.apache.org/r/37540/#comment153582>
typically constructor and destructor are defined first.
src/linux/perf.cpp (lines 547 - 549)
<https://reviews.apache.org/r/37540/#comment153601>
combine these.
int ret = ...
src/linux/perf.cpp (lines 574 - 576)
<https://reviews.apache.org/r/37540/#comment153602>
indent arguments.
src/linux/perf.cpp (line 577)
<https://reviews.apache.org/r/37540/#comment153603>
new line.
src/linux/perf.cpp (lines 622 - 623)
<https://reviews.apache.org/r/37540/#comment153600>
kill this. see below.
src/linux/perf.cpp (line 627)
<https://reviews.apache.org/r/37540/#comment153599>
you can just use an initializer list here
{cpu}
src/linux/perf.cpp (line 630)
<https://reviews.apache.org/r/37540/#comment153598>
new line please.
src/linux/perf.cpp (line 720)
<https://reviews.apache.org/r/37540/#comment153597>
s/pgsz/pageSize/
src/linux/perf.cpp (line 742)
<https://reviews.apache.org/r/37540/#comment153585>
these should be defined in the following order
```
read()
_read()
__read()
```
src/linux/perf.cpp (line 746)
<https://reviews.apache.org/r/37540/#comment153595>
s/pgsz/pageSize/
s/pgmsk/pageMask/ ?
src/linux/perf.cpp (line 790)
<https://reviews.apache.org/r/37540/#comment153593>
why is __read() a separate function? can you just pull the logic down here?
src/linux/perf.cpp (line 817)
<https://reviews.apache.org/r/37540/#comment153587>
put the .onReady() on the next line.
src/linux/perf.cpp (line 818)
<https://reviews.apache.org/r/37540/#comment153592>
who can discard this future?
src/tests/containerizer/perf_tests.cpp (line 126)
<https://reviews.apache.org/r/37540/#comment153538>
s/inherit/Inherit/
src/tests/containerizer/perf_tests.cpp (line 128)
<https://reviews.apache.org/r/37540/#comment153539>
argument on next line.
Can you add a comment on what this is trying to create? Not clear at all based on the arguments.
src/tests/containerizer/perf_tests.cpp (line 146)
<https://reviews.apache.org/r/37540/#comment153540>
ditto.
argument on next line.
also, needs a comment on what this is creating.
src/tests/containerizer/perf_tests.cpp (line 151)
<https://reviews.apache.org/r/37540/#comment153542>
ASSERT_SOME(event);
src/tests/containerizer/perf_tests.cpp (lines 156 - 157)
<https://reviews.apache.org/r/37540/#comment153548>
s/context/Context/
Can you add a better comment on what you are tryign to do here. "Causing 2 context switches so that..?
- Vinod Kone
On Sept. 2, 2015, 10:16 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 10:16 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Abstract Linux kernel perf event API and provide API to collect schedule events.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp c44445630118f4858b1a805f25a2db7a24ca0989
> src/linux/perf.cpp bb8b5918e4c8f4379784d4fa18c3b4709696e1ea
> src/tests/containerizer/perf_tests.cpp 8c29ca3dc2a8f6d1fb0f430482190f87f08eb4e8
>
> Diff: https://reviews.apache.org/r/37540/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Cong Wang
>
>