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

[Chris Plummer]

On 3/15/17 11:11 PM, David Holmes wrote:
> On 16/03/2017 3:51 PM, Chris Plummer wrote:
>> On 3/15/17 10:23 PM, David Holmes wrote:
>>> Hi Chris,
>>>
>>> On 16/03/2017 3: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
>>>
>>> Change looks good.
>>>
>>>> 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);
>>>
>>> So these really should be checking return values, at least in debug
>>> builds. But we can leave that until we refactor the thread startup
>>> code into os_posix.cpp.
>> I considered adding checks. I wasn't sure if we should abort or just
>> print a warning if it failed.
>
> When we check pthread lib routines we use:
>
>    int status = pthread_mutex_lock(_mutex);
>    assert_status(status == 0, status, "mutex_lock");
>
> This is for things that should only fail if we have a programming error.
Ok, but this is in the launcher, so I'll need to just use the built-in 
assert(). I'll add that if want.

Chris
>
>> What refactoring is planned?
>
> "Planned" might be a bit strong :) I was thinking of a number of 
> os_posix related cleanups for which issues exist, but also forgot that 
> some of our general clean-up RFE's have been closed as WNF :( I may do 
> some of them after hours anyway :)
>
> David
> -----
>
>> Chris
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> 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
>>
>>