----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5230/#review8219 -----------------------------------------------------------
src/linux/cgroups.hpp <https://reviews.apache.org/r/5230/#comment17803> Move the '{' to a new line (here and the rest of this patch please). src/linux/cgroups.hpp <https://reviews.apache.org/r/5230/#comment17817> If you want to expose this information then we need to add more documentation and make it "understandable". In particular, all this code tells me is that there are multiple "entries" in /proc/cgroups and that each entry has a subsystem name, hierarchy, number of cgroups, and enabled bit. But given just that I do not know how any other semantics. For example, is there an entry for each subsystem that is present in each hierarchy? Your map of 'entries' below would tell me no, but then I'm totally confused by why there is a hierarchy number (I thought there could be multiple hierarchies and a subsystem could be attached to more than one hierarchy if it was the only subsystem attached to each hierarchy). Again, I think we want some higher-level abstractions here. In particular, I think we want to be able to "ask" certain questions. For example, 'what subsystems are enabled on this machine?' (e.g., cgroups::subsystems() as you have below). Or, given a hierarchy, 'what subsystems are attached to that hierarchy?'. Or, given a cgroup, 'what hierarchy is it a part of?'. Or, given a task, 'which cgroups is it a part of?'. From just these one could even construct a "cross-cutting" question, e.g., what subsystems are controlling this task (determined by (1) getting cgroups the task is a part of then (2) what hierarchy the cgroups are a part of and then (3) what subsystems are attached to that hierarchy). My guess is that these will be much more useful and intuitive then forcing clients of this API to work with the low-level /proc/cgroups format. src/linux/cgroups.hpp <https://reviews.apache.org/r/5230/#comment17818> Just a thought. My hunch is that the cgroups_isolation_module will probably just use a single hierarchy. It's possible that when we introduce net_cls we'll have multiple hierarchies, one for each network class we care about. But for now, it seems like we want all subsytems attached to a single hierarchy. Or maybe I'm missing something? src/linux/cgroups.cpp <https://reviews.apache.org/r/5230/#comment17819> Again, let's not use 'rv' here. In this case, you can just call this thing 'info'. - Benjamin On 2012-05-30 05:17:23, Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5230/ > ----------------------------------------------------------- > > (Updated 2012-05-30 05:17:23) > > > Review request for mesos and Benjamin Hindman. > > > Summary > ------- > > These functions are very basic functions that are required to control Linux > cgroups. See src/linux.cgroups.h|cpp for details. > > This patch should be applied after https://reviews.apache.org/r/5186/ is > applied. > > > Diffs > ----- > > src/Makefile.am 96cb4c6 > src/linux/cgroups.hpp PRE-CREATION > src/linux/cgroups.cpp PRE-CREATION > src/tests/cgroups_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/5230/diff > > > Testing > ------- > > On Linux machines, make check. > > Manual tests for some functions since root permission is required. > > > Thanks, > > Jie > >
