RFR(M): 8170655: [posix] Fix minimum stack size computations
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Dec 6 20:31:01 UTC 2016
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.
> 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());
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