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

[David Holmes]

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

Given there is no error checking, or assertions, in those files I 
reluctantly have to say leave it out.

Thanks,
David
-----

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