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

[David Holmes]

On 18/03/2017 9:11 AM, Chris Plummer wrote:
> Looks like this will need some more work since the added asserts are
> triggering on mac os x (which is the only place we'd currently expect
> them to assert).
>
> The problem is that the code I found that rounds up to the page size is
> only applied to java threads created by the VM for which the java user
> specified no stack size. The VM and Compiler thread sizes are not
> rounded. The failure I saw was with
> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java when is
> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the assert
> with an EINVAL. The size is not aligned, but it could also be
> complaining because it is too big. I haven't tried aligning it yet to see.
>
> On Linux we do the following:
>
>   stack_size = align_size_up(stack_size +
> os::Linux::default_guard_size(thr_type), vm_page_size());
>
> We don't do this on BSD. I think the same on BSD would solve this
> problem. I'm not sure about adding the guard size. I'll need to see if
> mac os x has the same pthread bug as linux does.

At this stage I would only deal with alignment issues. IIRC the guard 
issue only affected Linux.

> BTW, did you know java users can specify the size of the a new thread's
> stack:

Yes I mentioned that in another reply - wondering whether we suitably 
check and aligned such requests.

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

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