----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8108/#review13711 -----------------------------------------------------------
src/slave/cgroups_isolation_module.hpp <https://reviews.apache.org/r/8108/#comment29391> Copy-paste error? Remove it. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29392> Also, we can consider providing some floating point comparison utility functions in stout. For example, something like this in gtest: http://code.google.com/p/googletest/wiki/AdvancedGuide#Floating-Point_Comparison src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29393> This is a little weird. Can we save "_resources" the same way as we save "_flags" and "_slave" in the beginning of this function and remove the leading underscore? src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29394> When I read here, I am wondering whether we can make "this->cpus" a Cpuset as well? In that way, we can hide some Cpuset manipulation logic inside the isolation module. See my comments below. src/slave/cgroups_isolation_module.cpp <https://reviews.apache.org/r/8108/#comment29396> For example, if "this->cpus" is a Cpuset, what you can do here is: Cpuset deallocated = info->cpuset->shrink(fabs(delta); cpus->add(deallocated); Cpuset allocated = info->cpuset->grow(delta, cpus); cpus->subtract(allocated); But I guess what's tricky here is that "this->cpus" embeds more information (i.e. the available cpus) so that you cannot delete a map entry when shrinking. One solution might be defining the "grow" interface to be: Cpuset grow(double delta, Cpuset& currentUsage, set<proc::CPU>& availableCpus); And you can save the "availableCpus" during initialization. Another solution might be not deleting map entries during shrinking, and always clone the root Cpuset when creating a new Cpuset. This is just a suggestion. Your code is totally fine to me:) - Jie Yu On Nov. 22, 2012, 4:27 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8108/ > ----------------------------------------------------------- > > (Updated Nov. 22, 2012, 4:27 a.m.) > > > Review request for mesos, Benjamin Hindman, Vinod Kone, and Jie Yu. > > > 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 > >
