RFR: 8189170: Add option to disable stack overflow checking in primordial thread for use with JNI_CreateJavaJVM

David Holmes david.holmes at oracle.com
Mon Nov 20 03:44:45 UTC 2017


Hi Dan,

Thanks for the review. I've addressed all of yours and Thomas's comments.

http://cr.openjdk.java.net/~dholmes/8189170/webrev.v2/

The main change is:

+   static bool is_primordial_thread(void)
+ #if defined(_WINDOWS) || defined(BSD)
+     // No way to identify the primordial thread.
+     { return false; }
+ #else
+   ;
+ #endif


Also to clarify this is logically a "product" flag for the systems that 
need it, not a diagnostic one. But as we don't want this flag to be in 
common use and it is of such narrow applicability, we have made it 
experimental. This was discussed previously in the CSR review thread for 
this:

http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-October/024859.html

Once Tomas K. has been able to test this, and you and Thomas sign-off on 
the updates, I'll get this pushed. But as I'm away from Thursday it may 
not be until next week.

Thanks,
David
-----


On 18/11/2017 1:40 AM, Daniel D. Daugherty wrote:
> On 11/14/17 5:39 AM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/
> 
> src/hotspot/os/aix/os_aix.cpp
>      No comments.
> 
> src/hotspot/os/aix/os_aix.hpp
>      No comments.
> 
> src/hotspot/os/bsd/os_bsd.cpp
>      L880: // thread in any special way - so just report 'false'
>          Nit - needs a period after 'false' to make it a sentence.
> 
>      L882:     return false;
>          Nit - indent should be 2 spaces.
> 
> src/hotspot/os/bsd/os_bsd.hpp
>      No comments.
> 
> src/hotspot/os/linux/os_linux.cpp
>      L947:   if (stack_size >= (size_t)(3 * page_size()))
>          Nit - needs '{' and '}' on the if-statement body.
> 
>      L3070: // always place it right after end of the mapped region
>          Nit - needs a period after 'region' to make it a sentence.
>          (Not your problem, but you did update the sentence...)
> 
> src/hotspot/os/linux/os_linux.hpp
>      No comments.
> 
> src/hotspot/os/solaris/os_solaris.cpp
>      No comments other than I learned something new about thr_main() :-)
>      Thanks for cleaning up that mess.
> 
> src/hotspot/os/windows/os_windows.cpp
>      L452: // thread in any special way - so just report 'false'
>          Nit - needs a period after 'false' to make it a sentence.
> 
>      L454:     return false;
>          Nit - indent should be 2 spaces.
> 
> src/hotspot/share/gc/shared/threadLocalAllocBuffer.cpp
>      No comments.
> 
> src/hotspot/share/runtime/globals.hpp
>      L1181:   experimental(bool, DisablePrimordialThreadGuardPages, false,
>          Experimental or diagnostic?
> 
> src/hotspot/share/runtime/os.hpp
>      L450:   // that loads/creates the JVM via JNI_CreateJavaVM
>          Nit - needs a period after 'JNI_CreateJavaVM'.
> 
>      L453:   // launcher nevers uses the primordial thread as the main 
> thread, but
>          Typo - 'nevers' -> 'never'
> 
>      L455:   // have to special-case handling of the primordial thread 
> if it attaches
>          Typo - 'have to' -> 'have'
> 
> src/hotspot/share/runtime/thread.cpp
>      I like the new log message.
> 
> src/hotspot/share/runtime/thread.inline.hpp
>      L159:   if (os::uses_stack_guard_pages() &&
>      L160:       !(DisablePrimordialThreadGuardPages && 
> os::is_primordial_thread())) {
>          I'm not sure why, but I had to read these two lines several
>          times before I convinced myself the logic is right.
> 
> I'm good with the code changes. Your call on whether to fix the
> bits or not.
> 
> Regarding this part of Thomas' review:
> 
>>> Or, something like this:
>>> static bool is_primordial_thread(void) WINDOWS_ONLY({return false;}) 
>>> BSD_ONLY({return false;})
>>
>> I can do that. I'll see if anyone else has any comments on this.
> 
> I'm assuming that Thomas means for the above to be at the
> platform independent layer (runtime/os.hpp?) so it is more
> obvious to the reader that the function doesn't do anything
> on those two platforms... I know we dislike such things at
> the platform independent layer, but it does feel appropriate
> to make this limitation more obvious.
> 
> I would be okay if you made the change that Thomas is suggesting.
> 
> Thumbs up!
> 
> Dan
> 
> 
>>
>>
>> There are three parts to this which made it somewhat more complicated 
>> than I had envisaged:
>>
>> 1. Add the flag and disable the guards.
>>
>> This is relatively straight-forward:
>>
>>  void JavaThread::create_stack_guard_pages() {
>>    if (!os::uses_stack_guard_pages() ||
>>        _stack_guard_state != stack_guard_unused ||
>>        (DisablePrimordialThreadGuardPages && 
>> os::is_primordial_thread())) {
>>        log_info(os, thread)("Stack guard page creation for thread "
>>                             UINTX_FORMAT " disabled", 
>> os::current_thread_id());
>>      return;
>>
>> with a tweak in JavaThread::stack_guards_enabled as well.
>>
>> 2. Promote os::XXX::is_initial_thread to os::is_primordial_thread()
>>
>> We have three platforms that already implement this functionality: 
>> Linux, Solaris and AIX. For BSD/OSX and Windows we don't attempt to do 
>> anything different for the primordial thread, and we don't have a way 
>> to detect the primordial thread - so is_primordial_thread() just 
>> returns false.
>>
>> I also tidied up some comments regarding terminology.
>>
>> 3. Thomas noticed that we unconditionally subtract 2*page_size() from 
>> the rlimit stack size, without checking it was bigger than 
>> 2*page_size() to start with. I was unsure how to tackle this. It's no 
>> good leaving stack_size at zero so I opted to ensure we would have at 
>> least one page left. Of course in such cases we would hit the bug in 
>> libc (if it still exists, which seems unlikely but time consuming to 
>> establish).
>>
>> Testing:
>>   - Tier 1 (JPRT) testing with a modified launcher that uses the 
>> primordial thread
>>     - with guard pages enabled
>>     - with guard pages disabled
>>   - Tier 1 (JPRT) normal JDK build (which should be unaffected by this 
>> change)
>>
>> The testing was primarily to ensure that disabling the stack guards on 
>> the primordial thread wasn't totally broken. A number of tests fail:
>> - setting -Xss32m fails for the primordial thread when guards are 
>> enabled and the rlimit is 8m
>> - tests that would hit StackOverflowError crash the VM when guards are 
>> disabled (as you would expect).
>> - runtime/execstack/Testexecstack.java crashes when the guards are 
>> disabled
>>
>> Thanks,
>> David
>>
> 


More information about the hotspot-runtime-dev mailing list