> On Nov. 20, 2012, 3:56 p.m., Vinod Kone wrote: > > src/slave/cgroups_isolation_module.cpp, line 93 > > <https://reviews.apache.org/r/8108/diff/1-2/?file=191733#file191733line93> > > > > Actually thinking a bit about it, fragmentation means sometimes you > > cannot do an allocation? > > > > Failing here seems drastic. We should probably find a way to reject the > > grow without failing.
Fragmentation simply means we're going to have to grow across many cpus. As long as we haven't used up all the cpu resources, we can continue to grow. If this check fails, then either: 1. this grow/shrink code is incorrect, or 2. mesos handed out more cpu resources than were made available to the mesos cgroup Case #2 can happen quite easily. If not specified the slave will consider all cpus as available as resources, this may not match the cgroup cpuset.cpus. If specified, the user can easily cause a mismatch as well. I've added a TODO here for this check, I'll add that today as well, but let me know what you think. > On Nov. 20, 2012, 3:56 p.m., Vinod Kone wrote: > > src/slave/cgroups_isolation_module.cpp, line 640 > > <https://reviews.apache.org/r/8108/diff/1-2/?file=191733#file191733line640> > > > > why don't you pull info->cpuset.get() out into a variable. Looks like > > its used a bunch here. > > > > some of the statements below might actually fit into one line then. I thought about that but it's used non const, so what would that look like? CPUSet& cpuset = info->cpuset; CPUSet* cpuset = &(info->cpuset); Both of which didn't feel consistent with mesos style. Actually, they fit on one line now that I went from hashmap -> map. > On Nov. 20, 2012, 3:56 p.m., Vinod Kone wrote: > > src/slave/cgroups_isolation_module.cpp, line 78 > > <https://reviews.apache.org/r/8108/diff/1-2/?file=191733#file191733line78> > > > > The order of cpus is guaranteed here because usage always contains all > > the cpus in the system? I don't know what you mean by the order, but usage is the cpu usage in the system, and it contains all cpus available to mesos. Note that it may not contain all the cpus in the _system_, since it will be restricted to those inside the cpuset.cpus for mesos. I've switched these from hashmap to std::map, so that the iteration order corresponds roughly with locality. > On Nov. 20, 2012, 3:56 p.m., Vinod Kone wrote: > > src/slave/cgroups_isolation_module.cpp, line 69 > > <https://reviews.apache.org/r/8108/diff/1-2/?file=191733#file191733line69> > > > > Can you doc the algorithm? I think the idea is to always have only 1 > > partial cpu globally? Added a description of the techniques in grow/shrink. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8108/#review13626 ----------------------------------------------------------- On Nov. 19, 2012, 11:03 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8108/ > ----------------------------------------------------------- > > (Updated Nov. 19, 2012, 11: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 > 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 > >
