RFR(M): 8170655: [posix] Fix minimum stack size computations
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Dec 13 08:16:00 UTC 2016
Hi Dan,
what's the status of my changes? I think hs is open again, at least
I see changes popping up ... just a gentle reminder :)
Should I rebase the changes to the latest repo version?
Best regards,
Goetz.
> -----Original Message-----
> From: Daniel D. Daugherty [mailto:daniel.daugherty at oracle.com]
> Sent: Dienstag, 6. Dezember 2016 22:15
> 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 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/v
> m/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?
>
> 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