> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.hpp, line 49 > > <https://reviews.apache.org/r/8108/diff/5/?file=222258#file222258line49> > > > > s/CPUSet/Cpuset/g > > > > I'd prefer this name because we're trying to capture the "cpuset" > > subsystem/abstraction as referenced by the Linux kernel documentation.
> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 656 > > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line656> > > > > Hmm, I'm confused. The very first time we invoke cpusetChanged > > info->cpuset.get().usage() should be 0 and so delta will be a negative > > number. But in this case we aren't shrinking, we are growing. I think you > > want to swap the operands. Nice catch. > On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 112 > > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line112> > > > > What about just using delta and subtracting from that? You can do the > > same above easily. Then here you'd check via: > > > > while (delta > 0.0) { > > > > } Done, ironically, I didn't do it that way because I seemed to remember a review in which you didn't like me mutating a parameter like that ;) > On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.hpp, line 151 > > <https://reviews.apache.org/r/8108/diff/5/?file=222258#file222258line151> > > > > You probably don't want to make this an Option (see comments below). If > > you make it a pointer, then you can also stick the Cpuset class into the > > .cpp. Nice catch. > On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 347 > > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line347> > > > > The use of this variable is quite far away from the > > declaration/definition, FYI. Moved. > On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, line 351 > > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line351> > > > > Yes, this really ought to be factored out. I'll take a look for you. Thanks. > On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote: > > src/slave/cgroups_isolation_module.cpp, lines 396-398 > > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line396> > > > > If I'm reading this correctly, this is an unnecessary optimization that > > requires me to think harder to make sure this loop is correct. Given that > > it probably doesn't save much time, is only executed once, and requires the > > extra thinking to make sure it's correct means it should probably be > > dropped. You're not reading this correctly ;) This is an essential part of the logic, in that, we don't want to use more cpus than the slave resources specified. That's why this loop stops adding cpus when it surpasses the amount specified in the slave --resources. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8108/#review13678 ----------------------------------------------------------- On Nov. 20, 2012, 9:03 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8108/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2012, 9:03 p.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > This is the first pass at adding cpuset isolation for pinning cgroups to cpus. > > We decided to start with a simplistic grow/shrink allocation technique, as > such this initial technique: > -Does not take cache locality into account. > -Does not actively fight fragmentation*, but does a good job at preventing > it in many cases, given it's simplicity. > -Note that when cpus resource requests are integral (non-fractional), then > fragmentation does not occur. > > *By fragmentation, I'm referring to the case where we've spread a cgroup over > more cpus than necessary, due to other cgroups sharing the same cpus. > High fragmentation would mean a lot of shared cpus across cgroups. > No fragmentation would mean each cgroup has a unique set of cpus. > > I've punted on documenting the pitfalls of this technique, wiring up the > handler, and adding tests for now. > > Note that this is diffed off of benh's changes: > https://reviews.apache.org/r/8058/ > https://reviews.apache.org/r/8059/ > > > Diffs > ----- > > src/linux/proc.hpp 27e15bf8695aa694b0d5bdb6881b9fa55a447528 > src/slave/cgroups_isolation_module.hpp > 9f80fc5a969b959b34eaea4cac40700662d7f8b2 > src/slave/cgroups_isolation_module.cpp > 8211618d7729350654e2d17946c5b912ed9dda6a > src/slave/isolation_module.hpp 4e7bfee4205b2a9953c06c7cc7128c9400ff79f4 > src/slave/lxc_isolation_module.hpp 49bf8741b96f58202acb737917e6cc353e758893 > src/slave/lxc_isolation_module.cpp 1d0a4c47386eedf610b10d40ad5300ff808cc1fd > src/slave/process_based_isolation_module.hpp > efe59ebc0e8120926ea9f36b9eaa2f0b25830faf > src/slave/process_based_isolation_module.cpp > 16fd584e78db2c517d828f2576ab8a38c5ce57ad > src/slave/slave.cpp 7deb4574943aae4cfc5da5d6b3f600042686975f > src/tests/utils.hpp cc1a81d07c6f23e3e2590c2df485f18d114cc6a6 > third_party/libprocess/include/stout/stringify.hpp > dcc5b95a54e6f34f93867e015d8c855fd7d6f950 > third_party/libprocess/include/stout/strings.hpp > 914c280a994733764957d19f37b48d151bb93778 > > Diff: https://reviews.apache.org/r/8108/diff/ > > > Testing > ------- > > None as of yet. > > > Thanks, > > Ben Mahler > >
