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

Tue Mar 21 19:53:05 UTC 2017

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?

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