RFR: 8189170: Add option to disable stack overflow checking in primordial thread for use with JNI_CreateJavaJVM
David Holmes
david.holmes at oracle.com
Tue Nov 14 12:10:02 UTC 2017
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 <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> 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
>
>
More information about the hotspot-runtime-dev
mailing list