RFR(L) 8140520 segfault on solaris-amd64 with "-XX:VMThreadStackSize=1" option

[Daniel D. Daugherty]

Thanks Coleen!

Dan


On 9/2/16 3:51 PM, Coleen Phillimore wrote:
>
> Looks good.  I'm glad to see all this work that you guys have done 
> nearing completion.
> thanks,
> Coleen
>
> On 9/2/16 2:28 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I am shepherding the fix for the following bug for Ron Durbin:
>>
>>     JDK-8140520 segfault on solaris-amd64 with 
>> "-XX:VMThreadStackSize=1" option
>>     https://bugs.openjdk.java.net/browse/JDK-8140520
>>
>> Almost all of the product code is Ron's, all of the test code is mine 
>> and
>> all of the product code refactoring is Coleen's. I will be handling the
>> responses to all code review comments and suggestions. Thanks to 
>> Jerry for
>> reviewing earlier versions of Ron's work on this bug. Thanks to David H.
>> and Coleen for reviewing versions after I started shepherding.
>>
>> The primary focus of this change is to decouple the various thread stack
>> sizes from each other. In particular, decouple CompilerThreadStackSize
>> from VMThreadStackSize to avoid use of VMThreadStackSize defaults for
>> CompilerThreadStackSize.
>>
>> There are almost no Win* changes here because the Win* thread stack size
>> implementation differs significantly from the *NIX implementation. I 
>> have
>> a first pass implementation for the Win* changes, but Win* stack pages
>> don't behave like I expect so I will split Win* off into another bug.
>>
>> The AIX and PPC parts of the fix are "best guesses" because we don't 
>> have
>> a way to build and test AIX and/or PPC in Oracle. I'll need help from 
>> the
>> usual AIX and PPC folks for checking out those changes.
>>
>> Overview of the changes:
>>
>> - The CompilerThreadStackSize default has been changed from zero to
>>   match the default VMThreadStackSize on each platform; see the various
>>   globals_*.hpp files.
>>
>> - There are now three minimum stack sizes: java_thread_min_stack_allowed
>>   compiler_thread_min_stack_allowed, and 
>> vm_internal_thread_min_stack_allowed.
>>   In all cases except for Solaris X64, the two new minimums are 
>> initialized
>>   to same value as the original min_stack_allowed; see the various
>>   */os_cpu/*/os_*.cpp files.
>>
>>   Key change: On Solaris X64 the new compiler_thread_min_stack_allowed
>>   value is 394K instead of the old min_stack_allowed value of 224K.
>>
>> - os::create_thread()'s stack size parameter has been clarified to be
>>   the "requested stack size". The stack size adjustment logic has been
>>   updated to support the three minimum stack sizes.
>>
>>   Key change: the VMThreadStackSize default is no longer used as the
>>   CompilerThreadStackSize default.
>>
>> - os::init_2()'s minimum stack size setting and checking logic has
>>   been updated to support the three minimum stack sizes.
>>
>>   Key change: CompilerThreadStackSize and VMThreadStackSize are no 
>> longer
>>   silently rounded up when their specified value is too small. This 
>> matches
>>   the behavior for both '-Xss<n_bytes>' and 
>> '-XX:ThreadStackSize=<k_bytes>'.
>>
>>   Note: The recent fix for the following bug:
>>
>>     8159335: Fix problems with stack overflow handling.
>>
>>   updated the existing minimum stack size calculations to ensure that 
>> they
>>   are aligned with os::vm_page_size(). I've updated Ron's solution for
>>   this bug to include the new alignment work with the two new minimum
>>   stack sizes.
>>
>> Bugs fixed:
>>
>> - The Solaris X64 crash when "-XX:VMThreadStackSize=1" is specified
>>   without specifying "-XX:CompilerThreadStackSize=<k_bytes>" is fixed.
>> - Newly defined ThreadType values are explicitly handled as VM internal
>>   threads instead of skating through the case statements.
>> - On platforms that use default_stack_size(), the bug where a default
>>   value returned for a compiler_thread was ignored is fixed.
>>
>> Changes in behavior:
>>
>> - Use of -XX:VMThreadStackSize=<k_foo> will no longer affect the value
>>   of CompilerThreadStackSize when -XX:CompilerThreadStackSize=<bar>
>>   is not specified.
>> - When neither -XX:VMThreadStackSize=<foo> or 
>> -XX:CompilerThreadStackSize=<bar>
>>   are specified on the command line, then CompilerThreadStackSize 
>> will use
>>   a distinct default value than VMThreadStackSize.
>> - Use of a too small -XX:CompilerThreadStackSize=<bar> value or
>>   a -XX:VMThreadStackSize=<foo> will generate an error message
>>   instead of silently rounding up.
>>
>> Refactoring changes:
>>
>> - os::Posix::get_initial_stack_size() and 
>> os::Posix::set_minimum_stack_sizes()
>>   created as a place to land common *NIX code; once we figure out how 
>> to add
>>   support for the three thread stack sizes to Win*, this code will 
>> move again.
>> - default_stack_size() added to all platforms.
>> - default_guard_size() removed from platforms that were not using it; 
>> only
>>   Linux uses it.
>>
>>
>> Webrev URL: 
>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/3-jdk9-hs-open/
>>
>>
>> Testing:
>>
>> - There is a temporary change in hotspot/test/TEST.groups that allows
>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java to
>>   run during JPRT jobs. That test is normally excluded from the
>>   hotspot_fast_runtime group in JPRT jobs.
>> - I've copied and modified the TooSmallStackSize.java test from the
>>   launcher's tests in the jdk repo. The jdk repo version exercises the
>>   '-Xss<n_bytes>' option. The hotspot repo version exercises the
>>   '-XX:ThreadStackSize=<k_bytes>', 
>> '-XX:CompilerThreadStackSize=<k_bytes>',
>>   and '-XX:VMThreadStackSize=<k_bytes>' options.
>> - RBT JDK9-hs nightly results were generated and no regressions were
>>   observed; another round was started after the latest resync, but has
>>   not yet finished
>>
>>
>> Future Work:
>>
>> - Add {compiler,vm_internal}_thread_min_stack_allowed support to Windows
>>
>>   - Have the beginnings of an implementation, but stack sizes and the
>>     underlying pages aren't working as I expect.
>>
>> - Switch TooSmallStackSize.java from local TestHelper.java to the newer
>>   test framework
>>
>>   - TestHelper.java is a copy of the launcher's TestHelper.java since
>>     that was the easiest way to bring up the new test. The dependencies
>>     in TooSmallStackSize.java on TestHelper.java need to be identified
>>     and replaced with newer test library equivalents if possible.
>>
>> - Determine whether the minimum stack values can be reduced on the
>>   different platforms.
>>
>>   - With a single minimum stack size value, it's possible that the
>>     minimum has been bumped up over the years for one thread type,
>>     e.g., Compiler threads, when the other thread types didn't need
>>     the extra space.
>>   - Will need to do some spelunking through the change histories
>>     and then some old fashioned value range testing.
>>
>> - There is a block of Solaris specific code in
>>   os::Posix::set_minimum_stack_sizes(); Coleen filed the following
>>   bug to remove that code:
>>
>>   JDK-8161093 Solaris for >8k pagesize adds extra guard pages
>>   https://bugs.openjdk.java.net/browse/JDK-8161093
>>
>> Thanks, in advance, for any comments, suggestions or questions!
>>
>> Dan (, Ron and Coleen)
>