RFR(M): 8170655: [posix] Fix minimum stack size computations
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Dec 6 19:31:56 UTC 2016
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.udiff.
>> 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