(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads

Tue Mar 21 01:25:19 UTC 2017

Hi David,

On 3/20/17 5:59 PM, David Holmes wrote:
> Hi Chris,
>
> <trimming>
>
> Getting closer :)
>
> Have you seen pthread_attr_setstacksize return EINVAL on very large 
> values? Or does pthread_create just give a ENOMEM?
It gives EAGAIN. The log for the new test contains:

[0.416s][warning][os,thread] Failed to start thread - pthread_create 
failed (EAGAIN) for attributes: stacksize: 9007199254740992k, guardsize: 
0k, detached.
Got exception for stack size 9223372036854774784: 
java.lang.OutOfMemoryError: unable to create native thread: possibly out 
of memory or process/resource limits reached

I assume it's also throwing OOME, which the test is catching and 
(correctly) ignoring.

>
> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>
>> Here's what's changed since webrev.01:
>>
>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>> platforms seem to be immune to this). Then check if the size is zero
>> after rounding up to the page size. Subtract the page size in this case
>> to produce the maximum stack size allowed. Surprisingly I got no
>> complaint from gcc for subtracting from an unsigned value that is known
>> to be 0.
>
> I'm a little surprised at that too. :)
>
>> os_linux.cpp: In os::create_thread(), I also check here to make sure the
>> size is not 0 after adding the guard page and aligning up, and subtract
>> the os page size if it is 0.
>
> What if adding the guard page and rounding up causes overflow so that 
> we get a very small positive stack size?
We already know the stack size is os page aligned up before getting 
here, so actually we could just add the guard page and not align up. You 
would get the same result.
>
>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>> result in subtracting the os page size to get the actual maximum allowed
>> stack size.
>
> Good catch!
>
> Nit: "-Avoid"  -> "- Avoid"
Ok.
>
>>
>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>
> Ok.
>
>> TestThreadStackSizes.java: New test. Creates new threads with every size
>> up to 320k in 1k increments. Then creates threads with a few other sizes
>> that can be problematic.
>
> So this test never actually fails as long as the VM doesn't crash - is 
> that right?
Yes.
>
>   27  * @modules java.base/jdk.internal.misc
>   28  * @library /test/lib
>
> The above are not needed by this test.
Ok.

thanks,

Chris
>
> Thanks,
> David
>
>> thanks,
>>
>> Chris
>>> Chris
>>>
>>>
>>>>>
>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>                   long stackSize) {
>>>>>>         init(group, target, name, stackSize);
>>>>>>     }
>>>>>>
>>>>>> Fortunately we already force the stackSize to be at least
>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>> assert by creating a thread with size 257k.
>>>>>
>>>>> Note this means that OSX stack logic is broken because it will
>>>>> currently be silently failing due to EINVAL!
>>>> Yes, that is correct.
>>>>
>>>> Chris
>>>>>
>>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>> have
>>>>>> a new test I'd like to add.
>>>>>
>>>>> Ok.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>> Ok, time for a new webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>> asserts
>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>> discuss
>>>>>>> already:
>>>>>>>
>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>> because we always align it to the page size.
>>>>>>>  - Any assert would therefore be a VM bug and not due to user 
>>>>>>> error.
>>>>>>>  - No fixing the java launcher. If the user specifies a stack 
>>>>>>> that is
>>>>>>> too small, hotspot will already detect this. If the user 
>>>>>>> specifies a
>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>
>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>> figure
>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>> down
>>>>>>> our testing resources.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> While working on 8175342 I noticed our stack size on xgene was 8mb
>>>>>>>> even though I was specifying -Xss72k. It turns out the following
>>>>>>>> code
>>>>>>>> was failing:
>>>>>>>>
>>>>>>>>       pthread_attr_setstacksize(&attr, stack_size);
>>>>>>>>
>>>>>>>> Although we computed a minimum stack size of 72k, so -Xss72k 
>>>>>>>> should
>>>>>>>> be fine, pthreads on this platform requires the stack be at least
>>>>>>>> 128k, so it failed the pthread_attr_setstacksize() call. The end
>>>>>>>> result is pthread_attr_setstacksize() had no impact on the 
>>>>>>>> thread's
>>>>>>>> stack size, and we ended up with the platform default of 8mb. The
>>>>>>>> fix
>>>>>>>> is to round up the following variables to PTHREAD_STACK_MIN after
>>>>>>>> computing their new values:
>>>>>>>>
>>>>>>>>       _java_thread_min_stack_allowed
>>>>>>>>       _compiler_thread_min_stack_allowed
>>>>>>>>       _vm_internal_thread_min_stack_allowed
>>>>>>>>
>>>>>>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You 
>>>>>>>> need to
>>>>>>>> #define _POSIX_C_SOURCE >= 199506L in order to get 
>>>>>>>> PTHREAD_STACK_MIN
>>>>>>>> #defined, and this needs to be done before including OS header
>>>>>>>> files.
>>>>>>>> I noticed that on solaris we were using thr_min_stack() elsewhere
>>>>>>>> instead of PTHREAD_STACK_MIN, so I decided to do the same with 
>>>>>>>> this
>>>>>>>> fix. Either way is ugly (the #define or using thr_min_stack()).
>>>>>>>>
>>>>>>>> And speaking of the existing use of thr_min_stack(), I deleted
>>>>>>>> it. It
>>>>>>>> was being applied before any adjustments to the stack sizes had 
>>>>>>>> been
>>>>>>>> made (rounding and adding red, yellow, and shadow zones). This 
>>>>>>>> mean
>>>>>>>> the stack ended up being larger than necessary. With the above
>>>>>>>> fix in
>>>>>>>> place, we are now applying thr_min_stack() after recomputing the
>>>>>>>> minimum stack sizes. If for any reason one of those stack sizes is
>>>>>>>> now too small, the correct fix is to adjust the initial stack 
>>>>>>>> sizes,
>>>>>>>> not apply thr_min_stack() to the initial stack sizes. However, it
>>>>>>>> looks like no adjustment is needed. I did something close to our
>>>>>>>> nightly testing on all affect platforms, and no new problems
>>>>>>>> turned up.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>