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

[David Holmes]

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. :)

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