You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2012/11/18 00:13:09 UTC

Review Request: Adding cpuset isolation to cgroups.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/
-----------------------------------------------------------

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
third_party/libprocess/include/stout/strings.hpp 914c280a994733764957d19f37b48d151bb93778

Diff: https://reviews.apache.org/r/8108/diff/

Testing
-------

None as of yet.

Thanks,

Ben Mahler

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Jie Yu <yu...@gmail.com>.


> On Nov. 19, 2012, 12:21 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 593
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line593>
> >
> >     Looks like setting cpuset.mems is also mandatory here..
> >     https://access.redhat.com/knowledge/docs/en-US/Red_Hat_Enterprise_Linux/6/html/Resource_Management_Guide/sec-cpuset.html

And seems that cpuset.mems is different from cpuset.cpus.

For example, it's possible that
cpuset.mems = 0-1
cpuset.cpus = 0-15

Probably it means that there are two chips, each chip has a memory node associated with, and each chip has 8 hardware thread (probably 4-cores with hyper threading).


- Jie


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13550
-----------------------------------------------------------


On Nov. 17, 2012, 11:13 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8108/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2012, 11:13 p.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 
>   third_party/libprocess/include/stout/strings.hpp 914c280a994733764957d19f37b48d151bb93778 
> 
> Diff: https://reviews.apache.org/r/8108/diff/
> 
> 
> Testing
> -------
> 
> None as of yet.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 19, 2012, 12:21 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 593
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line593>
> >
> >     Looks like setting cpuset.mems is also mandatory here..
> >     https://access.redhat.com/knowledge/docs/en-US/Red_Hat_Enterprise_Linux/6/html/Resource_Management_Guide/sec-cpuset.html
> 
> Jie Yu wrote:
>     And seems that cpuset.mems is different from cpuset.cpus.
>     
>     For example, it's possible that
>     cpuset.mems = 0-1
>     cpuset.cpus = 0-15
>     
>     Probably it means that there are two chips, each chip has a memory node associated with, and each chip has 8 hardware thread (probably 4-cores with hyper threading).
> 
> Jie Yu wrote:
>     No sure whether you need to configure cpuset.mems at all if you don't care about memory affinity. Probably you can just set cpuset.mems to be the value of all the available memory nodes.

Agreed, for this first pass, I think we shouldn't consider memory affinity.

We do need to be careful in that we must use a (possibly equal) subset of the hierarchy's cpuset.cpus and cpuset.mems.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13550
-----------------------------------------------------------


On Nov. 17, 2012, 11:13 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8108/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2012, 11:13 p.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 
>   third_party/libprocess/include/stout/strings.hpp 914c280a994733764957d19f37b48d151bb93778 
> 
> Diff: https://reviews.apache.org/r/8108/diff/
> 
> 
> Testing
> -------
> 
> None as of yet.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Jie Yu <yu...@gmail.com>.


> On Nov. 19, 2012, 12:21 a.m., Ben Mahler wrote:
> > src/slave/cgroups_isolation_module.cpp, line 593
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line593>
> >
> >     Looks like setting cpuset.mems is also mandatory here..
> >     https://access.redhat.com/knowledge/docs/en-US/Red_Hat_Enterprise_Linux/6/html/Resource_Management_Guide/sec-cpuset.html
> 
> Jie Yu wrote:
>     And seems that cpuset.mems is different from cpuset.cpus.
>     
>     For example, it's possible that
>     cpuset.mems = 0-1
>     cpuset.cpus = 0-15
>     
>     Probably it means that there are two chips, each chip has a memory node associated with, and each chip has 8 hardware thread (probably 4-cores with hyper threading).

No sure whether you need to configure cpuset.mems at all if you don't care about memory affinity. Probably you can just set cpuset.mems to be the value of all the available memory nodes.


- Jie


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13550
-----------------------------------------------------------


On Nov. 17, 2012, 11:13 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8108/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2012, 11:13 p.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 
>   third_party/libprocess/include/stout/strings.hpp 914c280a994733764957d19f37b48d151bb93778 
> 
> Diff: https://reviews.apache.org/r/8108/diff/
> 
> 
> Testing
> -------
> 
> None as of yet.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13550
-----------------------------------------------------------



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29113>

    Looks like setting cpuset.mems is also mandatory here..
    https://access.redhat.com/knowledge/docs/en-US/Red_Hat_Enterprise_Linux/6/html/Resource_Management_Guide/sec-cpuset.html


- Ben Mahler


On Nov. 17, 2012, 11:13 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8108/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2012, 11:13 p.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 
>   third_party/libprocess/include/stout/strings.hpp 914c280a994733764957d19f37b48d151bb93778 
> 
> Diff: https://reviews.apache.org/r/8108/diff/
> 
> 
> Testing
> -------
> 
> None as of yet.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.hpp, line 49
> > <https://reviews.apache.org/r/8108/diff/5/?file=222258#file222258line49>
> >
> >     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.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 656
> > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line656>
> >
> >     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.

Nice catch.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 112
> > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line112>
> >
> >     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) {
> >     
> >     }

Done, ironically, I didn't do it that way because I seemed to remember a review in which you didn't like me mutating a parameter like that ;)


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.hpp, line 151
> > <https://reviews.apache.org/r/8108/diff/5/?file=222258#file222258line151>
> >
> >     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.

Nice catch.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 347
> > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line347>
> >
> >     The use of this variable is quite far away from the declaration/definition, FYI.

Moved.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, line 351
> > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line351>
> >
> >     Yes, this really ought to be factored out. I'll take a look for you.

Thanks.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.cpp, lines 396-398
> > <https://reviews.apache.org/r/8108/diff/5/?file=222259#file222259line396>
> >
> >     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.

You're not reading this correctly ;)
This is an essential part of the logic, in that, we don't want to use more cpus than the slave resources specified. That's why this loop stops adding cpus when it surpasses the amount specified in the slave --resources.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13678
-----------------------------------------------------------


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 21, 2012, 11:37 p.m., Benjamin Hindman wrote:
> > src/slave/cgroups_isolation_module.hpp, line 151
> > <https://reviews.apache.org/r/8108/diff/5/?file=222258#file222258line151>
> >
> >     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.
> 
> Ben Mahler wrote:
>     Nice catch.

I'll also move it into the cpp in the next diff.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13678
-----------------------------------------------------------


On Nov. 22, 2012, 1:46 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, 1:46 a.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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 26, 2012, 6:33 p.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

Updated based on reviews from jie and benh.

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 25, 2012, 9:39 p.m., Benjamin Hindman wrote:
> > src/linux/proc.hpp, line 94
> > <https://reviews.apache.org/r/8108/diff/7/?file=222807#file222807line94>
> >
> >     Can this be 'return out << ...;'?

done


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13715
-----------------------------------------------------------


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Benjamin Hindman <be...@berkeley.edu>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13715
-----------------------------------------------------------

Ship it!



src/linux/proc.hpp
<https://reviews.apache.org/r/8108/#comment29397>

    s/other/that/g
    
    Most of our code typically uses 'that' instead of 'other' (the juxtaposition of "this" and "that").



src/linux/proc.hpp
<https://reviews.apache.org/r/8108/#comment29398>

    Can this be 'return out << ...;'?



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29399>

    Love this idea!



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29400>

    Missed a CPUSet here and a few other spots too.



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29401>

    s/cpu/'cpu'/ and s/cpuset/'cpuset'/



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29395>

    I'd swap these. The 'resource.name() == "cpus"' pairs with the if check below, and that "stanza" of code is shared in a few other places.


- Benjamin Hindman


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> 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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Jie Yu <yu...@gmail.com>.

-----------------------------------------------------------
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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Jie Yu <yu...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13716
-----------------------------------------------------------

Ship it!


Ship It!

- 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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

-----------------------------------------------------------
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.


Changes
-------

My review posting tool keeps omitting jie, sorry about that!


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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 22, 2012, 3:25 a.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

Added approximate double comparisons.

Good read on the subject: http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm

I'm uncomfortable having this in the long run, but for now I'm ok with it if we enforce that no one allocates anything with more precision than 0.001.

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 22, 2012, 1:46 a.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

Review from ben, will be posting another update to this with approximate double comparisons to avoid floating point errors.

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 20, 2012, 9:03 p.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

This now caps the number of cpus the CgroupIsolationModule pulls from /proc/cpuinfo to match the slave resources.

E.g. when --resources=cpus:4 and the machine has 16 cores, we'll only use 4 cores during cgroup cpu allocation.

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 20, 2012, 8:48 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 400
> > <https://reviews.apache.org/r/8108/diff/3-4/?file=222242#file222242line400>
> >
> >     They can specify, but it can  be atmost cpuset. Probably need to rephrase the error message?

Before: You cannot specify 17 cpus because it is more than allowed by the cgroup cpuset.cpus: 0-15
After: You have specified 17 cpus, but this is more than allowed by the cgroup cpuset.cpus: 0-15


> On Nov. 20, 2012, 8:48 p.m., Vinod Kone wrote:
> > src/slave/lxc_isolation_module.hpp, line 44
> > <https://reviews.apache.org/r/8108/diff/4/?file=222249#file222249line44>
> >
> >     too bad your lxc yak shave hasnt been submitted yet.

Hahah, yep. Dropped since I'm guessing you didn't mean to create an issue for this comment.


> On Nov. 20, 2012, 8:48 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 403
> > <https://reviews.apache.org/r/8108/diff/3-4/?file=222242#file222242line403>
> >
> >     As discussed over chat, we need to figure out a clean way to set the right defaults when slave resources are not specified on the command line. Otherwise, we might always fail here (for eg: the cpuset in which the slave was started has less than the total no.of cpus in the system)!

Yeah, as discussed you'll be sending out a review for this right? Thanks!


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13647
-----------------------------------------------------------


On Nov. 20, 2012, 8:15 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, 8:15 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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Vinod Kone <vi...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13647
-----------------------------------------------------------



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29250>

    They can specify, but it can  be atmost cpuset. Probably need to rephrase the error message?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29251>

    As discussed over chat, we need to figure out a clean way to set the right defaults when slave resources are not specified on the command line. Otherwise, we might always fail here (for eg: the cpuset in which the slave was started has less than the total no.of cpus in the system)!



src/slave/lxc_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29249>

    too bad your lxc yak shave hasnt been submitted yet.



src/slave/process_based_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29248>

    I think BenH mentioned that the convention for naming unused parameters is to call them "_" ?



src/slave/slave.cpp
<https://reviews.apache.org/r/8108/#comment29247>

    not yours, but each argument on a different line.



src/tests/utils.hpp
<https://reviews.apache.org/r/8108/#comment29246>

    s/</"
    s/>/"


- Vinod Kone


On Nov. 20, 2012, 8:15 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, 8:15 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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 20, 2012, 8:15 p.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

Added a check during CgroupIsolationModule initialization. It ensures we're not considering more cpus available than specified by cpuset.cpus.

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 20, 2012, 8:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 66
> > <https://reviews.apache.org/r/8108/diff/2-3/?file=221930#file221930line66>
> >
> >     How about "Returns the total cpu usage across all the cpus in this cpu set" ?

done


> On Nov. 20, 2012, 8:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 99
> > <https://reviews.apache.org/r/8108/diff/2-3/?file=221931#file221931line99>
> >
> >     s/slave doesn't not consider more cpus available/slave doesn't offer more cpus/ ?

killed this TODO entirely in the new diff (since I added the check)


> On Nov. 20, 2012, 8:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 114
> > <https://reviews.apache.org/r/8108/diff/2-3/?file=221931#file221931line114>
> >
> >     avoid?
> >     
> >     I think this algorithm (shrink()) doesn't impact fragmentation at all, because of the way you do grow()?

I'd argue grow() brings about the fragmentation, and shrink does a attempt to reduce it.

Suppose shrink were to pull off from the first cpu we have allocated (akin to grow()), then we'd definitely fragment more!
It's currently trying to reduce cpus.size() as much as possible.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13642
-----------------------------------------------------------


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Vinod Kone <vi...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13642
-----------------------------------------------------------

Ship it!



src/linux/proc.hpp
<https://reviews.apache.org/r/8108/#comment29232>

    Nice!



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29233>

    How about "Returns the total cpu usage across all the cpus in this cpu set" ?



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29234>

    great



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29235>

    s/slave doesn't not consider more cpus available/slave doesn't offer more cpus/ ?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29236>

    avoid?
    
    I think this algorithm (shrink()) doesn't impact fragmentation at all, because of the way you do grow()?


- Vinod Kone


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 20, 2012, 7:32 p.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

Changes based on Vinod's review.

Also realized that the slave needs to ensure that it respects the number of cpus in the mesos cgroup cpuset.cpus.
Otherwise we'll have issues allocating cpu resources. Added a TODO for this and will submit another iteration today to get that in.

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> 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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> 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.
> 
> Vinod Kone wrote:
>     we typically just store it in a variable (CPUSet cpuset = info->cpuset), irrespective of constness. But, I will leave it upto you.

That would create a copy! I need to mutate the one inside info.


- Ben


-----------------------------------------------------------
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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Vinod Kone <vi...@gmail.com>.


> 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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Vinod Kone <vi...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13626
-----------------------------------------------------------



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29212>

    doc please



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29208>

    Can you doc the algorithm? I think the idea is to always have only 1 partial cpu globally?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29210>

    The order of cpus is guaranteed here because usage always contains all the cpus in the system?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29211>

    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.



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29207>

    'delta' in a log line probably doesn't make sense to users. how about just say 'by' ?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29206>

    ditto



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29213>

    nice



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29214>

    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.


- Vinod Kone


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.

(Updated Nov. 19, 2012, 11:03 p.m.)

Review request for mesos, Benjamin Hindman and Vinod Kone.

Changes
-------

-Only allow one of the {cpu, cpuset} subsystems to be enabled.
-Read the cpuset.cpus from the mesos cgroup in order to ensure we don't allocate to cpus outside our cpuset.
-Addressed comments from vinod.

I do not write to cpuset.mems, since we're not going to consider memory affinity. Note that any cgroups created will have cpuset.mems copied from the parent (see cgroups::cloneCpusetCpusMems).

Description
-------

This is the first pass at adding cpuset isolation for pinning cgroups to 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 (updated)
-----

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

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 62
> > <https://reviews.apache.org/r/8108/diff/1/?file=191732#file191732line62>
> >
> >     you call this 'usage' when used as a parameter to grow() , but call it 'cpus' here.
> >     
> >     how about calling to 'global' and 'local' respectively?
> 
> Ben Mahler wrote:
>     I guess this wasn't as obvious as I anticipated. I've added jie-style docs to CPUSet to make this more clear.
>     
>     I'm going to drop this with the following justification:
>     
>     CPUSet holds cpus, these are local to the CPUSet. There can be many CPUSets (each CgroupInfo would contain one).
>     CgroupIsolationModule holds cpus, these are all the cpus.
>     
>     In the context you mentioned, the CgroupIsolationModule.cpus is passed into grow(), so that the CPUSet can know about the cpu "usage".
>     
>     So, I think they both make sense to be named as cpus (since the context differentiates them). I also liked usage because the Cgroup needs to know about the cpu "usage" when performing new allocations.
>     
>     Please re-open if you disagree, and let me know if you have more naming suggestions ;)
>     
>
> 
> Vinod Kone wrote:
>     My only problem is that you are using 3 different terms to refer to the same thing "cpus", "usage" and "allocation" in your code/doc. It kinda gets confusing, without looking at the docs.

I'd argue they are not the same thing though:
  cpus: the cpus belonging to this CPUSet
  allocation: the amount we've newly allocated during a grow()
  usage: the cpu usage

If it's just me and these are confusing for people I'd be happy to rename, but I'd like short names that are clear based on the context (which is what I thought I had set up here ;))


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13570
-----------------------------------------------------------


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Vinod Kone <vi...@gmail.com>.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 62
> > <https://reviews.apache.org/r/8108/diff/1/?file=191732#file191732line62>
> >
> >     you call this 'usage' when used as a parameter to grow() , but call it 'cpus' here.
> >     
> >     how about calling to 'global' and 'local' respectively?
> 
> Ben Mahler wrote:
>     I guess this wasn't as obvious as I anticipated. I've added jie-style docs to CPUSet to make this more clear.
>     
>     I'm going to drop this with the following justification:
>     
>     CPUSet holds cpus, these are local to the CPUSet. There can be many CPUSets (each CgroupInfo would contain one).
>     CgroupIsolationModule holds cpus, these are all the cpus.
>     
>     In the context you mentioned, the CgroupIsolationModule.cpus is passed into grow(), so that the CPUSet can know about the cpu "usage".
>     
>     So, I think they both make sense to be named as cpus (since the context differentiates them). I also liked usage because the Cgroup needs to know about the cpu "usage" when performing new allocations.
>     
>     Please re-open if you disagree, and let me know if you have more naming suggestions ;)
>     
>

My only problem is that you are using 3 different terms to refer to the same thing "cpus", "usage" and "allocation" in your code/doc. It kinda gets confusing, without looking at the docs.


- Vinod


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13570
-----------------------------------------------------------


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Ben Mahler <be...@gmail.com>.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 57
> > <https://reviews.apache.org/r/8108/diff/1/?file=191732#file191732line57>
> >
> >     what does totalUsage mean? total usage across all cpusets or just this cpuset? if the latter, just call it usage.

the latter


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 147
> > <https://reviews.apache.org/r/8108/diff/1/?file=191732#file191732line147>
> >
> >     not yours, but could you s/successes/succeeds/?


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 92
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line92>
> >
> >     I think the assumption here is that you can always find a feasible allocation? 
> >     If so, you should do a CHECK(needed <= 0.0).

Done, and added a dump of the state for easier debugging should this occur.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 140
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line140>
> >
> >     instead of doing this logic here, you should delete a cpu from cpus if usage falls to 0 during shrink phase.

Agreed. I like this because it better keeps the abstraction of a CPU"Set" where we only hold the CPUs we've allocated to.

I also introduced a CHECK here to enforce the new implicit invariant that all values in the map are > 0.0.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 105
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line105>
> >
> >     how about calling it least instead?

Also simplified this logic by using an Option instead of the map iterator.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 586
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line586>
> >
> >     why fabs here?


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 62
> > <https://reviews.apache.org/r/8108/diff/1/?file=191732#file191732line62>
> >
> >     you call this 'usage' when used as a parameter to grow() , but call it 'cpus' here.
> >     
> >     how about calling to 'global' and 'local' respectively?

I guess this wasn't as obvious as I anticipated. I've added jie-style docs to CPUSet to make this more clear.

I'm going to drop this with the following justification:

CPUSet holds cpus, these are local to the CPUSet. There can be many CPUSets (each CgroupInfo would contain one).
CgroupIsolationModule holds cpus, these are all the cpus.

In the context you mentioned, the CgroupIsolationModule.cpus is passed into grow(), so that the CPUSet can know about the cpu "usage".

So, I think they both make sense to be named as cpus (since the context differentiates them). I also liked usage because the Cgroup needs to know about the cpu "usage" when performing new allocations.

Please re-open if you disagree, and let me know if you have more naming suggestions ;)


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.hpp, line 248
> > <https://reviews.apache.org/r/8108/diff/1/?file=191732#file191732line248>
> >
> >     s/cpus/global?

See above, and let me know.


> On Nov. 19, 2012, 5:05 p.m., Vinod Kone wrote:
> > src/slave/cgroups_isolation_module.cpp, line 70
> > <https://reviews.apache.org/r/8108/diff/1/?file=191733#file191733line70>
> >
> >     how about creating its own source file for CPUSet. i think its complicated enough to not compound it with the isolation module.

Added a TODO for this. I'd like to move cgroups stuff into it's own folder in a separate CL.
For this review, I'll leave it as a TODO to avoid polluting the diff.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13570
-----------------------------------------------------------


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
> 
>

Re: Review Request: Adding cpuset isolation to cgroups.

Posted by Vinod Kone <vi...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8108/#review13570
-----------------------------------------------------------



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29122>

    s/allocated/newly allocated/?



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29123>

    what does totalUsage mean? total usage across all cpusets or just this cpuset? if the latter, just call it usage.



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29128>

    you call this 'usage' when used as a parameter to grow() , but call it 'cpus' here.
    
    how about calling to 'global' and 'local' respectively?



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29124>

    Make this optional?
    Option<CpuSet> cpuset?



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29125>

    not yours, but could you s/successes/succeeds/?



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29126>

    ditto



src/slave/cgroups_isolation_module.hpp
<https://reviews.apache.org/r/8108/#comment29129>

    s/cpus/global?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29134>

    how about creating its own source file for CPUSet. i think its complicated enough to not compound it with the isolation module.



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29130>

    I think the assumption here is that you can always find a feasible allocation? 
    If so, you should do a CHECK(needed <= 0.0). 



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29131>

    how about calling it least instead?



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29132>

    instead of doing this logic here, you should delete a cpu from cpus if usage falls to 0 during shrink phase.



src/slave/cgroups_isolation_module.cpp
<https://reviews.apache.org/r/8108/#comment29133>

    why fabs here?


- Vinod Kone


On Nov. 17, 2012, 11:13 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8108/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2012, 11:13 p.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 
>   third_party/libprocess/include/stout/strings.hpp 914c280a994733764957d19f37b48d151bb93778 
> 
> Diff: https://reviews.apache.org/r/8108/diff/
> 
> 
> Testing
> -------
> 
> None as of yet.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>