RFR(M): 8170655: [posix] Fix minimum stack size computations
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Dec 6 22:21:13 UTC 2016
On 12/6/16 4:15 PM, Daniel D. Daugherty wrote:
> On 12/6/16 1:31 PM, Lindenmaier, Goetz wrote:
>> Hi Dan,
>>
>> I fixed the comment. I will post a new webrev once the other
>> change is pushed so that I can make a real change.
>
> I'll resync my repo to the webrev.02 version and restart my
> testing on this fix. I'm hoping we have only comment changes
> after the webrev.02 version...
>
> Also gotta check the test results on 8169373.
>
>
>>
>>> Meta question: Now _java_thread_min_stack_allowed is specific
>>> to an
>>> OS/CPU platform (as are the other two). That OS/CPU platform
>>> enumeration
>>> doesn't take into account whether C1 or C2 is in play. We used
>>> to add
>>> just a little more space via the COMPILER2_PRESENT() macro,
>>> but now we
>>> don't. Yes, I know that you've done a bunch of testing and I'm
>>> running
>>> our usual batch of tests also, but this subtle change is
>>> bothering
>> Yep, I could have made sizing with C1, then C2, then TieredCompilation,
>> but I don't think that would make a change.
>> This extra size was meant for the compiler thread. 83251780 moved it
>> to the
>> Java threads. I think it was there because C2 used to have recursive
>> algorithms.
>> But it only would have had an effect in rare situations because the
>> other MAX
>> should mostly be used on systems with large pages, where the alignement
>> would bring more than these 4K. The compiler gained the alignment of
>> the
>> vm_default_page_size() region plus the space gained by bigger
>> StackShadowPages
>> which were huge at some point (20+ pages * 64K ...). Remember the
>> compiler does
>> no stack banging and thus could use the shadow region.
>>
>> The original code looked like this:
>>
>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/os/linux/vm/os_linux.cpp
>>
>> os::Linux::min_stack_allowed = MAX2(os::Linux::min_stack_allowed,
>> (size_t)(StackYellowPages+StackRedPages+StackShadowPages) *
>> Linux::page_size() +
>> (2*BytesPerWord COMPILER2_PRESENT(+1)) *
>> Linux::vm_default_page_size());
>
> I'm good with this explanation. Coleen?
Yes, I'm convinced.
Coleen
>
> Dan
>
>
>>
>> Best regards,
>> Goetz.
>>
>>> -----Original Message-----
>>> From: Daniel D. Daugherty [mailto:daniel.daugherty at oracle.com]
>>> Sent: Tuesday, December 06, 2016 8:32 PM
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Coleen Phillimore
>>> <coleen.phillimore at oracle.com>; hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR(M): 8170655: [posix] Fix minimum stack size
>>> computations
>>>
>>> On 12/6/16 2:35 AM, Lindenmaier, Goetz wrote:
>>>> Hi Coleen,
>>>>
>>>> thanks for looking at this change!
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~goetz/wr16/8170655-
>>> compilerGuardFix/webrev.02/
>>>
>>> src/cpu/ppc/vm/globals_ppc.hpp
>>> No comments.
>>>
>>> src/cpu/x86/vm/globals_x86.hpp
>>> No comments.
>>>
>>> src/os/posix/vm/os_posix.cpp
>>> L1112: // It dependes on word size, platform calling
>>> conventions, C
>>> frame layout and
>>> Typo: 'dependes' -> 'depends'
>>>
>>> If you put "It" at the end of the sentence on the previous
>>> line, then the formatting will look better. :-)
>>>
>>> Meta question: Now _java_thread_min_stack_allowed is specific
>>> to an
>>> OS/CPU platform (as are the other two). That OS/CPU platform
>>> enumeration
>>> doesn't take into account whether C1 or C2 is in play. We used
>>> to add
>>> just a little more space via the COMPILER2_PRESENT() macro,
>>> but now we
>>> don't. Yes, I know that you've done a bunch of testing and I'm
>>> running
>>> our usual batch of tests also, but this subtle change is
>>> bothering
>>> me...
>>>
>>> src/os/posix/vm/os_posix.hpp
>>> No comments.
>>>
>>> src/os_cpu/aix_ppc/vm/os_aix_ppc.cpp
>>> No comments.
>>>
>>> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
>>> No comments.
>>>
>>> src/os_cpu/linux_aarch64/vm/os_linux_aarch64.cpp
>>> No comments.
>>>
>>> src/os_cpu/linux_ppc/vm/os_linux_ppc.cpp
>>> No comments.
>>>
>>> src/os_cpu/linux_s390/vm/os_linux_s390.cpp
>>> No comments.
>>>
>>> src/os_cpu/linux_sparc/vm/os_linux_sparc.cpp
>>> No comments.
>>>
>>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>>> No comments.
>>>
>>> src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
>>> No comments.
>>>
>>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>> No comments.
>>>
>>> src/share/vm/runtime/os.hpp
>>> No comments.
>>>
>>> Dan
>>>
>>>
>>>> See my comments inline.
>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>>>>> bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
>>>>> Sent: Dienstag, 6. Dezember 2016 01:00
>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: Re: RFR(M): 8170655: [posix] Fix minimum stack size
>>> computations
>>>>>
>>>>>
>>>>> On 12/5/16 6:27 PM, Daniel D. Daugherty wrote:
>>>>>> On 12/3/16 11:17 AM, Lindenmaier, Goetz wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I would like to fix two issues of minimum stack size computation:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170655-
>>>>> compilerGuardFix/webrev.01/
>>>>>>> Please review and sponsor.
>>>>>> I'm sponsoring the related bug:
>>>>>>
>>>>>> 8169373 Work around linux NPTL stack guard error
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8169373
>>>>>>
>>>>>> so I guess I should sponsor this one also... For obvious reasons,
>>>>>> this
>>>>>> fix will also need a "full RBT" run...
>>>>> Thank you, Dan for sponsoring this.
>>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170655-
>>>>> compilerGuardFix/webrev.01/
>>>>>> src/cpu/ppc/vm/globals_ppc.hpp
>>>>>> No comments.
>>>>>>
>>>>>> src/cpu/x86/vm/globals_x86.hpp
>>>>>> No comments.
>>>>>>
>>>>>> src/os/posix/vm/os_posix.cpp
>>>>>> My brain read right past where you took out "MAX2" and
>>>>>> changed the
>>>>>> math to "cur = cur + guard_size + shadow_size". I think
>>>>>> I've been
>>>>>> staring at that particular line of code for way too long
>>>>>> (in a couple
>>>>>> of different bug fixes)...
>>>>>>
>>>>>> I think with your fix that Solaris specific block on
>>>>>> L1132 - L1148 can go away (see JDK-8161093). Hopefully
>>>>>> Coleen will chime in on just this part.
>>>>> Yes, I think with refactoring, this is dead code. I think you
>>>>> should
>>>>> remove it, and make 8161093 a duplicate so it can be closed with
>>>>> this fix.
>>>> I removed the code and closed the bug as duplicate.
>>>>
>>>>> The changes in this file (adding the guard_size and shadow_size)
>>>>> are a
>>>>> lot cleaner than the max of some random number of pages someone
>>>>> calculated a long time ago.
>>>> Thanks!
>>>>
>>>>> There's a comment above this function that should be rewritten also
>>> though:
>>>>> // Check minimum allowable stack sizes for thread creation and to
>>> initialize
>>>>> // the java system classes, including StackOverflowError - depends on
>>> page
>>>>> // size. Add two 4K pages for compiler2 recursion in main thread.
>>>>> // Add in 4*BytesPerWord 4K pages to account for VM stack during
>>>>> // class initialization depending on 32 or 64 bit VM.
>>>> Dan saw this too. I wrote a summary of the text in the bug.
>>>>
>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170655-
>>>>>
>>> compilerGuardFix/webrev.01/src/os_cpu/linux_sparc/vm/os_linux_sparc.cpp
>>> .u
>>>>> diff.html
>>>>> This seems small. I don't think it should be reduced because I don't
>>>>> think we have a lot of testing for this platform.
>>>> I changed it to 64K. I can't test on this platform, so this
>>>> probably really is
>>> better.
>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170655-
>>>>>
>>> compilerGuardFix/webrev.01/src/os_cpu/linux_x86/vm/os_linux_x86.cpp.ud
>>> iff.
>>>>> html
>>>>> Why is DEBUG_ONLY( + 4) removed? We do run native JVM code with
>>>>> debugging on in Java threads also, although the +4 seems arbitrary.
>>>> I checked startup and ran jvm98 and did not find any differences in
>>> slowdebug/opt
>>>> variants. So I thought I should get rid of this. Also, this kind
>>>> of accounts for
>>> the
>>>> extra space of shadow pages in dbg builds, which now is no more
>>>> included
>>> in that
>>>> number.
>>>>
>>>>> If you're removing the concept of DEBUG_ONLY to these sizes, it still
>>>>> remains in this change:
>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170655-
>>>>>
>>> compilerGuardFix/webrev.01/src/os_cpu/linux_s390/vm/os_linux_s390.cpp.
>>> udi
>>>>> ff.html
>>>> On s390, I quickly ran into stack overflows with the debug build.
>>>> Therefore
>>>> I increased the sizes.
>>>>
>>>>> I assume that all these minimum stack sizes have been tested in
>>>>> your lab.
>>>> Yes, the change is running with our nighly tests on the platforms
>>>> we have
>>> available.
>>>> This includes jck tests and hotspot jtreg tests.
>>>>
>>>>> This seems like a good change but I hope this is the last of these
>>>>> until
>>>>> JDK10 opens.
>>>> :) Yes, stack changes kind of got my special hobby ... I still have
>>> implementing
>>>> StackReservedPages support on s390 on my list, but that should be
>>> platform-only.
>>>> Also I'm not sure whether I'll find time for that.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>>> L1154: _compiler_thread_min_stack_allowed =
>>>>>> _compiler_thread_min_stack_allowed +
>>>>>> Please add a comment above this line:
>>>>>>
>>>>>> // Reminder: a compiler thread is-a Java thread.
>>>>>>
>>>>>> src/os/posix/vm/os_posix.hpp
>>>>>> L48: // Set_minimum_stack_sizes() ...
>>>>>> Please change 'S' -> 's' since it is the function's name.
>>>>>>
>>>>>> src/os_cpu/aix_ppc/vm/os_aix_ppc.cpp
>>>>>> L539: // HotSpotguard pages is added later.
>>>>>> Typo: space before 'guard'
>>>>>>
>>>>>> L542: size_t
>>>>>> os::Posix::_vm_internal_thread_min_stack_allowed = 64
>>>>>> * K;
>>>>>> VM internal thread size lowered from 128K to 64K
>>>>>> without any
>>>>>> changes to how _vm_internal_thread_min_stack_allowed is
>>>>>> set.
>>>>>> Any particular reason for this change?
>>>>>>
>>>>>> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/os_cpu/linux_aarch64/vm/os_linux_aarch64.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/os_cpu/linux_ppc/vm/os_linux_ppc.cpp
>>>>>> L542: size_t
>>>>>> os::Posix::_vm_internal_thread_min_stack_allowed = 64
>>>>>> * K;
>>>>>> VM internal thread size lowered from 128K to 64K
>>>>>> without any
>>>>>> changes to how _vm_internal_thread_min_stack_allowed is
>>>>>> set.
>>>>>> Any particular reason for this change?
>>>>>>
>>>>>> src/os_cpu/linux_s390/vm/os_linux_s390.cpp
>>>>>> L478: size_t os::Posix::_compiler_thread_min_stack_allowed
>>>>>> = (52
>>>>>> DEBUG_ONLY(+32)) * K;
>>>>>> L479: size_t os::Posix::_java_thread_min_stack_allowed = (32
>>>>>> DEBUG_ONLY(+8)) * K;
>>>>>> consistency - trying to put space around operators so...
>>>>>>
>>>>>> '+32' -> '+ 32'
>>>>>> '+8' -> '+ 8'
>>>>>>
>>>>>> L480: size_t
>>>>>> os::Posix::_vm_internal_thread_min_stack_allowed = 32
>>>>>> * K;
>>>>>> VM internal thread size lowered from 128K to 32K
>>>>>> without any
>>>>>> changes to how _vm_internal_thread_min_stack_allowed is
>>>>>> set.
>>>>>> Any particular reason for this change?
>>>>>>
>>>>>> src/os_cpu/linux_sparc/vm/os_linux_sparc.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/share/vm/runtime/os.hpp
>>>>>> L439: java_thread, // Java, CocdeCacheSweeper,
>>>>>> JVMTIAgent and Service threads.
>>>>>> Typo: 'CocdeCacheSweeper' -> 'CodeCacheSweeper'
>>>>>>
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>>
>>>>>>> This are the issues I excluded from the "8169373: Work around linux
>>>>>>> NPTL stack guard error."
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I wrote a lengthy explanation in the bug, trying to comment on what
>>> was
>>>>>>> said in the other thread. I'll repeat it here, I think that's
>>>>>>> better
>>>>>>> for discussion.
>>>>>>>
>>>>>>> Dan, thanks for improving the text, I use the improved variant
>>>>>>> here:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> HotSpot has three cmd line options to set stack sizes (besides
>>>>>>> -Xss):
>>>>>>>
>>>>>>> -XX:ThreadStackSize for threads executing Java code.
>>>>>>>
>>>>>>> -XX:CompilerThreadStackSize for threads used by the JIT
>>>>>>> compilers.
>>>>>>>
>>>>>>> -XX:VMThreadStackSize for threads executing VM internal
>>>>>>> tasks as gc.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> All these flags should not be set to a value that leads to a stack
>>>>>>> overflow before user code can be executed. As the VM executes a lot
>>>>>>> of code for initialization and also the JIT already compiles
>>>>>>> methods,
>>>>>>> considerable amounts of stack can be used during startup. We
>>>>>>> must try
>>>>>>> to avoid stack overflows before startup is complete as error
>>>>>>> handling
>>>>>>> might not be properly in place yet.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Required minimum stack sizes depend on frame sizes and program
>>>>>>> execution paths. Frame sizes again depend on the C compiler
>>>>>>> used, the
>>>>>>> platform compiled for, and design decisions in interpreter, C1
>>>>>>> and C2.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Required stack sizes also depend on option settings, e.g. with
>>>>>>> JVM/TI
>>>>>>> enabled, frames can get bigger. With inlining increased JIT
>>>>>>> compilers
>>>>>>> might do more optimizations leading to deeper call chains, etc.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> While the minimum stack sizes should reflect differences in
>>>>>>> Platform
>>>>>>> and compiler, they must not, and cannot, cover all possible option
>>>>>>> settings.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This change addresses two issues:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 1. Fixed minimum stack size configuration
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Currently, the minimum Java thread size is given in a constant per
>>>>>>> os/cpu platform for each of the three stack kinds. This number
>>>>>>> includes the size required for guard pages.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The guard pages are used for stack overflow detection. They make
>>>>>>> up 4
>>>>>>> zones on the stack: Red, Yellow, Reserved and Shadow pages. The
>>>>>>> Red,
>>>>>>> Yellow and Reserved pages are protected to detect stack overflows.
>>>>>>> The Shadow pages are just some extra space to allow methods that
>>>>>>> don't do a stack bang to execute.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Unfortunately, the size required for guard pages is not fixed at
>>>>>>> compile time. It depends on the concrete system the VM is
>>>>>>> started on.
>>>>>>> Thus the minimum sizes given can be too small to hold the guard
>>>>>>> pages. This lead to errors in the past that were solved by
>>>>>>> introducing code that overruled the per-platform minimum stack
>>>>>>> size.
>>>>>>> This code nowadays is the MAX2() in os_posix.cpp:1114 and the
>>> SOLARIS
>>>>>>> special case further down. It uses the value (4 * BytesPerWord
>>>>>>> COMPILER2_PRESENT(+ 2)) * 4 * K) (os_posix.cpp:1117) as minimum
>>>>>>> required space for frames. Thereby it effectively overrules the
>>>>>>> minimum stack size settings given in the os/cpu constants, and
>>>>>>> there
>>>>>>> is currently no way to specify this size per platform.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This change proposes to fix this issue by specifying the space
>>>>>>> needed
>>>>>>> for stack frames in the os/cpu constants. During startup, this
>>>>>>> number
>>>>>>> is increased by the space required for the guard pages. Thus, this
>>>>>>> takes into account the page size of the concrete system the VM runs
>>>>>>> on, and also eventual changes to the guard pages by the flags
>>>>>>> StackRed/Yellow/Reserved/Shadow/Pages. This gives the opportunity
>>> to
>>>>>>> reduce the minimum stack sizes on systems with small pages.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Minimum stack size configuration is more simple with this change
>>>>>>> and
>>>>>>> valid for systems with any page size.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2. Stack guard pages not considered for compiler thread stacks
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Compiler threads are Java threads. The C++ class CompilerThread
>>>>>>> is a
>>>>>>> subclass of JavaThread. When a compiler thread is started,
>>>>>>> JavaThread::run() is executed which protects the red, yellow and
>>>>>>> reserved pages.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Since 8140520 the minimum stack sizes for Compiler and VM internal
>>>>>>> threads no longer include the space for the guard pages. This is
>>>>>>> correct for the VM internal threads, but not for the Compiler
>>>>>>> thread.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> For the HotSpot C1 and C2 compilers it would be fine to reserve
>>>>>>> space
>>>>>>> for the Red/Yellow/Reserved pages, as they don't stack bang with
>>>>>>> the
>>>>>>> shadow page size. But since introducing JVMCI, compilers written in
>>>>>>> Java can be running on the compiler threads. Therefore the shadow
>>>>>>> pages are needed, too.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> As for the Java thread, this change uses a os/cpu constant for the
>>>>>>> required minimum space for compiler frames and then adds the zone
>>>>>>> sizes to the minimum stack sizes during startup.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> New sizing:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The constants of the os/cpu minimum thread sizes are reduced by the
>>>>>>> default guard page sizes and then verified by starting the VM to
>>>>>>> assure the stack still suffices to get through startup. Hotspot
>>>>>>> jtreg
>>>>>>> tests are passing. The overall sizes required (after adding guard
>>>>>>> pages) on the systems I have available get a bit smaller. In most
>>>>>>> cases the sizes even suffice to run simple programs as
>>>>>>> SpecJvm98. The
>>>>>>> table below gives the systems I tested and the required sizes
>>>>>>> reported when started with too small stacks.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> <pre>
>>>>>>>
>>>>>>> Thread kind: Java Compiler VM
>>>>>>>
>>>>>>> old new old new old new
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> bsd x86_64 dbg: 240 232 64 64 64 64
>>>>>>>
>>>>>>> opt: 240 232 64 64 64 64
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> linux x86_64 dbg: 240 144 64 152 64 64
>>>>>>>
>>>>>>> opt: 232 136 64 144 64 64
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> solaris sparc dbg:
>>>>>>>
>>>>>>> opt: 240 192 128 128 128 128
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> aix ppc64 dbg: 512 512 384 512 128 128
>>>>>>>
>>>>>>> opt: 512 512 384 512 128 128
>>>>>>>
>>>>>>> linux ppc64 dbg: 512 384 128 384 128 64
>>>>>>>
>>>>>>> opt: 512 384 128 384 128 64
>>>>>>>
>>>>>>> linux ppc64le dbg: 512 384 128 384 128 64
>>>>>>>
>>>>>>> opt: 512 384 128 384 128 64
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> linux s390 dbg: 236 140 128 184 128 32
>>>>>>>
>>>>>>> opt: 236 124 128 144 128 32
>>>>>>>
>>>>>>> </pre>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>
More information about the hotspot-runtime-dev
mailing list