Test and fix for issue 333. #334
Conversation
|
Hi @heiner! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Looks like your flake8 and various tests are broken anyway. Proper FB engineering, here. |
Rude. |
:( |
640K ought to be enough for anybody.
| @@ -19,7 +19,7 @@ | |||
| #include <bzlib.h> | |||
| #endif | |||
|
|
|||
| #define STACK_SIZE (1 << 15) /* 32KiB */ | |||
| #define STACK_SIZE (1 << 16) /* 64KiB */ | |||
There was a problem hiding this comment.
Perhaps a more fancy solution would be to do what pthreads do -- define a really large upper limit (8MiB or so) but use mmap to allocate the buffer and rely on page faults to grow actual memory usage. Perhaps @tscmoo can look into that :D
Edit: The above comment doesn't make much sense. Even just mallocing the buffer will have the same effect (pages are only "paged in" when accessed). The number above could just be set to a larger value.
There was a problem hiding this comment.
I guess what could be done is to add a guard page at the end, a la https://man7.org/linux/man-pages/man3/pthread_attr_setguardsize.3.html
There was a problem hiding this comment.
The above was in fact not that wrong after all. All ways of getting buffers short of mmap may/do initialize the memory and therefore page everything in.
But it turns out fcontext does exactly the right thing anyway: Itmprotects the bottom page of the new stack. I don't quite understand why this is a SIGBUS then, not a SIGSEGV.
However, I did learn: The page size of M1 MacBooks is 16384 and our 32KiB value was two pages, one of them protected and unavailable. Also, create_fcontext_stack actually picks a sensible default of called with 0, which we should just do.
There was a problem hiding this comment.
Except that that default is currently broken due to a recent glibc change, reported at septag/deboost.context#4.
|
Alles gut! |
* Test for issue 333. * Make fcontext stack larger. 640K ought to be enough for anybody.
Fixes #333.