(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads
David Holmes
david.holmes at oracle.com
Thu Mar 16 21:35:56 UTC 2017
On 17/03/2017 3:49 AM, Chris Plummer wrote:
> On 3/16/17 2:16 AM, David Holmes wrote:
>> On 16/03/2017 6:30 PM, Thomas Stüfe wrote:
>>> 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.
>>
>> Sorry I'm getting this group of bugs all muddled up.
>>
>> Chris: this issue does affect hotspot and the launcher (potentially).
>>
>> Ideally both should be checking for failures in the pthread calls but
>> neither do so. Hotspot at least does so in some places but not in a
>> lot of others.
>>
>> pthread_create is different in hotspot because failure can happen
>> easily and we need to detect it and report it (via an exception and
>> also via UL). The other pthread calls are not expected to fail under
>> "normal" conditions but only due to a programming error. Those calls
>> should at least be checked in debug builds as we already do in places
>> with assert_status.
>>
>> The launcher code doesn't do any error checking at all (but again
>> pthread_create is a special case).
> Are you just referring to the pthread related error checking? It does do
> other error checking.
pthread error checking.
So trying to think this through ...
If the user specifies a too small, or unaligned-to-page-size, -Xss
value the pthread_setstacksize() in the launcher will silently fail and
the main thread will get the default stack of 8M. It will then load the
VM which will then check the -Xss value, which will do its own validity
checking.
That seems like quite a reasonable position for the launcher to take.
David
-----
>
> Chris
>>
>> David
>> -----
>>
>>> 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
>>> <mailto: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
>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>> <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
>>>
>>>
>>>
>>>
>>>
>>>
>
>
More information about the hotspot-dev
mailing list