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

[Thomas Stüfe]

Hi Chris,

please find my answers inline.

On Fri, Mar 17, 2017 at 5:27 AM, Chris Plummer <chris.plummer at oracle.com>
wrote:

> Ok, time for a new webrev:
>
> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>
>
The changes look good.


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

As a side note, on AIX we have a complicated page scheme where thread
stacks may have a different page size from the java heap or from the
primordial thread. But using os::vm_page_size() should be ok, we ensure
that os::vm_page_size is always the same or a multiple of the thread stack
page size.

Would you mind adding the assert to os_aix.cpp too?


>  - 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
>
>
I am all fine with the changes.

Kind Regards, Thomas


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