----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8108/#review13678 -----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp <https://reviews.apache.org/r/8108/#comment29314> 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. src/slave/cgroups_isolation_module.hpp <https://reviews.apache.org/r/8108/#comment29313> 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. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29317> I think it would be nice to pull out the "allocated" constant to be extra explicit (note I'm using delta here instead of needed, see my comment below). double allocated = std::min(delta, free); allocation[cpu] = allocated; cpus[cpu] += allocated; delta -= allocated; src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29316> 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) { } src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29318> Like above, I think it would be nice to use an explicit "deallocated" variable: double used = cpus[least.get()]; double deallocated = std::min(used, delta); deallocation[least.get()] = deallocated; cpus[least.get()] -= deallocated; delta -= deallocated; src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29321> The use of this variable is quite far away from the declaration/definition, FYI. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29320> Yes, this really ought to be factored out. I'll take a look for you. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29306> s/write/cpuset/ src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29307> We've typically used string::npos. Also any reason not to use strings::contains? src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29308> 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. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29310> 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. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29311> You're getting a copy of cpuset here (the copy being returned via Option::get) and so you aren't actually updating the cpuset state. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29312> Ditto above. - Benjamin Hindman 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 > >
