> On Nov. 25, 2012, 9:24 p.m., Jie Yu wrote: > > src/slave/cgroups_isolation_module.hpp, line 62 > > <https://reviews.apache.org/r/8108/diff/7/?file=222808#file222808line62> > > > > Copy-paste error? Remove it.
done > On Nov. 25, 2012, 9:24 p.m., Jie Yu wrote: > > src/slave/cgroups_isolation_module.cpp, line 76 > > <https://reviews.apache.org/r/8108/diff/7/?file=222809#file222809line76> > > > > 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 Indeed I based this function off of ASSERT_NEAR from gtest ;) I suppose we could provide such utilities in stout, but I'd rather we eventually convert this code here to use an integral type, to avoid having to even consider relative errors. Hence my TODO on this function. > On Nov. 25, 2012, 9:24 p.m., Jie Yu wrote: > > src/slave/cgroups_isolation_module.cpp, line 389 > > <https://reviews.apache.org/r/8108/diff/7/?file=222809#file222809line389> > > > > 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? The existing functions: launchExecutor, killExecutor, resourcesChanged all take a different 'resources', so it will be a little dirty for me to add a resources member variable, unless I just call it slaveResources. Either way, we don't use it outside of initialize so it seems weird to store it, no? > On Nov. 25, 2012, 9:24 p.m., Jie Yu wrote: > > src/slave/cgroups_isolation_module.cpp, line 665 > > <https://reviews.apache.org/r/8108/diff/7/?file=222809#file222809line665> > > > > 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:) Hm.. I do like this suggestion, it would be nice to have Cpuset contain this logic. I could go either way, but I think that I will leave it as is for now. The way it is now is perhaps easier to reason about for others: no need to think "what is the difference between 'grow' and 'add', or 'shrink' and 'subtract'?". I'd also like to keep the Cpuset abstraction simple to make it easy to change to integer allocations later, which would clean up a lot of this code anyway ;) - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8108/#review13711 ----------------------------------------------------------- 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 > >
