Re: Review Request 34442: Support manipulating scheduler affinity on Linux.

[Joris Van Remoortere]

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

(Updated May 20, 2015, 5:16 a.m.)


Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone.


Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652


Repository: mesos


Description
---

This will be used to write a pre-emption test for the SCHED_OTHER vs SCHED_IDLE 
CFS queues.


Diffs
-

  src/linux/sched.hpp PRE-CREATION 
  src/tests/sched_tests.cpp PRE-CREATION 

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


Testing
---

Added a test that bounces around on different cores.


Thanks,

Joris Van Remoortere

Re: Review Request 34442: Support manipulating scheduler affinity on Linux.

[Benjamin Hindman]

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



src/linux/sched.hpp


I don't understand what this returns? How does a set of uint16_t represent 
the cores? Are these the core "numbers"? Why are the numbers uint16_t instead 
of just int or size_t? Some documentation here would be great.



src/linux/sched.hpp


s/ &/& /

Did you do a pass on your review? ;-) Please fix this here and everywhere 
else please (there are a handful of other places).

Also, seems like you can s/_cores/cores/ too since the member variable has 
a trailing suffix _ (and this is a static function anyway).



src/linux/sched.hpp


Based on how I'm reading this review, shouldn't this be s/_cores/mask/?



src/linux/sched.hpp


s/_cores/cores/ since you're using the trailing suffix _ as `cores_` for 
the member name.



src/linux/sched.hpp


Please put '{' on newline.



src/linux/sched.hpp


What about creating an 'affinity' namespace just like Ian did for 'policy'? 
Then we'd have `sched::policy::set(...)` and `sched::affinity::set(...)`.



src/linux/sched.hpp


The variable name 'cores' is used for a lot of different things, let's be 
consistent here. If I understand this should be s/cores/mask/ and the CPUSet 
variable below should be 'set'.



src/linux/sched.hpp


pid.get(0)

And then maybe it all fits on oneline too!?



src/linux/sched.hpp


s/cores/set/



src/linux/sched.hpp


pid.get(0)

And then maybe it all fits on oneline too!?



src/tests/sched_tests.cpp


One thing to consider here, I don't know how 
std::thread::hardware_concurrency is implemented but it's possible that this 
test itself will be run in a cpuset and thus we might have sufficient hardware 
concurrency but this process can't actually run on more than one core. If 
that's the case then this will yield a flaky test which will be a pain to 
debug. It seems like the better test here is to check that we have affinity on 
more than one core, then use those cores to further set reduced affinity, and 
then return affinity to the original.



src/tests/sched_tests.cpp


It is probably cleaner to just `return` here instead.



src/tests/sched_tests.cpp


Do you need the `set`? Here and below.



src/tests/sched_tests.cpp


You don't need the `.get()` here, that's why you're using the 
`EXPECT_SOME_EQ`. ;-)

Please clean up the rest in this test too please.



src/tests/sched_tests.cpp


I'm not sure what benefit this part of the test provides. First, it's 
possible that we don't actually run on multiple cores for whatever reason, in 
which case we have a flaky test which is a pain to debug. Second, what does 
this test? That the implementation of sched_setaffinity and sched_setaffinity 
correctly works? That's not our code so we don't need to bother testing it.



src/tests/sched_tests.cpp


When do we get `synchronized`!?


- Benjamin Hindman


On May 20, 2015, 2:55 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34442/
> ---
> 
> (Updated May 20, 2015, 2:55 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used to write a pre-emption test for the SCHED_OTHER vs 
> SCHED_IDLE CFS queues.
> 
> 
> Diffs
> -
> 
>   src/linux/sched.hpp PRE-CREATION 
>   src/tests/sched_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34442/diff/
> 
> 
> Testing
> ---
> 
> Added a test that bounces around on different cores.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 34442: Support manipulating scheduler affinity on Linux.

[Joris Van Remoortere]

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

(Updated May 20, 2015, 2:55 a.m.)


Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone.


Changes
---

Fix comment.
Use capture by value default for lambda.


Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652


Repository: mesos


Description
---

This will be used to write a pre-emption test for the SCHED_OTHER vs SCHED_IDLE 
CFS queues.


Diffs (updated)
-

  src/linux/sched.hpp PRE-CREATION 
  src/tests/sched_tests.cpp PRE-CREATION 

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


Testing
---

Added a test that bounces around on different cores.


Thanks,

Joris Van Remoortere

Re: Review Request 34442: Support manipulating scheduler affinity on Linux.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [34308, 34309, 34442]

Failed command: ./support/apply-review.sh -n -r 34442

Error:
 2015-05-19 23:34:57 URL:https://reviews.apache.org/r/34442/diff/raw/ 
[6471/6471] -> "34442.patch" [1]
error: patch failed: src/linux/sched.hpp:19
error: src/linux/sched.hpp: patch does not apply
error: patch failed: src/tests/sched_tests.cpp:18
error: src/tests/sched_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On May 19, 2015, 11:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34442/
> ---
> 
> (Updated May 19, 2015, 11:32 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2652
> https://issues.apache.org/jira/browse/MESOS-2652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used to write a pre-emption test for the SCHED_OTHER vs 
> SCHED_IDLE CFS queues.
> 
> 
> Diffs
> -
> 
>   src/linux/sched.hpp PRE-CREATION 
>   src/tests/sched_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34442/diff/
> 
> 
> Testing
> ---
> 
> Added a test that bounces around on different cores.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 34442: Support manipulating scheduler affinity on Linux.

[Joris Van Remoortere]

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

Review request for mesos, Ian Downes, Niklas Nielsen, and Vinod Kone.


Bugs: MESOS-2652
https://issues.apache.org/jira/browse/MESOS-2652


Repository: mesos


Description
---

This will be used to write a pre-emption test for the SCHED_OTHER vs SCHED_IDLE 
CFS queues.


Diffs
-

  src/linux/sched.hpp PRE-CREATION 
  src/tests/sched_tests.cpp PRE-CREATION 

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


Testing
---

Added a test that bounces around on different cores.


Thanks,

Joris Van Remoortere