(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads
David Holmes
david.holmes at oracle.com
Thu Mar 16 09:16:53 UTC 2017
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).
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