----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34442/#review84453 -----------------------------------------------------------
src/linux/sched.hpp <https://reviews.apache.org/r/34442/#comment135723> 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 <https://reviews.apache.org/r/34442/#comment135724> 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 <https://reviews.apache.org/r/34442/#comment135727> Based on how I'm reading this review, shouldn't this be s/_cores/mask/? src/linux/sched.hpp <https://reviews.apache.org/r/34442/#comment135728> s/_cores/cores/ since you're using the trailing suffix _ as `cores_` for the member name. src/linux/sched.hpp <https://reviews.apache.org/r/34442/#comment135744> Please put '{' on newline. src/linux/sched.hpp <https://reviews.apache.org/r/34442/#comment135731> 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 <https://reviews.apache.org/r/34442/#comment135732> 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 <https://reviews.apache.org/r/34442/#comment135743> pid.get(0) And then maybe it all fits on oneline too!? src/linux/sched.hpp <https://reviews.apache.org/r/34442/#comment135733> s/cores/set/ src/linux/sched.hpp <https://reviews.apache.org/r/34442/#comment135742> pid.get(0) And then maybe it all fits on oneline too!? src/tests/sched_tests.cpp <https://reviews.apache.org/r/34442/#comment135741> 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 <https://reviews.apache.org/r/34442/#comment135735> It is probably cleaner to just `return` here instead. src/tests/sched_tests.cpp <https://reviews.apache.org/r/34442/#comment135736> Do you need the `set<uint16_t>`? Here and below. src/tests/sched_tests.cpp <https://reviews.apache.org/r/34442/#comment135737> 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 <https://reviews.apache.org/r/34442/#comment135740> 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 <https://reviews.apache.org/r/34442/#comment135738> 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 > >
