(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads
David Holmes
david.holmes at oracle.com
Thu Mar 16 22:01:42 UTC 2017
On 17/03/2017 7:43 AM, Chris Plummer wrote:
> 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.
Perhaps because that is not specified by POSIX. For POSIX we only have:
[EINVAL]
The value of stacksize is less than {PTHREAD_STACK_MIN} or exceeds
a system-imposed limit.
Anyway that is a check that hotspot could perform if
pthread_attr_setstacksize fails. Though that then makes me wonder if we
do any rounding when the stack size set on a per thread basis via the
java.lang.Thread constructor?
I think imposing the PTHREAD_STACK_MIN in hotspot, with an assert
checking pthread_attr_setstacksize succeeded (in hotspot) would suffice
here.
David
-----
> 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