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

[Thomas Stüfe]

Hi Chris, David,

the change looks good.

I see that in the launcher we require a minimum stack size across all
platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust for
PTHREAD_STACK_MIN) there?

I do not understand, why does error checking in the hotspot have to be
consistent with the launcher? What prevents us from asserting in the
hotspot - or at least print a warning? Note that in the hotspot, there is
already UL logging ("os", "thread") after pthread_create() in the platform
files, so the least we could do is add a warning log output case
ppthread_attr_setstacksize
fails.

If we ever refactor this coding, could we rename the variables holding the
base stack size requirement for java frames - in all its incarnations in
all the os_cpu files - to be renamed to something different? It is a bit
confusing to have a variable which at different times in VM life means
different things (before and after the call to
os::Posix::set_minimum_stack_sizes()).
Or, at least, rename "set_minimum_stack_sizes" to something like
"adjust_minimum_stack_sizes" which makes the intent clearer.

Kind Regards, Thomas

On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <david.holmes at oracle.com>
wrote:

> 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/webr
>>>>>>>> ev.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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>