> On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.hpp, line 97 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087487#file1087487line97> > > > > linux/cgroups has an internal {{Result<string> cgroup(pid_t pid, const > > string& subsystem)}} that is wrapped for cpu, cpuacct, etc. Can you add an > > wrapper for the perf_event cgroup and use that? Yes, it may be less > > efficient than looking at the hashmap, but it'll reuse the code and keep > > consistency.
We need to cache that value before sampling the events, we can't call cgroup() each time when we lookup the pid, because cgroup() always reads from the cgroup tasks file. > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, lines 472-482 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line472> > > > > remove this, see earlier comment. Removed after switching to pid -> cgroup mapping. > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 486 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line486> > > > > Suggest that you use a hashmap<pid_t, string>, i.e., just directly > > store the cgroup for each pid. Slight tradeoff in size but then you can > > avoid findCgroupByPid() and simplify the code. Done. But note that with pid namespace enabled we could have duplicated pid's from different containers. I just add a TODO there. > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 502 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line502> > > > > You don't actually use this as a map? It would be much clearer to use a > > vector and explicitly sort with a comparision function on the timestamp. > > Suggest just calling it events. > > > > ``` > > events.push_back(event); > > } > > > > std::sort(begin(events), end(events), [](...){}); > > ``` I don't see any advantage of sorting it to just using std::map, therefore I keep as it is. > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 547 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line547> > > > > Are states enumerated somewhere? Ideally kernel should export these values, but it doesn't. We could enumerate by ourselves, but here we only care about RUNNING which is 0, so... > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 560 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line560> > > > > sort the latencies in schedLatency here? I don't see any advantage to sort them here than later. > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, lines 625-627 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line625> > > > > How expensive is this...? Depends on the workload, we could have tens of thousands events on a busy system. > On Nov. 18, 2015, 12:51 a.m., Ian Downes wrote: > > src/slave/containerizer/isolators/cgroups/perf_event.cpp, lines 629-632 > > <https://reviews.apache.org/r/38074/diff/5/?file=1087488#file1087488line629> > > > > Users have different kernels, code should determine the version at run > > time and act accordingly. That means we would need two separated code to handle two kernel versions. - Cong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38074/#review106958 ----------------------------------------------------------- On Sept. 30, 2015, 12:15 a.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38074/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2015, 12:15 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 > ------- > > Finally, calculate schedule latency with the sched trace events, and add it > to our statistics > > > Diffs > ----- > > include/mesos/mesos.proto 4a16be1f570769f3ce42a50a9da9f4fb1c227999 > src/slave/containerizer/isolators/cgroups/perf_event.hpp > 1f722ef3ef7ab7fce5542d4affae961d6cec2406 > src/slave/containerizer/isolators/cgroups/perf_event.cpp > 03035dfbfb84470ba39ed9ecfd1eb73890e3f784 > src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 > src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 > > Diff: https://reviews.apache.org/r/38074/diff/ > > > Testing > ------- > > manual tests > > > Thanks, > > Cong Wang > >
