(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads
Chris Plummer
chris.plummer at oracle.com
Thu Mar 16 17:49:49 UTC 2017
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.
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