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

[David Holmes]

On 16/03/2017 4:14 PM, Chris Plummer wrote:
> 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.

Oops! I was forgetting that. Need to be consistent with launcher error 
checking or lack thereof. And ignore refactoring comments - not relevant.

David

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