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

Tue Mar 21 20:53:19 UTC 2017

On 3/21/17 12:53 PM, Daniel D. Daugherty wrote:
> On 3/20/17 6: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?
>>
>> 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. :)
>
> Chris, would you mind checking with Kim Barrett on the
> advisability of doing it this way?
Ok.

Chris
>
> Dan
>
>
>>
>>> 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?
>>
>>> 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"
>>
>>>
>>> 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?
>>
>>   27  * @modules java.base/jdk.internal.misc
>>   28  * @library /test/lib
>>
>> The above are not needed by this test.
>>
>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>