(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads
Chris Plummer
chris.plummer at oracle.com
Thu Mar 16 21:43:58 UTC 2017
On 3/16/17 2:35 PM, David Holmes wrote:
> 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.
>
Close, except there is still a potential issue if the size is bigger
than the minimum hotspot requires, but is not page size aligned.
pthread_setstacksize *could* fail in this case, and there would be no
"stack size too small" rejection from the hotspot. However,
pthread_setstacksize did not fail on the two platforms I tried unaligned
stack sizes on.
Chris
> 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