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

[Chris Plummer]

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.

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

     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.

I'll get another webrev out once I've made the needed fixes. I also have 
a new test I'd like to add.

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