----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54996/#review160513 -----------------------------------------------------------
3rdparty/stout/include/stout/os/linux.hpp (lines 57 - 60) <https://reviews.apache.org/r/54996/#comment231647> I would actually suggest keep this `create` method, but move the allocation logic in `create`. ``` static Try<Stack> create(size_t size) { Stack stack(size); if (posix_memalign(...) != 0) { return ErrnoError("Failed to allocate stack"); } return stack; } ``` We can get rid of the `allocate` function. Once created, it's by default allocated. 3rdparty/stout/include/stout/os/linux.hpp (line 58) <https://reviews.apache.org/r/54996/#comment231648> Please refer to our style guide: https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md You should move the tailing `{` to the next line. Ditto everywhere else. 3rdparty/stout/include/stout/os/linux.hpp (lines 74 - 76) <https://reviews.apache.org/r/54996/#comment231649> add a space after `if` 3rdparty/stout/include/stout/os/linux.hpp (line 75) <https://reviews.apache.org/r/54996/#comment231650> +1 to James's suggestion. 3rdparty/stout/include/stout/os/linux.hpp (lines 102 - 111) <https://reviews.apache.org/r/54996/#comment231655> I would simply the logics here: ``` if (stack.isNone()) { Try<Stack> _stack = Stack::create(...); if (_stack.isError()) { return -1; } stack = _stack.get(); cleanup = true; } char* address = stack->address + stack->size; ... if (cleanup && ...) { stack->deallocate(); } ``` 3rdparty/stout/include/stout/os/linux.hpp (line 118) <https://reviews.apache.org/r/54996/#comment231651> Let's try to use `char*` consistently. Pointer math on void pointers is disallowed in both C and C++. src/linux/ns.hpp (lines 432 - 439) <https://reviews.apache.org/r/54996/#comment231656> ``` Try<Stack> stack = os::Stack::create(...); if (stack.isError()) { return Error("Failed to allocate stack: " + stack.error()); } ... ``` - Jie Yu On Jan. 4, 2017, 12:26 a.m., Aaron Wood wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54996/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2017, 12:26 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6835 > https://issues.apache.org/jira/browse/MESOS-6835 > > > Repository: mesos > > > Description > ------- > > Currently in the Linux launcher when the stack is allocated and prepared for > a call to clone() it is not properly aligned. This is not an issue for x86 or > x64 but for ARM64/AArch64 it is because of the requirement of having the > stack aligned to a 16 byte boundary. While x86 and x64 also expect the stack > to have a 16 byte aligned stack, it is not enforced. An explanation of the > stack and requirements for ARM64 can be found here > http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf > (specifically section 5.2.2.1 that says SP mod 16 = 0. The stack must be > quad-word aligned.) > > Additionally, the way that the stack is currently allocated and passed to > clone() accidentally chops off one entry, making a stack overflow using those > missing 8 bytes a possibility. Fixing this while aligning the memory will fix > both the issue of the stack overflow issue as well as the SIGBUS crash. We > should also net better performance from having the stack aligned. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os/linux.hpp 530f1a55b > src/linux/ns.hpp 77789717e > > Diff: https://reviews.apache.org/r/54996/diff/ > > > Testing > ------- > > Built Mesos from source and am currently running it in a test cluster. > Launched both Docker and Mesos tasks via Marathon without any resulting crash > (initial crash only happened with Mesos containerizer + linux_launcher, not > with the posix_launcher). > > > Thanks, > > Aaron Wood > >
