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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Bug: https://bugs.openjdk.java.net/browse/JDK-8189170

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


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
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
Hi David,

Looks mostly fine, nice cleanup.

Minor nits:

os_bsd.cpp, os_windows.cpp:

I do not like the fact that we offer a generic function but do not
implement it. The comment

+// Unlike Linux we don't try to handle the initial process
+// thread in any special way - so just report 'false'

assumes a given usage of this function, but the function looks general
purpose and may be used in different contexts in the future. I'd either
invest the work to implement it on bsd/windows or at least add a comment
that the function should not be relied upon on these platforms.

Or, something like this:
static bool is_primordial_thread(void) WINDOWS_ONLY({return false;})
BSD_ONLY({return false;})

-----

src/hotspot/os/linux/os_linux.cpp

+  //   But ensure we don't underflow the stack size - allow 1 page spare
+  if (stack_size >= (size_t)(3 * page_size()))
   stack_size -= 2 * page_size();

Could you please add {} ?

----

Sidenote (not touched by your thread):

3045     uintptr_t stack_extent = (uintptr_t)
os::Linux::initial_thread_stack_bottom();
3046     unsigned char vec[1];
3047
3048     if (mincore((address)stack_extent, os::vm_page_size(), vec) == -1)
{

I feel a tiny bit queasy about this coding. mincore(2) is defined to work
with pages as returned by sysconf(_SC_PAGESIZE). Should the implementation
of os::vm_page_size() ever be something different than
sysconf(_SC_PAGESIZE), this may overflow vec.

-----

src/hotspot/share/runtime/os.hpp

s/Some platforms have to special-case handling/Some platforms need
special-case handling

----

Kind Regards, Thomas


On Tue, Nov 14, 2017 at 11:39 AM, David Holmes <[hidden email]>
wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>
> webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>
>
> 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
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Thomas,

Thanks for taking a look so quickly.

On 14/11/2017 9:07 PM, Thomas Stüfe wrote:

> Hi David,
>
> Looks mostly fine, nice cleanup.
>
> Minor nits:
>
> os_bsd.cpp, os_windows.cpp:
>
> I do not like the fact that we offer a generic function but do not
> implement it. The comment
>
> +// Unlike Linux we don't try to handle the initial process
> +// thread in any special way - so just report 'false'
>
> assumes a given usage of this function, but the function looks general
> purpose and may be used in different contexts in the future. I'd either
> invest the work to implement it on bsd/windows or at least add a comment
> that the function should not be relied upon on these platforms.

I went hunting for a way to implement it and could not find anything. I
hadn't realized initially that we didn't already have something on each
platform. Given the narrow problem this is trying to solve it's already
exceeded the time budget I can allocate to it. I was tempted to drop
back to having a Linux only solution ... but three platforms can support
it, while three platforms can't.

> 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.

> -----
>
> src/hotspot/os/linux/os_linux.cpp
>
> +  //   But ensure we don't underflow the stack size - allow 1 page spare
> +  if (stack_size >= (size_t)(3 * page_size()))
>     stack_size -= 2 * page_size();
>
> Could you please add {} ?

Fixed.

> ----
>
> Sidenote (not touched by your thread):
>
> 3045     uintptr_t stack_extent = (uintptr_t)
> os::Linux::initial_thread_stack_bottom();
> 3046     unsigned char vec[1];
> 3047
> 3048     if (mincore((address)stack_extent, os::vm_page_size(), vec) ==
> -1) {
>
> I feel a tiny bit queasy about this coding. mincore(2) is defined to
> work with pages as returned by sysconf(_SC_PAGESIZE). Should the
> implementation of os::vm_page_size() ever be something different than
> sysconf(_SC_PAGESIZE), this may overflow vec.

Is there any reason to think it would ever be something different? On
Linux os::vm_page_size() returns os::Linux::page_size() which is set by:

   Linux::set_page_size(sysconf(_SC_PAGESIZE));

> -----
>
> src/hotspot/share/runtime/os.hpp
>
> s/Some platforms have to special-case handling/Some platforms need
> special-case handling

Fixed.

Thanks,
David

> ----
>
> Kind Regards, Thomas
>
>
> On Tue, Nov 14, 2017 at 11:39 AM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>     <https://bugs.openjdk.java.net/browse/JDK-8189170>
>
>     webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>     <http://cr.openjdk.java.net/~dholmes/8189170/webrev/>
>
>
>     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
>
>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
Hi David,

On Tue, Nov 14, 2017 at 1:10 PM, David Holmes <[hidden email]>
wrote:

> Hi Thomas,
>
> Thanks for taking a look so quickly.
>
> On 14/11/2017 9:07 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> Looks mostly fine, nice cleanup.
>>
>> Minor nits:
>>
>> os_bsd.cpp, os_windows.cpp:
>>
>> I do not like the fact that we offer a generic function but do not
>> implement it. The comment
>>
>> +// Unlike Linux we don't try to handle the initial process
>> +// thread in any special way - so just report 'false'
>>
>> assumes a given usage of this function, but the function looks general
>> purpose and may be used in different contexts in the future. I'd either
>> invest the work to implement it on bsd/windows or at least add a comment
>> that the function should not be relied upon on these platforms.
>>
>
> I went hunting for a way to implement it and could not find anything. I
> hadn't realized initially that we didn't already have something on each
> platform. Given the narrow problem this is trying to solve it's already
> exceeded the time budget I can allocate to it. I was tempted to drop back
> to having a Linux only solution ... but three platforms can support it,
> while three platforms can't.
>
> 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.
>
> -----
>>
>> src/hotspot/os/linux/os_linux.cpp
>>
>> +  //   But ensure we don't underflow the stack size - allow 1 page spare
>> +  if (stack_size >= (size_t)(3 * page_size()))
>>     stack_size -= 2 * page_size();
>>
>> Could you please add {} ?
>>
>
> Fixed.
>
> ----
>>
>> Sidenote (not touched by your thread):
>>
>> 3045     uintptr_t stack_extent = (uintptr_t)
>> os::Linux::initial_thread_stack_bottom();
>> 3046     unsigned char vec[1];
>> 3047
>> 3048     if (mincore((address)stack_extent, os::vm_page_size(), vec) ==
>> -1) {
>>
>> I feel a tiny bit queasy about this coding. mincore(2) is defined to work
>> with pages as returned by sysconf(_SC_PAGESIZE). Should the implementation
>> of os::vm_page_size() ever be something different than
>> sysconf(_SC_PAGESIZE), this may overflow vec.
>>
>
> Is there any reason to think it would ever be something different? On
> Linux os::vm_page_size() returns os::Linux::page_size() which is set by:
>
>   Linux::set_page_size(sysconf(_SC_PAGESIZE));
>
>
We had https://bugs.openjdk.java.net/browse/JDK-8186665 and since then I'm
wary around mincore(2).. Basically, on AIX we decide - for complicated
reasons - to make os::vm_page_size() a larger size than the system page
size. This triggered the overflow with mincore.

But you are right, the implementation of os::vm_page_size() is is not very
probable to change. One could add a sentinel like this:

unsigned char vec[2];
vec[1] = 'X';

mincore((address)stack_extent, os::vm_page_size(), vec);

assert(vec[1] == 'X');

but maybe it is not worth the trouble.


> -----
>>
>> src/hotspot/share/runtime/os.hpp
>>
>> s/Some platforms have to special-case handling/Some platforms need
>> special-case handling
>>
>
> Fixed.
>
> Thanks,
> David
>
>
Thanks, Thomas


> ----
>>
>> Kind Regards, Thomas
>>
>>
>>
>> On Tue, Nov 14, 2017 at 11:39 AM, David Holmes <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>>     <https://bugs.openjdk.java.net/browse/JDK-8189170>
>>
>>     webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>>     <http://cr.openjdk.java.net/~dholmes/8189170/webrev/>
>>
>>
>>     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
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by David Holmes
Looking for a runtime Reviewer please.

Also Tomas: are you able to test this patch with R?

Thanks,
David

On 14/11/2017 8:39 PM, David Holmes wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8189170
>
> webrev: http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by David Holmes
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
>

Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
On Fri, Nov 17, 2017 at 4:40 PM, Daniel D. Daugherty <
[hidden email]> 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
>
>
>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
In reply to this post by Daniel D. Daugherty
Ooops, pressed Send to soon...

...
>


> 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.
>
>
Yes, that's what I meant. Putting that into os.hpp makes it sprint up
readily, e.g. in Tooltips in Eclipse CDT when hovering over the function
call.

..Thomas


> 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
>
>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Daniel D. Daugherty
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
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
Hi David,

looks fine.

Thanks, Thomas

On Mon, Nov 20, 2017 at 4:44 AM, David Holmes <[hidden email]>
wrote:

> 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/2
> 017-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
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
I'm also good with this change.

Dan


On 11/20/17 4:04 AM, Thomas Stüfe wrote:

> Hi David,
>
> looks fine.
>
> Thanks, Thomas
>
> On Mon, Nov 20, 2017 at 4:44 AM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     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/
>     <http://cr.openjdk.java.net/%7Edholmes/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
>     <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
>             <https://bugs.openjdk.java.net/browse/JDK-8189170>
>
>             webrev:
>             http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>             <http://cr.openjdk.java.net/%7Edholmes/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
>
>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Thank you both.

David

On 20/11/2017 11:05 PM, Daniel D. Daugherty wrote:

> I'm also good with this change.
>
> Dan
>
>
> On 11/20/17 4:04 AM, Thomas Stüfe wrote:
>> Hi David,
>>
>> looks fine.
>>
>> Thanks, Thomas
>>
>> On Mon, Nov 20, 2017 at 4:44 AM, David Holmes <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     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/
>>     <http://cr.openjdk.java.net/%7Edholmes/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
>>     <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
>>             <https://bugs.openjdk.java.net/browse/JDK-8189170>
>>
>>             webrev:
>>             http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>>             <http://cr.openjdk.java.net/%7Edholmes/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
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Tomas Kalibera

I have tested with R and it seems to be working fine.

Specifically, I've tested with jdk/hs 47868 as baseline, with R-devel
73756, rJava 0.9-9 (modified to print VM args). I've run R recursive
calls after initializing the jvm via .jinit(), using
RJAVA_JVM_STACK_WORKAROUND=1 (detect inserted guard pages, but only
print a message when they appear).

The baseline inserted guard pages as expected following -Xss (default
and when specified 1m..9m) and following the implied cap when rlimit
stack is unlimited, and R consequenlty crashed in the baseline. The
modified version with the fix activated did not insert guard pages.
Tested with rlimit stack of 8m (Ubuntu default) and unlimited.

 From R, I activated the fix via
options(java.parameters=c("-XX:+UnlockExperimentalVMOptions",
"-XX:+DisablePrimordialThreadGuardPages"))

Tested on Ubuntu 17.04, x86_64. jdk/hs built with gcc 4.9.4 (but still
needed --disable-warnings-as-errors)

Thanks a lot for the new option!
Tomas

On 11/20/2017 02:06 PM, David Holmes wrote:

> Thank you both.
>
> David
>
> On 20/11/2017 11:05 PM, Daniel D. Daugherty wrote:
>> I'm also good with this change.
>>
>> Dan
>>
>>
>> On 11/20/17 4:04 AM, Thomas Stüfe wrote:
>>> Hi David,
>>>
>>> looks fine.
>>>
>>> Thanks, Thomas
>>>
>>> On Mon, Nov 20, 2017 at 4:44 AM, David Holmes
>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>>     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/
>>> <http://cr.openjdk.java.net/%7Edholmes/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
>>> <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
>>> <https://bugs.openjdk.java.net/browse/JDK-8189170>
>>>
>>>             webrev:
>>> http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>>> <http://cr.openjdk.java.net/%7Edholmes/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
>>>
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Thanks for confirming Tomas!

David

On 21/11/2017 12:37 AM, Tomas Kalibera wrote:

>
> I have tested with R and it seems to be working fine.
>
> Specifically, I've tested with jdk/hs 47868 as baseline, with R-devel
> 73756, rJava 0.9-9 (modified to print VM args). I've run R recursive
> calls after initializing the jvm via .jinit(), using
> RJAVA_JVM_STACK_WORKAROUND=1 (detect inserted guard pages, but only
> print a message when they appear).
>
> The baseline inserted guard pages as expected following -Xss (default
> and when specified 1m..9m) and following the implied cap when rlimit
> stack is unlimited, and R consequenlty crashed in the baseline. The
> modified version with the fix activated did not insert guard pages.
> Tested with rlimit stack of 8m (Ubuntu default) and unlimited.
>
>  From R, I activated the fix via
> options(java.parameters=c("-XX:+UnlockExperimentalVMOptions",
> "-XX:+DisablePrimordialThreadGuardPages"))
>
> Tested on Ubuntu 17.04, x86_64. jdk/hs built with gcc 4.9.4 (but still
> needed --disable-warnings-as-errors)
>
> Thanks a lot for the new option!
> Tomas
>
> On 11/20/2017 02:06 PM, David Holmes wrote:
>> Thank you both.
>>
>> David
>>
>> On 20/11/2017 11:05 PM, Daniel D. Daugherty wrote:
>>> I'm also good with this change.
>>>
>>> Dan
>>>
>>>
>>> On 11/20/17 4:04 AM, Thomas Stüfe wrote:
>>>> Hi David,
>>>>
>>>> looks fine.
>>>>
>>>> Thanks, Thomas
>>>>
>>>> On Mon, Nov 20, 2017 at 4:44 AM, David Holmes
>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>>
>>>>     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/
>>>> <http://cr.openjdk.java.net/%7Edholmes/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 
>>>>
>>>> <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
>>>> <https://bugs.openjdk.java.net/browse/JDK-8189170>
>>>>
>>>>             webrev:
>>>> http://cr.openjdk.java.net/~dholmes/8189170/webrev/
>>>> <http://cr.openjdk.java.net/%7Edholmes/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
>>>>
>>>>
>>>>
>>>
>