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

[Chris Plummer]

On 3/15/17 11:18 PM, David Holmes wrote:
> 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.
So don't add the error check?
>
> 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
>>>>
>>>>
>>