(RFR)(S)(10): 8176768: hotspot ignores PTHREAD_STACK_MIN when creating new threads (original) (raw)

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 ("STACKSIZEMINIMUM"), should we do the same fix (adjust for PTHREADSTACKMIN) 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 pthreadcreate() in the platform files, so the least we could do is add a warning log output case ppthreadattrsetstacksize 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. pthreadcreate 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 assertstatus. The launcher code doesn't do any error checking at all (but again pthreadcreate 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 pthreadsetstacksize() 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. pthreadsetstacksize could fail in this case, and there would be no "stack size too small" rejection from the hotspot. However, pthreadsetstacksize 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 oscpu 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::setminimumstacksizes()). Or, at least, rename "setminimumstacksizes" to something like "adjustminimumstacksizes" 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: pthreadattrsetstacksize(&attr, stacksize); 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 osposix.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 = pthreadmutexlock(mutex); assertstatus(status == 0, status, "mutexlock"); 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 osposix 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 pthreadattrsetstacksize() call. The end result is pthreadattrsetstacksize() 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 PTHREADSTACKMIN after computing their new values: javathreadminstackallowed compilerthreadminstackallowed vminternalthreadminstackallowed For solaris, there was an issue using PTHREADSTACKMIN. You need to #define POSIXCSOURCE >= 199506L in order to get PTHREADSTACKMIN #defined, and this needs to be done before including OS header files. I noticed that on solaris we were using thrminstack() elsewhere instead of PTHREADSTACKMIN, so I decided to do the same with this fix. Either way is ugly (the #define or using thrminstack()). And speaking of the existing use of thrminstack(), 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 thrminstack() 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 thrminstack() 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