RFR(L) 8140520 segfault on solaris-amd64 with "-XX:VMThreadStackSize=1" option
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Sep 9 22:55:25 UTC 2016
Thanks!
Dan
On 9/9/16 4:47 PM, David Holmes wrote:
> Update looks fine Dan!
>
> Thanks,
> David
>
> On 10/09/2016 4:02 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've updated both tests to address comments from David H and Coleen.
>>
>> Changes made in this (hopefully final) round:
>>
>> hotspot/test/runtime/Thread/TooSmallStackSize.java
>>
>> - updated options comment (Coleen)
>> - added more diagnostics for RuntimeException failure mode (David)
>>
>> hotspot/test/TEST.groups
>>
>> - backed out the temporary change for testing during JPRT runs
>> - this file is no longer changed by this bug fix
>>
>> jdk/test/tools/launcher/TooSmallStackSize.java
>>
>> - updated options comment (Coleen)
>> - added more diagnostics for RuntimeException failure modes
>> in three places (David)
>>
>> The diagnostics were tested by temporarily inducing failures
>> at each point and verifying that the extra output was produced.
>>
>>
>> Webrev URL:
>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/5-jdk9-hs-open/
>>
>> Dan
>>
>>
>>
>>
>> On 9/9/16 10:00 AM, Daniel D. Daugherty wrote:
>>> On 9/9/16 5:48 AM, Coleen Phillimore wrote:
>>>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/4-jdk9-hs-open/jdk/test/tools/launcher/TooSmallStackSize.java.udiff.html
>>>>
>>>>
>>>>
>>>> + *
>>>> + * Note: This repo's version of the test uses the '-Xss' option. The
>>>> + * hotspot repo's version of the test uses the
>>>> '-XX:ThreadStackSize=n'
>>>> + * VM option.
>>>>
>>>>
>>>> Can you add a sentence, that this is the same thing to make it clear?
>>>
>>> I'll figure out a wording change and apply it to both tests.
>>>
>>>
>>>>
>>>> This test is a smaller version of the one in the runtime
>>>> repository? Do we need both?
>>>
>>> The JDK repo test belongs to the launcher team. It intentionally only
>>> tests the option (-Xss) that the launcher also interprets. Yes, the
>>> -Xss option is interpreted by the launcher for initial thread stack
>>> size and by the VM for subsequent Java thread stack sizes...
>>>
>>> The hotspot repo version of the test covers the the three HotSpot
>>> specific -XX options that control thread stack size. So the two tests
>>> are each focused on their respective tech areas and we need them
>>> both.
>>>
>>>
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/4-jdk9-hs-open/hotspot/test/runtime/Thread/TooSmallStackSize.java.html
>>>>
>>>>
>>>>
>>>> This looks great. I think this is a good test:
>>>>
>>>> 177 * Try again with a the minimum stack size that was
>>>> given in the error message
>>>>
>>>>
>>>> The change looks good.
>>>
>>> Thanks for the review!
>>>
>>> Dan
>>>
>>>
>>>> Thanks!
>>>> Coleen
>>>>
>>>>
>>>> On 9/8/16 9:05 PM, Daniel D. Daugherty wrote:
>>>>> > Future Work:
>>>>> >
>>>>> > - 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.
>>>>>
>>>>> hotspot/test/runtime/Thread/TooSmallStackSize.java has been rewritten
>>>>> to use the newer test framework and
>>>>> hotspot/test/runtime/Thread/TestHelper.java
>>>>> is no longer included in the path.
>>>>>
>>>>> Webrev URL:
>>>>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/4-jdk9-hs-open/
>>>>>
>>>>> Other than timestamps, only
>>>>> hotspot/test/runtime/Thread/TooSmallStackSize.java
>>>>> is changed in this round.
>>>>>
>>>>> I need just a couple of reviews of the new test.
>>>>>
>>>>> I added some of the test output to this comment in the bug report:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8140520?focusedCommentId=13999090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13999090
>>>>>
>>>>>
>>>>>
>>>>> so the new test passes on all platforms.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>
>>>>> On 9/2/16 12: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)
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
More information about the hotspot-runtime-dev
mailing list