> 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. > > Ben Mahler wrote: > 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.
we typically just store it in a variable (CPUSet cpuset = info->cpuset), irrespective of constness. But, I will leave it upto you. > 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. > > Ben Mahler wrote: > 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. Right, I forgot. I was convinced the first time I read your 1st diff, but somewhere down the line I forgot how the algorithm works. So, ignore my comment. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8108/#review13626 ----------------------------------------------------------- On Nov. 20, 2012, 7:32 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, 7:32 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 > >
