> On 2012-05-30 17:39:15, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 51 > > <https://reviews.apache.org/r/5230/diff/2/?file=110712#file110712line51> > > > > Move the '{' to a new line (here and the rest of this patch please).
Done. > On 2012-05-30 17:39:15, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 56 > > <https://reviews.apache.org/r/5230/diff/2/?file=110712#file110712line56> > > > > 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. In fact, I don't want to expose these to user. How to make them internal to this module? static for functions? what about structs? Here is my plan: (1) I want to make the following functions internal to this module: (internal functions) info/info(pid) createHierarch/removeHierarchy createCgroup/removeCgroup readControl/writeControl (2) I want to expose the following functions to user: (external functions) subsystems() enabled() enabled(subsystem) ... (there will be more like you suggested, but these functions will use the above internal functions) I am currently working on cgroups_isolation_module. More external functions will be added to this module to satisfy the needs of the isolation module. --- "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"' No, a subsystem can only be attached to one hierarchy. But a hierarchy can have more than one subsystems been attached. > On 2012-05-30 17:39:15, Benjamin Hindman wrote: > > src/linux/cgroups.hpp, line 164 > > <https://reviews.apache.org/r/5230/diff/2/?file=110712#file110712line164> > > > > 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? Yes, you are right. We will just create a single hierarchy root (e.g. /mnt/cgroups) and attach all subsystems (e.g. cpu, memory) to this hierarchy for now. > On 2012-05-30 17:39:15, Benjamin Hindman wrote: > > src/linux/cgroups.cpp, line 43 > > <https://reviews.apache.org/r/5230/diff/2/?file=110713#file110713line43> > > > > Again, let's not use 'rv' here. In this case, you can just call this > > thing 'info'. Done. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5230/#review8219 ----------------------------------------------------------- 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 > >
