Page MenuHomeFreeBSD

Raise __MINSIGSTKSZ on amd64.
Needs ReviewPublic

Authored by markj on Jan 11 2019, 7:29 PM.

Details

Reviewers
kib
Summary

We use the larger value on all other 64-bit platforms except powerpc64.
As a side effect, PTHREAD_STACK_MIN grows as well. The current value is
too small and we run out of stack space when exiting; see PR 234775.

Add a test to act as a canary.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 21930
Build 21173: arc lint + arc unit

Event Timeline

markj created this revision.Jan 11 2019, 7:29 PM
markj added a reviewer: kib.Jan 11 2019, 7:30 PM
markj added a comment.Jan 11 2019, 7:47 PM

I should have noted that this breaks the ABI, so I was planning to bump __FreeBSD_version. Alternately, we might instead decouple PTHREAD_STACK_MIN from MINSIGSTKSZ and just hardcode it to 4096.

kib added a comment.Jan 12 2019, 1:03 AM

Indeed this changes the ABI and while I do not see why not change PTHREAD_STACK_MIN value, I would prefer to keep the library accepting the old min stack value. Otherwise we break old programs which use PTHREAD_STACK_MIN, and such programs exist even in base.

Also, it is not clear to me how changing this value impacts allocation of actual stack mapping, since mmap() would round up to the full page size anyway ? Can you show ktrace output with and without your change ?

markj added a comment.Jan 12 2019, 4:49 PM
In D18824#401882, @kib wrote:

Indeed this changes the ABI and while I do not see why not change PTHREAD_STACK_MIN value, I would prefer to keep the library accepting the old min stack value. Otherwise we break old programs which use PTHREAD_STACK_MIN, and such programs exist even in base.

Hmm, I don't see any actual uses? Indeed, there are some references in ntpd, but the code there only makes sure that its internal minimum stack size of 64KB is not smaller than PTHREAD_STACK_MIN. And, I do not see how any dynamically linked programs can successfully use a stack size of PTHREAD_STACK_MIN today.

Also, it is not clear to me how changing this value impacts allocation of actual stack mapping, since mmap() would round up to the full page size anyway ? Can you show ktrace output with and without your change ?

Without the change: https://reviews.freebsd.org/P244
With the change: https://reviews.freebsd.org/P245

Indeed, we map a full page for the stack, but the requested stack size is passed to thr_new(2) and is used to initialize %rsp.

kib added a comment.Jan 13 2019, 1:52 AM
In D18824#401882, @kib wrote:

Indeed this changes the ABI and while I do not see why not change PTHREAD_STACK_MIN value, I would prefer to keep the library accepting the old min stack value. Otherwise we break old programs which use PTHREAD_STACK_MIN, and such programs exist even in base.

Hmm, I don't see any actual uses? Indeed, there are some references in ntpd, but the code there only makes sure that its internal minimum stack size of 64KB is not smaller than PTHREAD_STACK_MIN. And, I do not see how any dynamically linked programs can successfully use a stack size of PTHREAD_STACK_MIN today.

They can, if they do not exit threads, but terminate the whole process e.g. from the main thread. And from my experience, this is very popular usage model.

I looked at the PTHREAD_STACK_MIN usage on debian code search tool. Unfortunately, it is not very useful because the search results are overloaded with hits from glibc and gcc. Most other uses are to ensure that if some user provided a stack size less that PTHREAD_STACK_MIN than PTHREAD_STACK_MIN is specified. Worrying part is that such code exists e.g. in the perl xs processor, which means that lazy users which pass 0 as a stack size suddently stop work after we require 4096 min stack size.

Also, it is not clear to me how changing this value impacts allocation of actual stack mapping, since mmap() would round up to the full page size anyway ? Can you show ktrace output with and without your change ?

Without the change: https://reviews.freebsd.org/P244
With the change: https://reviews.freebsd.org/P245
Indeed, we map a full page for the stack, but the requested stack size is passed to thr_new(2) and is used to initialize %rsp.

I see. thanks for the explanation.

markj added a comment.Jan 13 2019, 8:27 PM
In D18824#401965, @kib wrote:
In D18824#401882, @kib wrote:

Indeed this changes the ABI and while I do not see why not change PTHREAD_STACK_MIN value, I would prefer to keep the library accepting the old min stack value. Otherwise we break old programs which use PTHREAD_STACK_MIN, and such programs exist even in base.

Hmm, I don't see any actual uses? Indeed, there are some references in ntpd, but the code there only makes sure that its internal minimum stack size of 64KB is not smaller than PTHREAD_STACK_MIN. And, I do not see how any dynamically linked programs can successfully use a stack size of PTHREAD_STACK_MIN today.

They can, if they do not exit threads, but terminate the whole process e.g. from the main thread. And from my experience, this is very popular usage model.
I looked at the PTHREAD_STACK_MIN usage on debian code search tool. Unfortunately, it is not very useful because the search results are overloaded with hits from glibc and gcc. Most other uses are to ensure that if some user provided a stack size less that PTHREAD_STACK_MIN than PTHREAD_STACK_MIN is specified. Worrying part is that such code exists e.g. in the perl xs processor, which means that lazy users which pass 0 as a stack size suddently stop work after we require 4096 min stack size.

I see. I can think of a few ways to deal with this:

  1. Ignore the problem. FWIW, I encountered this problem in a valgrind regression test.
  2. Modify PTHREAD_STACK_MIN (define it to 4096 for all platforms) and define a compat version of pthread_attr_setstacksize() which accepts the old PTHREAD_STACK_MIN.
  3. Modify pthread_attr_setstacksize() to silently round up the requested stack size to a multiple of the page size. The specification for this function seems to imply that this is acceptable. This solution will not work if we run into the PTHREAD_STACK_MIN limit again in the future.
kib added a comment.Jan 14 2019, 7:14 AM
  1. Ignore the problem. FWIW, I encountered this problem in a valgrind regression test.
  2. Modify PTHREAD_STACK_MIN (define it to 4096 for all platforms) and define a compat version of pthread_attr_setstacksize() which accepts the old PTHREAD_STACK_MIN.

Isn't this is what was proposed by you before ? Except without a compat symbol, and I do not see how compat symbol is helpful.
And then it is what should be done if we change PTHREAD_STACK_MIN.

  1. Modify pthread_attr_setstacksize() to silently round up the requested stack size to a multiple of the page size. The specification for this function seems to imply that this is acceptable. This solution will not work if we run into the PTHREAD_STACK_MIN limit again in the future.
markj added a comment.Jan 14 2019, 4:41 PM
In D18824#402111, @kib wrote:
  1. Ignore the problem. FWIW, I encountered this problem in a valgrind regression test.
  2. Modify PTHREAD_STACK_MIN (define it to 4096 for all platforms) and define a compat version of pthread_attr_setstacksize() which accepts the old PTHREAD_STACK_MIN.

Isn't this is what was proposed by you before ? Except without a compat symbol, and I do not see how compat symbol is helpful.
And then it is what should be done if we change PTHREAD_STACK_MIN.

The PTHREAD_STACK_MIN limit is enforced by pthread_attr_setstacksize(); as you pointed out, applications that internally round up the stack size to PTHREAD_STACK_MIN before calling pthread_attr_setstacksize() would be broken if we blindly raised PTHREAD_STACK_MIN. A compat version of the symbol could be used to preserve the old limit for applications that are linked against the old libthr.

kib added a comment.Jan 15 2019, 4:00 AM
In D18824#402111, @kib wrote:
  1. Ignore the problem. FWIW, I encountered this problem in a valgrind regression test.
  2. Modify PTHREAD_STACK_MIN (define it to 4096 for all platforms) and define a compat version of pthread_attr_setstacksize() which accepts the old PTHREAD_STACK_MIN.

Isn't this is what was proposed by you before ? Except without a compat symbol, and I do not see how compat symbol is helpful.
And then it is what should be done if we change PTHREAD_STACK_MIN.

The PTHREAD_STACK_MIN limit is enforced by pthread_attr_setstacksize(); as you pointed out, applications that internally round up the stack size to PTHREAD_STACK_MIN before calling pthread_attr_setstacksize() would be broken if we blindly raised PTHREAD_STACK_MIN. A compat version of the symbol could be used to preserve the old limit for applications that are linked against the old libthr.

No, I mean that such compat version is not too useful because apps really cannot distinguish between old and new compilation environments. Whether the app is linked to old or new symbol is purely defined by the timing of the base system install and not due to any changes in the app code _OR_ depended headers. So if you compile the binary on old system (before the STACK_MIN was raised) and run it on new system, it would magically work. But then you take the same sources and compile them on new system and it is unexpectedly broken.