RFR: 8189170: Add option to disable stack overflow checking in primordial thread for use with JNI_CreateJavaJVM
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Nov 14 12:52:45 UTC 2017
Hi David,
On Tue, Nov 14, 2017 at 1:10 PM, David Holmes <david.holmes at oracle.com>
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 <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