mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cong Wang <xiyou.wangc...@gmail.com>
Subject Re: Review Request 38074: Calculate schedule latency with trace events
Date Fri, 26 Feb 2016 21:32:54 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message