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

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 3/16/17 11:26 PM, David Holmes wrote:
> On 17/03/2017 2:27 PM, Chris Plummer wrote:
>> Ok, time for a new webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>
> Looks good. Full agreement from me on all the below.
Hi David,

Thanks for your review(s) and all your help and suggestions. Much
appreciated.

Chris

>
> Thanks,
> David
>
>> The only thing that has changed since the first webrev are the asserts
>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
>> already:
>>
>>  - The assert should never happen due to the stack size being too small
>> since it will be at least PTHREAD_STACK_MIN.
>>  - The assert should never happen due to an unaligned stack size because
>> we always align it to the page size.
>>  - Any assert would therefore be a VM bug and not due to user error.
>>  - No fixing the java launcher. If the user specifies a stack that is
>> too small, hotspot will already detect this. If the user specifies a
>> stack size that is large enough but not page aligned, then we just
>> ignore any error (if the platform doth protest) and the user gets a main
>> thread with a stack size set to whatever the OS default is.
>>
>> I still need to retest (I only ran TooSmallStackSize.java), but figure
>> getting agreement on the changes first would be best before I bog down
>> our testing resources.
>>
>> thanks,
>>
>> Chris
>>
>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>> Hello,
>>>
>>> Please review the following:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>
>>> 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);
>>>
>>> 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
>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
In reply to this post by Chris Plummer
Looks like this will need some more work since the added asserts are
triggering on mac os x (which is the only place we'd currently expect
them to assert).

The problem is that the code I found that rounds up to the page size is
only applied to java threads created by the VM for which the java user
specified no stack size. The VM and Compiler thread sizes are not
rounded. The failure I saw was with
runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java when is
specified -XX:CompilerThreadStackSize=9007199254740991. I hit the assert
with an EINVAL. The size is not aligned, but it could also be
complaining because it is too big. I haven't tried aligning it yet to see.

On Linux we do the following:

   stack_size = align_size_up(stack_size +
os::Linux::default_guard_size(thr_type), vm_page_size());

We don't do this on BSD. I think the same on BSD would solve this
problem. I'm not sure about adding the guard size. I'll need to see if
mac os x has the same pthread bug as linux does.

BTW, did you know java users can specify the size of the a new thread's
stack:

     public Thread(ThreadGroup group, Runnable target, String name,
                   long stackSize) {
         init(group, target, name, stackSize);
     }

Fortunately we already force the stackSize to be at least
_java_thread_min_stack_allowed. However, we don't do any OS page
rounding on Mac OS X as noted above, and I was able to trigger the
assert by creating a thread with size 257k.

I'll get another webrev out once I've made the needed fixes. I also have
a new test I'd like to add.

Chris

On 3/16/17 9:27 PM, Chris Plummer wrote:

> Ok, time for a new webrev:
>
> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>
> The only thing that has changed since the first webrev are the asserts
> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
> already:
>
>  - The assert should never happen due to the stack size being too
> small since it will be at least PTHREAD_STACK_MIN.
>  - The assert should never happen due to an unaligned stack size
> because we always align it to the page size.
>  - Any assert would therefore be a VM bug and not due to user error.
>  - No fixing the java launcher. If the user specifies a stack that is
> too small, hotspot will already detect this. If the user specifies a
> stack size that is large enough but not page aligned, then we just
> ignore any error (if the platform doth protest) and the user gets a
> main thread with a stack size set to whatever the OS default is.
>
> I still need to retest (I only ran TooSmallStackSize.java), but figure
> getting agreement on the changes first would be best before I bog down
> our testing resources.
>
> thanks,
>
> Chris
>
> On 3/15/17 10:03 PM, Chris Plummer wrote:
>> Hello,
>>
>> Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8176768
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>
>> 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);
>>
>> 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
>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 18/03/2017 9:11 AM, Chris Plummer wrote:

> Looks like this will need some more work since the added asserts are
> triggering on mac os x (which is the only place we'd currently expect
> them to assert).
>
> The problem is that the code I found that rounds up to the page size is
> only applied to java threads created by the VM for which the java user
> specified no stack size. The VM and Compiler thread sizes are not
> rounded. The failure I saw was with
> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java when is
> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the assert
> with an EINVAL. The size is not aligned, but it could also be
> complaining because it is too big. I haven't tried aligning it yet to see.
>
> On Linux we do the following:
>
>   stack_size = align_size_up(stack_size +
> os::Linux::default_guard_size(thr_type), vm_page_size());
>
> We don't do this on BSD. I think the same on BSD would solve this
> problem. I'm not sure about adding the guard size. I'll need to see if
> mac os x has the same pthread bug as linux does.

At this stage I would only deal with alignment issues. IIRC the guard
issue only affected Linux.

> BTW, did you know java users can specify the size of the a new thread's
> stack:

Yes I mentioned that in another reply - wondering whether we suitably
check and aligned such requests.

>     public Thread(ThreadGroup group, Runnable target, String name,
>                   long stackSize) {
>         init(group, target, name, stackSize);
>     }
>
> Fortunately we already force the stackSize to be at least
> _java_thread_min_stack_allowed. However, we don't do any OS page
> rounding on Mac OS X as noted above, and I was able to trigger the
> assert by creating a thread with size 257k.

Note this means that OSX stack logic is broken because it will currently
be silently failing due to EINVAL!

> I'll get another webrev out once I've made the needed fixes. I also have
> a new test I'd like to add.

Ok.

Thanks,
David

> Chris
>
> On 3/16/17 9:27 PM, Chris Plummer wrote:
>> Ok, time for a new webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>
>> The only thing that has changed since the first webrev are the asserts
>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
>> already:
>>
>>  - The assert should never happen due to the stack size being too
>> small since it will be at least PTHREAD_STACK_MIN.
>>  - The assert should never happen due to an unaligned stack size
>> because we always align it to the page size.
>>  - Any assert would therefore be a VM bug and not due to user error.
>>  - No fixing the java launcher. If the user specifies a stack that is
>> too small, hotspot will already detect this. If the user specifies a
>> stack size that is large enough but not page aligned, then we just
>> ignore any error (if the platform doth protest) and the user gets a
>> main thread with a stack size set to whatever the OS default is.
>>
>> I still need to retest (I only ran TooSmallStackSize.java), but figure
>> getting agreement on the changes first would be best before I bog down
>> our testing resources.
>>
>> thanks,
>>
>> Chris
>>
>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>> Hello,
>>>
>>> Please review the following:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>
>>> 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);
>>>
>>> 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
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 3/17/17 7:01 PM, David Holmes wrote:

> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>> Looks like this will need some more work since the added asserts are
>> triggering on mac os x (which is the only place we'd currently expect
>> them to assert).
>>
>> The problem is that the code I found that rounds up to the page size is
>> only applied to java threads created by the VM for which the java user
>> specified no stack size. The VM and Compiler thread sizes are not
>> rounded. The failure I saw was with
>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java when is
>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the assert
>> with an EINVAL. The size is not aligned, but it could also be
>> complaining because it is too big. I haven't tried aligning it yet to
>> see.
>>
>> On Linux we do the following:
>>
>>   stack_size = align_size_up(stack_size +
>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>
>> We don't do this on BSD. I think the same on BSD would solve this
>> problem. I'm not sure about adding the guard size. I'll need to see if
>> mac os x has the same pthread bug as linux does.
>
> At this stage I would only deal with alignment issues. IIRC the guard
> issue only affected Linux.
Yes, that's what I eventually concluded. I put the fix in
os::Posix::get_initial_stack_size() in os_posix.cpp, and only did the
page aligning, not add the guard page. That way all Posix ports are
fixed in one place. It seems to be working.
>
>> BTW, did you know java users can specify the size of the a new thread's
>> stack:
>
> Yes I mentioned that in another reply - wondering whether we suitably
> check and aligned such requests.
No we don't. Below I mentioned I was able to trigger the assert with a
257k stack size. I guess I wasn't clear that I did that from Java. I
have a new test to add that will be testing this, plus the
9007199254740991 stack size (which fails to create the thread with an
OOME, but that's acceptable). The fix I mention above in
os::Posix::get_initial_stack_size() takes care of this issue also.

>
>>     public Thread(ThreadGroup group, Runnable target, String name,
>>                   long stackSize) {
>>         init(group, target, name, stackSize);
>>     }
>>
>> Fortunately we already force the stackSize to be at least
>> _java_thread_min_stack_allowed. However, we don't do any OS page
>> rounding on Mac OS X as noted above, and I was able to trigger the
>> assert by creating a thread with size 257k.
>
> Note this means that OSX stack logic is broken because it will
> currently be silently failing due to EINVAL!
Yes, that is correct.

Chris

>
>> I'll get another webrev out once I've made the needed fixes. I also have
>> a new test I'd like to add.
>
> Ok.
>
> Thanks,
> David
>
>> Chris
>>
>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>> Ok, time for a new webrev:
>>>
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>>
>>> The only thing that has changed since the first webrev are the asserts
>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
>>> already:
>>>
>>>  - The assert should never happen due to the stack size being too
>>> small since it will be at least PTHREAD_STACK_MIN.
>>>  - The assert should never happen due to an unaligned stack size
>>> because we always align it to the page size.
>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>  - No fixing the java launcher. If the user specifies a stack that is
>>> too small, hotspot will already detect this. If the user specifies a
>>> stack size that is large enough but not page aligned, then we just
>>> ignore any error (if the platform doth protest) and the user gets a
>>> main thread with a stack size set to whatever the OS default is.
>>>
>>> I still need to retest (I only ran TooSmallStackSize.java), but figure
>>> getting agreement on the changes first would be best before I bog down
>>> our testing resources.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>> Hello,
>>>>
>>>> Please review the following:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>
>>>> 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);
>>>>
>>>> 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
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 3/17/17 8:17 PM, Chris Plummer wrote:

> On 3/17/17 7:01 PM, David Holmes wrote:
>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>> Looks like this will need some more work since the added asserts are
>>> triggering on mac os x (which is the only place we'd currently expect
>>> them to assert).
>>>
>>> The problem is that the code I found that rounds up to the page size is
>>> only applied to java threads created by the VM for which the java user
>>> specified no stack size. The VM and Compiler thread sizes are not
>>> rounded. The failure I saw was with
>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>> when is
>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>> assert
>>> with an EINVAL. The size is not aligned, but it could also be
>>> complaining because it is too big. I haven't tried aligning it yet
>>> to see.
>>>
>>> On Linux we do the following:
>>>
>>>   stack_size = align_size_up(stack_size +
>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>
>>> We don't do this on BSD. I think the same on BSD would solve this
>>> problem. I'm not sure about adding the guard size. I'll need to see if
>>> mac os x has the same pthread bug as linux does.
>>
>> At this stage I would only deal with alignment issues. IIRC the guard
>> issue only affected Linux.
> Yes, that's what I eventually concluded. I put the fix in
> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did the
> page aligning, not add the guard page. That way all Posix ports are
> fixed in one place. It seems to be working.
>>
>>> BTW, did you know java users can specify the size of the a new thread's
>>> stack:
>>
>> Yes I mentioned that in another reply - wondering whether we suitably
>> check and aligned such requests.
> No we don't. Below I mentioned I was able to trigger the assert with a
> 257k stack size. I guess I wasn't clear that I did that from Java. I
> have a new test to add that will be testing this, plus the
> 9007199254740991 stack size (which fails to create the thread with an
> OOME, but that's acceptable). The fix I mention above in
> os::Posix::get_initial_stack_size() takes care of this issue also.
Rounding up triggers a new assert, this time on 32-bit x86 and arm.

I should have clarified it's 9007199254740991 "K", which is
9223372036854774784. Unfortunately on 32bit systems that is asserting
with EINVAL. I think we need to do a better job of dealing with 32-bit
size_t values:

jlong java_lang_Thread::stackSize(oop java_thread) {
   if (_stackSize_offset > 0) {
     return java_thread->long_field(_stackSize_offset);
   } else {
     return 0;
   }
}

       jlong size =
java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
       // Allocate the C++ Thread structure and create the native
thread.  The
       // stack size retrieved from java is signed, but the constructor
takes
       // size_t (an unsigned type), so avoid passing negative values
which would
       // result in really large stacks.
       size_t sz = size > 0 ? (size_t) size : 0;
       native_thread = new JavaThread(&thread_entry, sz);

9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT),
which is 0xfffffc00 when cast to a size_t on a 32-bit system (close to
32-bit MAX_UINT). Round it up to the 4k page size and you get 0, which I
guess pthread_attr_setstacksize() doesn't like. So I think more
processing of the size is needed here. Maybe in os::create_thread() we
should check for 0 after rounding up, and subtract the os page size if
it is 0. However, I think we should also avoid truncating on 32-bit to
what is basically some random number. Maybe if "size" (a jlong) is
greater than UINT_MAX, then just set "sz" (a size_t) it to UINT_MAX.

Chris


>>
>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>                   long stackSize) {
>>>         init(group, target, name, stackSize);
>>>     }
>>>
>>> Fortunately we already force the stackSize to be at least
>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>> assert by creating a thread with size 257k.
>>
>> Note this means that OSX stack logic is broken because it will
>> currently be silently failing due to EINVAL!
> Yes, that is correct.
>
> Chris
>>
>>> I'll get another webrev out once I've made the needed fixes. I also
>>> have
>>> a new test I'd like to add.
>>
>> Ok.
>>
>> Thanks,
>> David
>>
>>> Chris
>>>
>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>> Ok, time for a new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>>>
>>>> The only thing that has changed since the first webrev are the asserts
>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
>>>> already:
>>>>
>>>>  - The assert should never happen due to the stack size being too
>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>  - The assert should never happen due to an unaligned stack size
>>>> because we always align it to the page size.
>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>  - No fixing the java launcher. If the user specifies a stack that is
>>>> too small, hotspot will already detect this. If the user specifies a
>>>> stack size that is large enough but not page aligned, then we just
>>>> ignore any error (if the platform doth protest) and the user gets a
>>>> main thread with a stack size set to whatever the OS default is.
>>>>
>>>> I still need to retest (I only ran TooSmallStackSize.java), but figure
>>>> getting agreement on the changes first would be best before I bog down
>>>> our testing resources.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>> Hello,
>>>>>
>>>>> Please review the following:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>
>>>>>
>>>>> 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);
>>>>>
>>>>> 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
>>>>
>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
Hi Chris,

On Sat, Mar 18, 2017 at 7:37 AM, Chris Plummer <[hidden email]>
wrote:

> On 3/17/17 8:17 PM, Chris Plummer wrote:
>
>> On 3/17/17 7:01 PM, David Holmes wrote:
>>
>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>
>>>> Looks like this will need some more work since the added asserts are
>>>> triggering on mac os x (which is the only place we'd currently expect
>>>> them to assert).
>>>>
>>>> The problem is that the code I found that rounds up to the page size is
>>>> only applied to java threads created by the VM for which the java user
>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>> rounded. The failure I saw was with
>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java when
>>>> is
>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>>> assert
>>>> with an EINVAL. The size is not aligned, but it could also be
>>>> complaining because it is too big. I haven't tried aligning it yet to
>>>> see.
>>>>
>>>> On Linux we do the following:
>>>>
>>>>   stack_size = align_size_up(stack_size +
>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>
>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>> problem. I'm not sure about adding the guard size. I'll need to see if
>>>> mac os x has the same pthread bug as linux does.
>>>>
>>>
>>> At this stage I would only deal with alignment issues. IIRC the guard
>>> issue only affected Linux.
>>>
>> Yes, that's what I eventually concluded. I put the fix in
>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did the
>> page aligning, not add the guard page. That way all Posix ports are fixed
>> in one place. It seems to be working.
>>
>>>
>>> BTW, did you know java users can specify the size of the a new thread's
>>>> stack:
>>>>
>>>
>>> Yes I mentioned that in another reply - wondering whether we suitably
>>> check and aligned such requests.
>>>
>> No we don't. Below I mentioned I was able to trigger the assert with a
>> 257k stack size. I guess I wasn't clear that I did that from Java. I have a
>> new test to add that will be testing this, plus the 9007199254740991 stack
>> size (which fails to create the thread with an OOME, but that's
>> acceptable). The fix I mention above in os::Posix::get_initial_stack_size()
>> takes care of this issue also.
>>
> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>
> I should have clarified it's 9007199254740991 "K", which is
> 9223372036854774784. Unfortunately on 32bit systems that is asserting with
> EINVAL. I think we need to do a better job of dealing with 32-bit size_t
> values:
>
> jlong java_lang_Thread::stackSize(oop java_thread) {
>   if (_stackSize_offset > 0) {
>     return java_thread->long_field(_stackSize_offset);
>   } else {
>     return 0;
>   }
> }
>
>       jlong size =
> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>       // Allocate the C++ Thread structure and create the native thread.
> The
>       // stack size retrieved from java is signed, but the constructor
> takes
>       // size_t (an unsigned type), so avoid passing negative values which
> would
>       // result in really large stacks.
>       size_t sz = size > 0 ? (size_t) size : 0;
>       native_thread = new JavaThread(&thread_entry, sz);
>
> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT), which
> is 0xfffffc00 when cast to a size_t on a 32-bit system (close to 32-bit
> MAX_UINT). Round it up to the 4k page size and you get 0, which I guess
> pthread_attr_setstacksize() doesn't like. So I think more processing of the
> size is needed here. Maybe in os::create_thread() we should check for 0
> after rounding up, and subtract the os page size if it is 0. However, I
> think we should also avoid truncating on 32-bit to what is basically some
> random number. Maybe if "size" (a jlong) is greater than UINT_MAX, then
> just set "sz" (a size_t) it to UINT_MAX.
>
> Chris
>
>
IMHO both solutions are fine: if cast to size_t would truncate the value,
either to cap the value to SIZE_MAX (to be portable, instead of UINT_MAX),
maybe minus one page size to forestall any rounding up. Then let the OS
fail with ENOMEM and handle that as usual. Alternatively fail immediately
with OOME, which will happen anyway, so we save some cycles.

Funny, I never get suspicious of (size_t) casts, because I am too used to
64bit. I did a quick grep for (size_t) casts in the hotspot sources, there
are quite a few. Probably overwhelmingly harmless, but it may be worth
checking them nevertheless.

... (took a cursory glance at the grep results, found one example of
casting jlong to size_t, see JVM_ENTRY
<http://ld8443:8080/source/s?defs=JVM_ENTRY&project=openjdk-jdk10-hs>(jlong
<http://ld8443:8080/source/s?defs=jlong&project=openjdk-jdk10-hs>,
jmm_SetPoolThreshold
<http://ld8443:8080/source/s?defs=jmm_SetPoolThreshold&project=openjdk-jdk10-hs>
(JNIEnv <http://ld8443:8080/source/s?defs=JNIEnv&project=openjdk-jdk10-hs>*
env
<http://ld8443:8080/source/xref/openjdk-jdk10-hs/hotspot/src/share/vm/services/management.cpp#env>,
jobject <http://ld8443:8080/source/s?defs=jobject&project=openjdk-jdk10-hs>
obj <http://ld8443:8080/source/s?defs=obj&project=openjdk-jdk10-hs>,
jmmThresholdType
<http://ld8443:8080/source/s?defs=jmmThresholdType&project=openjdk-jdk10-hs>
type <http://ld8443:8080/source/s?defs=type&project=openjdk-jdk10-hs>, jlong
<http://ld8443:8080/source/s?defs=jlong&project=openjdk-jdk10-hs> threshold
<http://ld8443:8080/source/s?defs=threshold&project=openjdk-jdk10-hs>)) in
management.cpp). So, its an issue.

Kind Regards, Thomas


>
>
>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>                   long stackSize) {
>>>>         init(group, target, name, stackSize);
>>>>     }
>>>>
>>>> Fortunately we already force the stackSize to be at least
>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>> assert by creating a thread with size 257k.
>>>>
>>>
>>> Note this means that OSX stack logic is broken because it will currently
>>> be silently failing due to EINVAL!
>>>
>> Yes, that is correct.
>>
>> Chris
>>
>>>
>>> I'll get another webrev out once I've made the needed fixes. I also have
>>>> a new test I'd like to add.
>>>>
>>>
>>> Ok.
>>>
>>> Thanks,
>>> David
>>>
>>> Chris
>>>>
>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>
>>>>> Ok, time for a new webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>>>>
>>>>> The only thing that has changed since the first webrev are the asserts
>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
>>>>> already:
>>>>>
>>>>>  - The assert should never happen due to the stack size being too
>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>  - The assert should never happen due to an unaligned stack size
>>>>> because we always align it to the page size.
>>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>>  - No fixing the java launcher. If the user specifies a stack that is
>>>>> too small, hotspot will already detect this. If the user specifies a
>>>>> stack size that is large enough but not page aligned, then we just
>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>
>>>>> I still need to retest (I only ran TooSmallStackSize.java), but figure
>>>>> getting agreement on the changes first would be best before I bog down
>>>>> our testing resources.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webr
>>>>>> ev.hotspot
>>>>>>
>>>>>> 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);
>>>>>>
>>>>>> 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
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
In reply to this post by Chris Plummer
On 3/17/17 11:37 PM, Chris Plummer wrote:

> On 3/17/17 8:17 PM, Chris Plummer wrote:
>> On 3/17/17 7:01 PM, David Holmes wrote:
>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>> Looks like this will need some more work since the added asserts are
>>>> triggering on mac os x (which is the only place we'd currently expect
>>>> them to assert).
>>>>
>>>> The problem is that the code I found that rounds up to the page
>>>> size is
>>>> only applied to java threads created by the VM for which the java user
>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>> rounded. The failure I saw was with
>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>> when is
>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>>> assert
>>>> with an EINVAL. The size is not aligned, but it could also be
>>>> complaining because it is too big. I haven't tried aligning it yet
>>>> to see.
>>>>
>>>> On Linux we do the following:
>>>>
>>>>   stack_size = align_size_up(stack_size +
>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>
>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>> problem. I'm not sure about adding the guard size. I'll need to see if
>>>> mac os x has the same pthread bug as linux does.
>>>
>>> At this stage I would only deal with alignment issues. IIRC the
>>> guard issue only affected Linux.
>> Yes, that's what I eventually concluded. I put the fix in
>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did the
>> page aligning, not add the guard page. That way all Posix ports are
>> fixed in one place. It seems to be working.
>>>
>>>> BTW, did you know java users can specify the size of the a new
>>>> thread's
>>>> stack:
>>>
>>> Yes I mentioned that in another reply - wondering whether we
>>> suitably check and aligned such requests.
>> No we don't. Below I mentioned I was able to trigger the assert with
>> a 257k stack size. I guess I wasn't clear that I did that from Java.
>> I have a new test to add that will be testing this, plus the
>> 9007199254740991 stack size (which fails to create the thread with an
>> OOME, but that's acceptable). The fix I mention above in
>> os::Posix::get_initial_stack_size() takes care of this issue also.
> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>
> I should have clarified it's 9007199254740991 "K", which is
> 9223372036854774784. Unfortunately on 32bit systems that is asserting
> with EINVAL. I think we need to do a better job of dealing with 32-bit
> size_t values:
>
> jlong java_lang_Thread::stackSize(oop java_thread) {
>   if (_stackSize_offset > 0) {
>     return java_thread->long_field(_stackSize_offset);
>   } else {
>     return 0;
>   }
> }
>
>       jlong size =
> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>       // Allocate the C++ Thread structure and create the native
> thread.  The
>       // stack size retrieved from java is signed, but the constructor
> takes
>       // size_t (an unsigned type), so avoid passing negative values
> which would
>       // result in really large stacks.
>       size_t sz = size > 0 ? (size_t) size : 0;
>       native_thread = new JavaThread(&thread_entry, sz);
>
> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT),
> which is 0xfffffc00 when cast to a size_t on a 32-bit system (close to
> 32-bit MAX_UINT). Round it up to the 4k page size and you get 0, which
> I guess pthread_attr_setstacksize() doesn't like. So I think more
> processing of the size is needed here. Maybe in os::create_thread() we
> should check for 0 after rounding up, and subtract the os page size if
> it is 0. However, I think we should also avoid truncating on 32-bit to
> what is basically some random number. Maybe if "size" (a jlong) is
> greater than UINT_MAX, then just set "sz" (a size_t) it to UINT_MAX.
>
Ok, I think I have this all worked out now. I've added fixes for
unaligned stack sizes, 32-bit truncating of stack size, and the
"aligning up to 0" problem. I also added a test. Here's the latest webrev:

http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot

Here's what's changed since webrev.01:

os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
stack size to be paged aligned. This fixes issues on Mac OS X (other
platforms seem to be immune to this). Then check if the size is zero
after rounding up to the page size. Subtract the page size in this case
to produce the maximum stack size allowed. Surprisingly I got no
complaint from gcc for subtracting from an unsigned value that is known
to be 0.

os_linux.cpp: In os::create_thread(), I also check here to make sure the
size is not 0 after adding the guard page and aligning up, and subtract
the os page size if it is 0.

jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
rounded up to 0 in os::Posix::get_initial_stack_size(), which will
result in subtracting the os page size to get the actual maximum allowed
stack size.

TooSmallStackSize.java: added test case for unaligned stack sizes.

TestThreadStackSizes.java: New test. Creates new threads with every size
up to 320k in 1k increments. Then creates threads with a few other sizes
that can be problematic.

thanks,

Chris

> Chris
>
>
>>>
>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>                   long stackSize) {
>>>>         init(group, target, name, stackSize);
>>>>     }
>>>>
>>>> Fortunately we already force the stackSize to be at least
>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>> assert by creating a thread with size 257k.
>>>
>>> Note this means that OSX stack logic is broken because it will
>>> currently be silently failing due to EINVAL!
>> Yes, that is correct.
>>
>> Chris
>>>
>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>> have
>>>> a new test I'd like to add.
>>>
>>> Ok.
>>>
>>> Thanks,
>>> David
>>>
>>>> Chris
>>>>
>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>> Ok, time for a new webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>
>>>>>
>>>>> The only thing that has changed since the first webrev are the
>>>>> asserts
>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>> discuss
>>>>> already:
>>>>>
>>>>>  - The assert should never happen due to the stack size being too
>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>  - The assert should never happen due to an unaligned stack size
>>>>> because we always align it to the page size.
>>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>>  - No fixing the java launcher. If the user specifies a stack that is
>>>>> too small, hotspot will already detect this. If the user specifies a
>>>>> stack size that is large enough but not page aligned, then we just
>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>
>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>> figure
>>>>> getting agreement on the changes first would be best before I bog
>>>>> down
>>>>> our testing resources.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>
>>>>>>
>>>>>> 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);
>>>>>>
>>>>>> 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
>>>>>
>>>>>
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Chris,

<trimming>

Getting closer :)

Have you seen pthread_attr_setstacksize return EINVAL on very large
values? Or does pthread_create just give a ENOMEM?

On 21/03/2017 9:29 AM, Chris Plummer wrote:

> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>
> Here's what's changed since webrev.01:
>
> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
> stack size to be paged aligned. This fixes issues on Mac OS X (other
> platforms seem to be immune to this). Then check if the size is zero
> after rounding up to the page size. Subtract the page size in this case
> to produce the maximum stack size allowed. Surprisingly I got no
> complaint from gcc for subtracting from an unsigned value that is known
> to be 0.

I'm a little surprised at that too. :)

> os_linux.cpp: In os::create_thread(), I also check here to make sure the
> size is not 0 after adding the guard page and aligning up, and subtract
> the os page size if it is 0.

What if adding the guard page and rounding up causes overflow so that we
get a very small positive stack size?

> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
> result in subtracting the os page size to get the actual maximum allowed
> stack size.

Good catch!

Nit: "-Avoid"  -> "- Avoid"

>
> TooSmallStackSize.java: added test case for unaligned stack sizes.

Ok.

> TestThreadStackSizes.java: New test. Creates new threads with every size
> up to 320k in 1k increments. Then creates threads with a few other sizes
> that can be problematic.

So this test never actually fails as long as the VM doesn't crash - is
that right?

   27  * @modules java.base/jdk.internal.misc
   28  * @library /test/lib

The above are not needed by this test.

Thanks,
David

> thanks,
>
> Chris
>> Chris
>>
>>
>>>>
>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>                   long stackSize) {
>>>>>         init(group, target, name, stackSize);
>>>>>     }
>>>>>
>>>>> Fortunately we already force the stackSize to be at least
>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>> assert by creating a thread with size 257k.
>>>>
>>>> Note this means that OSX stack logic is broken because it will
>>>> currently be silently failing due to EINVAL!
>>> Yes, that is correct.
>>>
>>> Chris
>>>>
>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>> have
>>>>> a new test I'd like to add.
>>>>
>>>> Ok.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Chris
>>>>>
>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>> Ok, time for a new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>>>>>
>>>>>>
>>>>>> The only thing that has changed since the first webrev are the
>>>>>> asserts
>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>> discuss
>>>>>> already:
>>>>>>
>>>>>>  - The assert should never happen due to the stack size being too
>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>> because we always align it to the page size.
>>>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>>>  - No fixing the java launcher. If the user specifies a stack that is
>>>>>> too small, hotspot will already detect this. If the user specifies a
>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>
>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>> figure
>>>>>> getting agreement on the changes first would be best before I bog
>>>>>> down
>>>>>> our testing resources.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>>>>
>>>>>>>
>>>>>>> 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);
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
Hi David,

On 3/20/17 5:59 PM, David Holmes wrote:
> Hi Chris,
>
> <trimming>
>
> Getting closer :)
>
> Have you seen pthread_attr_setstacksize return EINVAL on very large
> values? Or does pthread_create just give a ENOMEM?
It gives EAGAIN. The log for the new test contains:

[0.416s][warning][os,thread] Failed to start thread - pthread_create
failed (EAGAIN) for attributes: stacksize: 9007199254740992k, guardsize:
0k, detached.
Got exception for stack size 9223372036854774784:
java.lang.OutOfMemoryError: unable to create native thread: possibly out
of memory or process/resource limits reached

I assume it's also throwing OOME, which the test is catching and
(correctly) ignoring.

>
> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>
>> Here's what's changed since webrev.01:
>>
>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>> platforms seem to be immune to this). Then check if the size is zero
>> after rounding up to the page size. Subtract the page size in this case
>> to produce the maximum stack size allowed. Surprisingly I got no
>> complaint from gcc for subtracting from an unsigned value that is known
>> to be 0.
>
> I'm a little surprised at that too. :)
>
>> os_linux.cpp: In os::create_thread(), I also check here to make sure the
>> size is not 0 after adding the guard page and aligning up, and subtract
>> the os page size if it is 0.
>
> What if adding the guard page and rounding up causes overflow so that
> we get a very small positive stack size?
We already know the stack size is os page aligned up before getting
here, so actually we could just add the guard page and not align up. You
would get the same result.

>
>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>> result in subtracting the os page size to get the actual maximum allowed
>> stack size.
>
> Good catch!
>
> Nit: "-Avoid"  -> "- Avoid"
Ok.

>
>>
>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>
> Ok.
>
>> TestThreadStackSizes.java: New test. Creates new threads with every size
>> up to 320k in 1k increments. Then creates threads with a few other sizes
>> that can be problematic.
>
> So this test never actually fails as long as the VM doesn't crash - is
> that right?
Yes.
>
>   27  * @modules java.base/jdk.internal.misc
>   28  * @library /test/lib
>
> The above are not needed by this test.
Ok.

thanks,

Chris

>
> Thanks,
> David
>
>> thanks,
>>
>> Chris
>>> Chris
>>>
>>>
>>>>>
>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>                   long stackSize) {
>>>>>>         init(group, target, name, stackSize);
>>>>>>     }
>>>>>>
>>>>>> Fortunately we already force the stackSize to be at least
>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>> assert by creating a thread with size 257k.
>>>>>
>>>>> Note this means that OSX stack logic is broken because it will
>>>>> currently be silently failing due to EINVAL!
>>>> Yes, that is correct.
>>>>
>>>> Chris
>>>>>
>>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>> have
>>>>>> a new test I'd like to add.
>>>>>
>>>>> Ok.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>> Ok, time for a new webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>> asserts
>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>> discuss
>>>>>>> already:
>>>>>>>
>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>> because we always align it to the page size.
>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>> error.
>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>> that is
>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>> specifies a
>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>
>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>> figure
>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>> down
>>>>>>> our testing resources.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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);
>>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 3/20/17 6:25 PM, Chris Plummer wrote:

> Hi David,
>
> On 3/20/17 5:59 PM, David Holmes wrote:
>> Hi Chris,
>>
>> <trimming>
>>
>> Getting closer :)
>>
>> Have you seen pthread_attr_setstacksize return EINVAL on very large
>> values? Or does pthread_create just give a ENOMEM?
> It gives EAGAIN. The log for the new test contains:
>
> [0.416s][warning][os,thread] Failed to start thread - pthread_create
> failed (EAGAIN) for attributes: stacksize: 9007199254740992k,
> guardsize: 0k, detached.
> Got exception for stack size 9223372036854774784:
> java.lang.OutOfMemoryError: unable to create native thread: possibly
> out of memory or process/resource limits reached
>
> I assume it's also throwing OOME, which the test is catching and
> (correctly) ignoring.
Obviously it is getting the OOME because the test output above is
telling us as much.

Chris

>
>>
>> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>>
>>> Here's what's changed since webrev.01:
>>>
>>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up
>>> the
>>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>>> platforms seem to be immune to this). Then check if the size is zero
>>> after rounding up to the page size. Subtract the page size in this case
>>> to produce the maximum stack size allowed. Surprisingly I got no
>>> complaint from gcc for subtracting from an unsigned value that is known
>>> to be 0.
>>
>> I'm a little surprised at that too. :)
>>
>>> os_linux.cpp: In os::create_thread(), I also check here to make sure
>>> the
>>> size is not 0 after adding the guard page and aligning up, and subtract
>>> the os page size if it is 0.
>>
>> What if adding the guard page and rounding up causes overflow so that
>> we get a very small positive stack size?
> We already know the stack size is os page aligned up before getting
> here, so actually we could just add the guard page and not align up.
> You would get the same result.
>>
>>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>>> result in subtracting the os page size to get the actual maximum
>>> allowed
>>> stack size.
>>
>> Good catch!
>>
>> Nit: "-Avoid"  -> "- Avoid"
> Ok.
>>
>>>
>>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>
>> Ok.
>>
>>> TestThreadStackSizes.java: New test. Creates new threads with every
>>> size
>>> up to 320k in 1k increments. Then creates threads with a few other
>>> sizes
>>> that can be problematic.
>>
>> So this test never actually fails as long as the VM doesn't crash -
>> is that right?
> Yes.
>>
>>   27  * @modules java.base/jdk.internal.misc
>>   28  * @library /test/lib
>>
>> The above are not needed by this test.
> Ok.
>
> thanks,
>
> Chris
>>
>> Thanks,
>> David
>>
>>> thanks,
>>>
>>> Chris
>>>> Chris
>>>>
>>>>
>>>>>>
>>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>>                   long stackSize) {
>>>>>>>         init(group, target, name, stackSize);
>>>>>>>     }
>>>>>>>
>>>>>>> Fortunately we already force the stackSize to be at least
>>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>>> assert by creating a thread with size 257k.
>>>>>>
>>>>>> Note this means that OSX stack logic is broken because it will
>>>>>> currently be silently failing due to EINVAL!
>>>>> Yes, that is correct.
>>>>>
>>>>> Chris
>>>>>>
>>>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>>> have
>>>>>>> a new test I'd like to add.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>>> Ok, time for a new webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>>> asserts
>>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>>> discuss
>>>>>>>> already:
>>>>>>>>
>>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>>> because we always align it to the page size.
>>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>>> error.
>>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>>> that is
>>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>>> specifies a
>>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>>> ignore any error (if the platform doth protest) and the user
>>>>>>>> gets a
>>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>>
>>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>>> figure
>>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>>> down
>>>>>>>> our testing resources.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please review the following:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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);
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Chris Plummer
On 21/03/2017 11:25 AM, Chris Plummer wrote:

> Hi David,
>
> On 3/20/17 5:59 PM, David Holmes wrote:
>> Hi Chris,
>>
>> <trimming>
>>
>> Getting closer :)
>>
>> Have you seen pthread_attr_setstacksize return EINVAL on very large
>> values? Or does pthread_create just give a ENOMEM?
> It gives EAGAIN. The log for the new test contains:
>
> [0.416s][warning][os,thread] Failed to start thread - pthread_create
> failed (EAGAIN) for attributes: stacksize: 9007199254740992k, guardsize:
> 0k, detached.
> Got exception for stack size 9223372036854774784:
> java.lang.OutOfMemoryError: unable to create native thread: possibly out
> of memory or process/resource limits reached
>
> I assume it's also throwing OOME, which the test is catching and
> (correctly) ignoring.

Yep that is all good. I was just wondering of pthread_attr_setstacksize
did any sanity checking of big values and returned EINVAL - but it seems
not.

>>
>> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>>
>>> Here's what's changed since webrev.01:
>>>
>>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
>>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>>> platforms seem to be immune to this). Then check if the size is zero
>>> after rounding up to the page size. Subtract the page size in this case
>>> to produce the maximum stack size allowed. Surprisingly I got no
>>> complaint from gcc for subtracting from an unsigned value that is known
>>> to be 0.
>>
>> I'm a little surprised at that too. :)
>>
>>> os_linux.cpp: In os::create_thread(), I also check here to make sure the
>>> size is not 0 after adding the guard page and aligning up, and subtract
>>> the os page size if it is 0.
>>
>> What if adding the guard page and rounding up causes overflow so that
>> we get a very small positive stack size?
> We already know the stack size is os page aligned up before getting
> here, so actually we could just add the guard page and not align up. You
> would get the same result.

So we can get rid of the align_up here:

727   stack_size = align_size_up(stack_size +
os::Linux::default_guard_size(thr_type), vm_page_size());

But are we assuming the guard page is always one virtual-memory page? Or
put another way: what is the difference between page_size() and
vm_page_size() ?

Thanks,
David
-----

>>
>>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>>> result in subtracting the os page size to get the actual maximum allowed
>>> stack size.
>>
>> Good catch!
>>
>> Nit: "-Avoid"  -> "- Avoid"
> Ok.
>>
>>>
>>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>
>> Ok.
>>
>>> TestThreadStackSizes.java: New test. Creates new threads with every size
>>> up to 320k in 1k increments. Then creates threads with a few other sizes
>>> that can be problematic.
>>
>> So this test never actually fails as long as the VM doesn't crash - is
>> that right?
> Yes.
>>
>>   27  * @modules java.base/jdk.internal.misc
>>   28  * @library /test/lib
>>
>> The above are not needed by this test.
> Ok.
>
> thanks,
>
> Chris
>>
>> Thanks,
>> David
>>
>>> thanks,
>>>
>>> Chris
>>>> Chris
>>>>
>>>>
>>>>>>
>>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>>                   long stackSize) {
>>>>>>>         init(group, target, name, stackSize);
>>>>>>>     }
>>>>>>>
>>>>>>> Fortunately we already force the stackSize to be at least
>>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>>> assert by creating a thread with size 257k.
>>>>>>
>>>>>> Note this means that OSX stack logic is broken because it will
>>>>>> currently be silently failing due to EINVAL!
>>>>> Yes, that is correct.
>>>>>
>>>>> Chris
>>>>>>
>>>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>>> have
>>>>>>> a new test I'd like to add.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>>> Ok, time for a new webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>>> asserts
>>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>>> discuss
>>>>>>>> already:
>>>>>>>>
>>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>>> because we always align it to the page size.
>>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>>> error.
>>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>>> that is
>>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>>> specifies a
>>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>>
>>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>>> figure
>>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>>> down
>>>>>>>> our testing resources.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please review the following:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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);
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 3/20/17 6:45 PM, David Holmes wrote:

> On 21/03/2017 11:25 AM, Chris Plummer wrote:
>> Hi David,
>>
>> On 3/20/17 5:59 PM, David Holmes wrote:
>>> Hi Chris,
>>>
>>> <trimming>
>>>
>>> Getting closer :)
>>>
>>> Have you seen pthread_attr_setstacksize return EINVAL on very large
>>> values? Or does pthread_create just give a ENOMEM?
>> It gives EAGAIN. The log for the new test contains:
>>
>> [0.416s][warning][os,thread] Failed to start thread - pthread_create
>> failed (EAGAIN) for attributes: stacksize: 9007199254740992k, guardsize:
>> 0k, detached.
>> Got exception for stack size 9223372036854774784:
>> java.lang.OutOfMemoryError: unable to create native thread: possibly out
>> of memory or process/resource limits reached
>>
>> I assume it's also throwing OOME, which the test is catching and
>> (correctly) ignoring.
>
> Yep that is all good. I was just wondering of
> pthread_attr_setstacksize did any sanity checking of big values and
> returned EINVAL - but it seems not.
>
>>>
>>> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>>>
>>>> Here's what's changed since webrev.01:
>>>>
>>>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round
>>>> up the
>>>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>>>> platforms seem to be immune to this). Then check if the size is zero
>>>> after rounding up to the page size. Subtract the page size in this
>>>> case
>>>> to produce the maximum stack size allowed. Surprisingly I got no
>>>> complaint from gcc for subtracting from an unsigned value that is
>>>> known
>>>> to be 0.
>>>
>>> I'm a little surprised at that too. :)
>>>
>>>> os_linux.cpp: In os::create_thread(), I also check here to make
>>>> sure the
>>>> size is not 0 after adding the guard page and aligning up, and
>>>> subtract
>>>> the os page size if it is 0.
>>>
>>> What if adding the guard page and rounding up causes overflow so that
>>> we get a very small positive stack size?
>> We already know the stack size is os page aligned up before getting
>> here, so actually we could just add the guard page and not align up. You
>> would get the same result.
>
> So we can get rid of the align_up here:
>
> 727   stack_size = align_size_up(stack_size +
> os::Linux::default_guard_size(thr_type), vm_page_size());
I think so. I'm going to try the following:

   stack_size += os::Linux::default_guard_size(thr_type);
   assert(is_size_aligned(stack_size, os::vm_page_size()), "stack_size
not aligned");

>
> But are we assuming the guard page is always one virtual-memory page?
Yes.

size_t os::Linux::default_guard_size(os::ThreadType thr_type) {
   // Creating guard page is very expensive. Java thread has HotSpot
   // guard pages, only enable glibc guard page for non-Java threads.
   // (Remember: compiler thread is a Java thread, too!)
   return ((thr_type == java_thread || thr_type == compiler_thread) ? 0
: page_size());
}

> Or put another way: what is the difference between page_size() and
> vm_page_size() ?
They are the same:

   static int page_size(void) { return _page_size; }
   static void set_page_size(int val) { _page_size = val; }
   Linux::set_page_size(sysconf(_SC_PAGESIZE));

int os::vm_page_size() {
   // Seems redundant as all get out
   assert(os::Linux::page_size() != -1, "must call os::init");
   return os::Linux::page_size();
}

I'm not sure why we have both.

thanks,

Chris

>
> Thanks,
> David
> -----
>
>>>
>>>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is
>>>> greater
>>>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>>>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>>>> result in subtracting the os page size to get the actual maximum
>>>> allowed
>>>> stack size.
>>>
>>> Good catch!
>>>
>>> Nit: "-Avoid"  -> "- Avoid"
>> Ok.
>>>
>>>>
>>>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>>
>>> Ok.
>>>
>>>> TestThreadStackSizes.java: New test. Creates new threads with every
>>>> size
>>>> up to 320k in 1k increments. Then creates threads with a few other
>>>> sizes
>>>> that can be problematic.
>>>
>>> So this test never actually fails as long as the VM doesn't crash - is
>>> that right?
>> Yes.
>>>
>>>   27  * @modules java.base/jdk.internal.misc
>>>   28  * @library /test/lib
>>>
>>> The above are not needed by this test.
>> Ok.
>>
>> thanks,
>>
>> Chris
>>>
>>> Thanks,
>>> David
>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>> Chris
>>>>>
>>>>>
>>>>>>>
>>>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>>>                   long stackSize) {
>>>>>>>>         init(group, target, name, stackSize);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> Fortunately we already force the stackSize to be at least
>>>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>>>> assert by creating a thread with size 257k.
>>>>>>>
>>>>>>> Note this means that OSX stack logic is broken because it will
>>>>>>> currently be silently failing due to EINVAL!
>>>>>> Yes, that is correct.
>>>>>>
>>>>>> Chris
>>>>>>>
>>>>>>>> I'll get another webrev out once I've made the needed fixes. I
>>>>>>>> also
>>>>>>>> have
>>>>>>>> a new test I'd like to add.
>>>>>>>
>>>>>>> Ok.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>>>> Ok, time for a new webrev:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>>>> asserts
>>>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>>>> discuss
>>>>>>>>> already:
>>>>>>>>>
>>>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>>>> because we always align it to the page size.
>>>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>>>> error.
>>>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>>>> that is
>>>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>>>> specifies a
>>>>>>>>> stack size that is large enough but not page aligned, then we
>>>>>>>>> just
>>>>>>>>> ignore any error (if the platform doth protest) and the user
>>>>>>>>> gets a
>>>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>>>
>>>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>>>> figure
>>>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>>>> down
>>>>>>>>> our testing resources.
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Please review the following:
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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);
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 21/03/2017 12:35 PM, Chris Plummer wrote:

> On 3/20/17 6:45 PM, David Holmes wrote:
>> On 21/03/2017 11:25 AM, Chris Plummer wrote:
>>> Hi David,
>>>
>>> On 3/20/17 5:59 PM, David Holmes wrote:
>>>> Hi Chris,
>>>>
>>>> <trimming>
>>>>
>>>> Getting closer :)
>>>>
>>>> Have you seen pthread_attr_setstacksize return EINVAL on very large
>>>> values? Or does pthread_create just give a ENOMEM?
>>> It gives EAGAIN. The log for the new test contains:
>>>
>>> [0.416s][warning][os,thread] Failed to start thread - pthread_create
>>> failed (EAGAIN) for attributes: stacksize: 9007199254740992k, guardsize:
>>> 0k, detached.
>>> Got exception for stack size 9223372036854774784:
>>> java.lang.OutOfMemoryError: unable to create native thread: possibly out
>>> of memory or process/resource limits reached
>>>
>>> I assume it's also throwing OOME, which the test is catching and
>>> (correctly) ignoring.
>>
>> Yep that is all good. I was just wondering of
>> pthread_attr_setstacksize did any sanity checking of big values and
>> returned EINVAL - but it seems not.
>>
>>>>
>>>> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>>>>
>>>>> Here's what's changed since webrev.01:
>>>>>
>>>>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round
>>>>> up the
>>>>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>>>>> platforms seem to be immune to this). Then check if the size is zero
>>>>> after rounding up to the page size. Subtract the page size in this
>>>>> case
>>>>> to produce the maximum stack size allowed. Surprisingly I got no
>>>>> complaint from gcc for subtracting from an unsigned value that is
>>>>> known
>>>>> to be 0.
>>>>
>>>> I'm a little surprised at that too. :)
>>>>
>>>>> os_linux.cpp: In os::create_thread(), I also check here to make
>>>>> sure the
>>>>> size is not 0 after adding the guard page and aligning up, and
>>>>> subtract
>>>>> the os page size if it is 0.
>>>>
>>>> What if adding the guard page and rounding up causes overflow so that
>>>> we get a very small positive stack size?
>>> We already know the stack size is os page aligned up before getting
>>> here, so actually we could just add the guard page and not align up. You
>>> would get the same result.
>>
>> So we can get rid of the align_up here:
>>
>> 727   stack_size = align_size_up(stack_size +
>> os::Linux::default_guard_size(thr_type), vm_page_size());
> I think so. I'm going to try the following:
>
>   stack_size += os::Linux::default_guard_size(thr_type);
>   assert(is_size_aligned(stack_size, os::vm_page_size()), "stack_size
> not aligned");

Looks good.

>>
>> But are we assuming the guard page is always one virtual-memory page?
> Yes.
>
> size_t os::Linux::default_guard_size(os::ThreadType thr_type) {
>   // Creating guard page is very expensive. Java thread has HotSpot
>   // guard pages, only enable glibc guard page for non-Java threads.
>   // (Remember: compiler thread is a Java thread, too!)
>   return ((thr_type == java_thread || thr_type == compiler_thread) ? 0 :
> page_size());
> }
>
>> Or put another way: what is the difference between page_size() and
>> vm_page_size() ?
> They are the same:
>
>   static int page_size(void) { return _page_size; }
>   static void set_page_size(int val) { _page_size = val; }
>   Linux::set_page_size(sysconf(_SC_PAGESIZE));
>
> int os::vm_page_size() {
>   // Seems redundant as all get out
>   assert(os::Linux::page_size() != -1, "must call os::init");
>   return os::Linux::page_size();
> }
>
> I'm not sure why we have both.

Okay. Yeah no reason to actually define os::Linux::page_size() when we
could just have a static _page_size variable in os_linux.cpp outside any
class.

Thanks,
David
-----

> thanks,
>
> Chris
>>
>> Thanks,
>> David
>> -----
>>
>>>>
>>>>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is
>>>>> greater
>>>>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>>>>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>>>>> result in subtracting the os page size to get the actual maximum
>>>>> allowed
>>>>> stack size.
>>>>
>>>> Good catch!
>>>>
>>>> Nit: "-Avoid"  -> "- Avoid"
>>> Ok.
>>>>
>>>>>
>>>>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>>>
>>>> Ok.
>>>>
>>>>> TestThreadStackSizes.java: New test. Creates new threads with every
>>>>> size
>>>>> up to 320k in 1k increments. Then creates threads with a few other
>>>>> sizes
>>>>> that can be problematic.
>>>>
>>>> So this test never actually fails as long as the VM doesn't crash - is
>>>> that right?
>>> Yes.
>>>>
>>>>   27  * @modules java.base/jdk.internal.misc
>>>>   28  * @library /test/lib
>>>>
>>>> The above are not needed by this test.
>>> Ok.
>>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>> Chris
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>>>>                   long stackSize) {
>>>>>>>>>         init(group, target, name, stackSize);
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>> Fortunately we already force the stackSize to be at least
>>>>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>>>>> assert by creating a thread with size 257k.
>>>>>>>>
>>>>>>>> Note this means that OSX stack logic is broken because it will
>>>>>>>> currently be silently failing due to EINVAL!
>>>>>>> Yes, that is correct.
>>>>>>>
>>>>>>> Chris
>>>>>>>>
>>>>>>>>> I'll get another webrev out once I've made the needed fixes. I
>>>>>>>>> also
>>>>>>>>> have
>>>>>>>>> a new test I'd like to add.
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>>>>> Ok, time for a new webrev:
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>>>>> asserts
>>>>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>>>>> discuss
>>>>>>>>>> already:
>>>>>>>>>>
>>>>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>>>>> because we always align it to the page size.
>>>>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>>>>> error.
>>>>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>>>>> that is
>>>>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>>>>> specifies a
>>>>>>>>>> stack size that is large enough but not page aligned, then we
>>>>>>>>>> just
>>>>>>>>>> ignore any error (if the platform doth protest) and the user
>>>>>>>>>> gets a
>>>>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>>>>
>>>>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>>>>> figure
>>>>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>>>>> down
>>>>>>>>>> our testing resources.
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>>
>>>>>>>>>> Chris
>>>>>>>>>>
>>>>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Please review the following:
>>>>>>>>>>>
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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);
>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
In reply to this post by Chris Plummer
Hi Chris,

looks fine to me.

small nit: os_linux_cpp:

+  // In that cast just subract the page size to get the maximum possible
stack size.

subract->substract

I would have preferred SIZE_MAX instead of UINT_MAX, but both is fine to me.

--

As for the size_overflow problem, I wonder whether we are overthinking
this. Maybe a more pragmatic approach would have been to just define a
reasonable upper maximum for stack sizes which could be well below
UINT_MAX. Somehow I cannot see users wanting to start threads with more
than ~4GB of thread stack size.

Kind Regards, Thomas



On Tue, Mar 21, 2017 at 12:29 AM, Chris Plummer <[hidden email]>
wrote:

> On 3/17/17 11:37 PM, Chris Plummer wrote:
>
>> On 3/17/17 8:17 PM, Chris Plummer wrote:
>>
>>> On 3/17/17 7:01 PM, David Holmes wrote:
>>>
>>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>>
>>>>> Looks like this will need some more work since the added asserts are
>>>>> triggering on mac os x (which is the only place we'd currently expect
>>>>> them to assert).
>>>>>
>>>>> The problem is that the code I found that rounds up to the page size is
>>>>> only applied to java threads created by the VM for which the java user
>>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>>> rounded. The failure I saw was with
>>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java when
>>>>> is
>>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>>>> assert
>>>>> with an EINVAL. The size is not aligned, but it could also be
>>>>> complaining because it is too big. I haven't tried aligning it yet to
>>>>> see.
>>>>>
>>>>> On Linux we do the following:
>>>>>
>>>>>   stack_size = align_size_up(stack_size +
>>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>>
>>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>>> problem. I'm not sure about adding the guard size. I'll need to see if
>>>>> mac os x has the same pthread bug as linux does.
>>>>>
>>>>
>>>> At this stage I would only deal with alignment issues. IIRC the guard
>>>> issue only affected Linux.
>>>>
>>> Yes, that's what I eventually concluded. I put the fix in
>>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did the
>>> page aligning, not add the guard page. That way all Posix ports are fixed
>>> in one place. It seems to be working.
>>>
>>>>
>>>> BTW, did you know java users can specify the size of the a new thread's
>>>>> stack:
>>>>>
>>>>
>>>> Yes I mentioned that in another reply - wondering whether we suitably
>>>> check and aligned such requests.
>>>>
>>> No we don't. Below I mentioned I was able to trigger the assert with a
>>> 257k stack size. I guess I wasn't clear that I did that from Java. I have a
>>> new test to add that will be testing this, plus the 9007199254740991 stack
>>> size (which fails to create the thread with an OOME, but that's
>>> acceptable). The fix I mention above in os::Posix::get_initial_stack_size()
>>> takes care of this issue also.
>>>
>> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>>
>> I should have clarified it's 9007199254740991 "K", which is
>> 9223372036854774784. Unfortunately on 32bit systems that is asserting with
>> EINVAL. I think we need to do a better job of dealing with 32-bit size_t
>> values:
>>
>> jlong java_lang_Thread::stackSize(oop java_thread) {
>>   if (_stackSize_offset > 0) {
>>     return java_thread->long_field(_stackSize_offset);
>>   } else {
>>     return 0;
>>   }
>> }
>>
>>       jlong size =
>> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>>       // Allocate the C++ Thread structure and create the native thread.
>> The
>>       // stack size retrieved from java is signed, but the constructor
>> takes
>>       // size_t (an unsigned type), so avoid passing negative values
>> which would
>>       // result in really large stacks.
>>       size_t sz = size > 0 ? (size_t) size : 0;
>>       native_thread = new JavaThread(&thread_entry, sz);
>>
>> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT),
>> which is 0xfffffc00 when cast to a size_t on a 32-bit system (close to
>> 32-bit MAX_UINT). Round it up to the 4k page size and you get 0, which I
>> guess pthread_attr_setstacksize() doesn't like. So I think more processing
>> of the size is needed here. Maybe in os::create_thread() we should check
>> for 0 after rounding up, and subtract the os page size if it is 0. However,
>> I think we should also avoid truncating on 32-bit to what is basically some
>> random number. Maybe if "size" (a jlong) is greater than UINT_MAX, then
>> just set "sz" (a size_t) it to UINT_MAX.
>>
>> Ok, I think I have this all worked out now. I've added fixes for
> unaligned stack sizes, 32-bit truncating of stack size, and the "aligning
> up to 0" problem. I also added a test. Here's the latest webrev:
>
> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>
> Here's what's changed since webrev.01:
>
> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
> stack size to be paged aligned. This fixes issues on Mac OS X (other
> platforms seem to be immune to this). Then check if the size is zero after
> rounding up to the page size. Subtract the page size in this case to
> produce the maximum stack size allowed. Surprisingly I got no complaint
> from gcc for subtracting from an unsigned value that is known to be 0.
>
> os_linux.cpp: In os::create_thread(), I also check here to make sure the
> size is not 0 after adding the guard page and aligning up, and subtract the
> os page size if it is 0.
>
> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
> rounded up to 0 in os::Posix::get_initial_stack_size(), which will result
> in subtracting the os page size to get the actual maximum allowed stack
> size.
>
> TooSmallStackSize.java: added test case for unaligned stack sizes.
>
> TestThreadStackSizes.java: New test. Creates new threads with every size
> up to 320k in 1k increments. Then creates threads with a few other sizes
> that can be problematic.
>
> thanks,
>
> Chris
>
> Chris
>>
>>
>>
>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>                   long stackSize) {
>>>>>         init(group, target, name, stackSize);
>>>>>     }
>>>>>
>>>>> Fortunately we already force the stackSize to be at least
>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>> assert by creating a thread with size 257k.
>>>>>
>>>>
>>>> Note this means that OSX stack logic is broken because it will
>>>> currently be silently failing due to EINVAL!
>>>>
>>> Yes, that is correct.
>>>
>>> Chris
>>>
>>>>
>>>> I'll get another webrev out once I've made the needed fixes. I also have
>>>>> a new test I'd like to add.
>>>>>
>>>>
>>>> Ok.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> Chris
>>>>>
>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>
>>>>>> Ok, time for a new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webr
>>>>>> ev.hotspot
>>>>>>
>>>>>> The only thing that has changed since the first webrev are the asserts
>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we discuss
>>>>>> already:
>>>>>>
>>>>>>  - The assert should never happen due to the stack size being too
>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>> because we always align it to the page size.
>>>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>>>  - No fixing the java launcher. If the user specifies a stack that is
>>>>>> too small, hotspot will already detect this. If the user specifies a
>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>
>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but figure
>>>>>> getting agreement on the changes first would be best before I bog down
>>>>>> our testing resources.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webr
>>>>>>> ev.hotspot
>>>>>>>
>>>>>>> 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);
>>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
Hi Chris,

On Tue, Mar 21, 2017 at 6:43 PM, Chris Plummer <[hidden email]>
wrote:

> Hi Thomas,
>
> On 3/21/17 1:08 AM, Thomas Stüfe wrote:
>
> Hi Chris,
>
> looks fine to me.
>
> small nit: os_linux_cpp:
>
>
> +  // In that cast just subract the page size to get the maximum possible
> stack size.
>
> subract->substract
>
> Ok. I had the same typo in os_posix.cpp.
>
>
> I would have preferred SIZE_MAX instead of UINT_MAX, but both is fine to
> me.
>
> I'll use SIZE_MAX. I didn't see it in limits.h so I didn't know of it's
> existence. Turns out it is in stdint.h.
>
>
> --
>
> As for the size_overflow problem, I wonder whether we are overthinking
> this. Maybe a more pragmatic approach would have been to just define a
> reasonable upper maximum for stack sizes which could be well below
> UINT_MAX. Somehow I cannot see users wanting to start threads with more
> than ~4GB of thread stack size.
>
> I had debated doing this. I decided not to artificially limit the stack
> size for a couple of reasons. (1) I figured we should make a best effort to
> use the size the user requested. (2) There is no obvious large size to
> choose other than the max size. If we choose a smaller size it might still
> fail or might be smaller than the largest size that will succeed.
>
>
Makes sense.


> Thanks again for the review.
>
>
Sure, thanks for taking my input.

Kind regards, Thomas

Chris

>
>
> Kind Regards, Thomas
>
>
>
> On Tue, Mar 21, 2017 at 12:29 AM, Chris Plummer <[hidden email]>
> wrote:
>
>> On 3/17/17 11:37 PM, Chris Plummer wrote:
>>
>>> On 3/17/17 8:17 PM, Chris Plummer wrote:
>>>
>>>> On 3/17/17 7:01 PM, David Holmes wrote:
>>>>
>>>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>>>
>>>>>> Looks like this will need some more work since the added asserts are
>>>>>> triggering on mac os x (which is the only place we'd currently expect
>>>>>> them to assert).
>>>>>>
>>>>>> The problem is that the code I found that rounds up to the page size
>>>>>> is
>>>>>> only applied to java threads created by the VM for which the java user
>>>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>>>> rounded. The failure I saw was with
>>>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>>>> when is
>>>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>>>>> assert
>>>>>> with an EINVAL. The size is not aligned, but it could also be
>>>>>> complaining because it is too big. I haven't tried aligning it yet to
>>>>>> see.
>>>>>>
>>>>>> On Linux we do the following:
>>>>>>
>>>>>>   stack_size = align_size_up(stack_size +
>>>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>>>
>>>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>>>> problem. I'm not sure about adding the guard size. I'll need to see if
>>>>>> mac os x has the same pthread bug as linux does.
>>>>>>
>>>>>
>>>>> At this stage I would only deal with alignment issues. IIRC the guard
>>>>> issue only affected Linux.
>>>>>
>>>> Yes, that's what I eventually concluded. I put the fix in
>>>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did the
>>>> page aligning, not add the guard page. That way all Posix ports are fixed
>>>> in one place. It seems to be working.
>>>>
>>>>>
>>>>> BTW, did you know java users can specify the size of the a new thread's
>>>>>> stack:
>>>>>>
>>>>>
>>>>> Yes I mentioned that in another reply - wondering whether we suitably
>>>>> check and aligned such requests.
>>>>>
>>>> No we don't. Below I mentioned I was able to trigger the assert with a
>>>> 257k stack size. I guess I wasn't clear that I did that from Java. I have a
>>>> new test to add that will be testing this, plus the 9007199254740991 stack
>>>> size (which fails to create the thread with an OOME, but that's
>>>> acceptable). The fix I mention above in os::Posix::get_initial_stack_size()
>>>> takes care of this issue also.
>>>>
>>> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>>>
>>> I should have clarified it's 9007199254740991 "K", which is
>>> 9223372036854774784. Unfortunately on 32bit systems that is asserting with
>>> EINVAL. I think we need to do a better job of dealing with 32-bit size_t
>>> values:
>>>
>>> jlong java_lang_Thread::stackSize(oop java_thread) {
>>>   if (_stackSize_offset > 0) {
>>>     return java_thread->long_field(_stackSize_offset);
>>>   } else {
>>>     return 0;
>>>   }
>>> }
>>>
>>>       jlong size =
>>> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>>>       // Allocate the C++ Thread structure and create the native
>>> thread.  The
>>>       // stack size retrieved from java is signed, but the constructor
>>> takes
>>>       // size_t (an unsigned type), so avoid passing negative values
>>> which would
>>>       // result in really large stacks.
>>>       size_t sz = size > 0 ? (size_t) size : 0;
>>>       native_thread = new JavaThread(&thread_entry, sz);
>>>
>>> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT),
>>> which is 0xfffffc00 when cast to a size_t on a 32-bit system (close to
>>> 32-bit MAX_UINT). Round it up to the 4k page size and you get 0, which I
>>> guess pthread_attr_setstacksize() doesn't like. So I think more processing
>>> of the size is needed here. Maybe in os::create_thread() we should check
>>> for 0 after rounding up, and subtract the os page size if it is 0. However,
>>> I think we should also avoid truncating on 32-bit to what is basically some
>>> random number. Maybe if "size" (a jlong) is greater than UINT_MAX, then
>>> just set "sz" (a size_t) it to UINT_MAX.
>>>
>>> Ok, I think I have this all worked out now. I've added fixes for
>> unaligned stack sizes, 32-bit truncating of stack size, and the "aligning
>> up to 0" problem. I also added a test. Here's the latest webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>
>> Here's what's changed since webrev.01:
>>
>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>> platforms seem to be immune to this). Then check if the size is zero after
>> rounding up to the page size. Subtract the page size in this case to
>> produce the maximum stack size allowed. Surprisingly I got no complaint
>> from gcc for subtracting from an unsigned value that is known to be 0.
>>
>> os_linux.cpp: In os::create_thread(), I also check here to make sure the
>> size is not 0 after adding the guard page and aligning up, and subtract the
>> os page size if it is 0.
>>
>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>> result in subtracting the os page size to get the actual maximum allowed
>> stack size.
>>
>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>
>> TestThreadStackSizes.java: New test. Creates new threads with every size
>> up to 320k in 1k increments. Then creates threads with a few other sizes
>> that can be problematic.
>>
>> thanks,
>>
>> Chris
>>
>> Chris
>>>
>>>
>>>
>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>                   long stackSize) {
>>>>>>         init(group, target, name, stackSize);
>>>>>>     }
>>>>>>
>>>>>> Fortunately we already force the stackSize to be at least
>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>> assert by creating a thread with size 257k.
>>>>>>
>>>>>
>>>>> Note this means that OSX stack logic is broken because it will
>>>>> currently be silently failing due to EINVAL!
>>>>>
>>>> Yes, that is correct.
>>>>
>>>> Chris
>>>>
>>>>>
>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>> have
>>>>>> a new test I'd like to add.
>>>>>>
>>>>>
>>>>> Ok.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> Chris
>>>>>>
>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>
>>>>>>> Ok, time for a new webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webr
>>>>>>> ev.hotspot
>>>>>>>
>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>> asserts
>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>> discuss
>>>>>>> already:
>>>>>>>
>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>> because we always align it to the page size.
>>>>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>>>>  - No fixing the java launcher. If the user specifies a stack that is
>>>>>>> too small, hotspot will already detect this. If the user specifies a
>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>
>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>> figure
>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>> down
>>>>>>> our testing resources.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webr
>>>>>>>> ev.hotspot
>>>>>>>>
>>>>>>>> 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);
>>>>>>>>
>>>>>>>> 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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by Chris Plummer
 > http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot

src/os/aix/vm/os_aix.cpp
     No comments.

src/os/bsd/vm/os_bsd.cpp
     No comments.

src/os/linux/vm/os_linux.cpp
     L729:   // In that cast just subract the page size to get the
maximum possible stack size.
         Typo: 'cast' -> 'case'
         Typo: 'subract' -> 'subtract' (Thomas also commented on it)

src/os/posix/vm/os_posix.cpp
     L263:   // aligning up could have resulted in the size being 0. In
that case just subract the
         Nit: 'aligning' -> 'Aligning' (since it's a sentence)
         Typo: 'subract' -> 'subtract'

src/os/solaris/vm/os_solaris.cpp
     No comments.

src/share/vm/prims/jvm.cpp
     L2812:       // -Avoid truncating on 32-bit platforms if size is
greater than UINT_MAX
         Nit: needs a period at the end like L2813.

test/runtime/Thread/TooSmallStackSize.java
     No comments.

test/runtime/Thread/TestThreadStackSizes.java
     L26:  * @summary Test user threads with various stacks sizes.
         Typo?: "stacks sizes" -> "stack sizes"

     L36:         super(null, null, "TestThreadStackSizes"+stackSize,
stackSize);
         Nit: spaces around the "+".

     L46:             TestThreadStackSizes testThreadStackSize  = new
TestThreadStackSizes(stackSize);
         Nit: extra space before '='.

     So this test makes 326 createThread() calls... how long does
     it take to run?


Thumbs up! I don't need to see another webrev if you choose to
fix these minor typos...

Dan


On 3/20/17 5:29 PM, Chris Plummer wrote:

> On 3/17/17 11:37 PM, Chris Plummer wrote:
>> On 3/17/17 8:17 PM, Chris Plummer wrote:
>>> On 3/17/17 7:01 PM, David Holmes wrote:
>>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>>> Looks like this will need some more work since the added asserts are
>>>>> triggering on mac os x (which is the only place we'd currently expect
>>>>> them to assert).
>>>>>
>>>>> The problem is that the code I found that rounds up to the page
>>>>> size is
>>>>> only applied to java threads created by the VM for which the java
>>>>> user
>>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>>> rounded. The failure I saw was with
>>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>>> when is
>>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>>>> assert
>>>>> with an EINVAL. The size is not aligned, but it could also be
>>>>> complaining because it is too big. I haven't tried aligning it yet
>>>>> to see.
>>>>>
>>>>> On Linux we do the following:
>>>>>
>>>>>   stack_size = align_size_up(stack_size +
>>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>>
>>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>>> problem. I'm not sure about adding the guard size. I'll need to
>>>>> see if
>>>>> mac os x has the same pthread bug as linux does.
>>>>
>>>> At this stage I would only deal with alignment issues. IIRC the
>>>> guard issue only affected Linux.
>>> Yes, that's what I eventually concluded. I put the fix in
>>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did
>>> the page aligning, not add the guard page. That way all Posix ports
>>> are fixed in one place. It seems to be working.
>>>>
>>>>> BTW, did you know java users can specify the size of the a new
>>>>> thread's
>>>>> stack:
>>>>
>>>> Yes I mentioned that in another reply - wondering whether we
>>>> suitably check and aligned such requests.
>>> No we don't. Below I mentioned I was able to trigger the assert with
>>> a 257k stack size. I guess I wasn't clear that I did that from Java.
>>> I have a new test to add that will be testing this, plus the
>>> 9007199254740991 stack size (which fails to create the thread with
>>> an OOME, but that's acceptable). The fix I mention above in
>>> os::Posix::get_initial_stack_size() takes care of this issue also.
>> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>>
>> I should have clarified it's 9007199254740991 "K", which is
>> 9223372036854774784. Unfortunately on 32bit systems that is asserting
>> with EINVAL. I think we need to do a better job of dealing with
>> 32-bit size_t values:
>>
>> jlong java_lang_Thread::stackSize(oop java_thread) {
>>   if (_stackSize_offset > 0) {
>>     return java_thread->long_field(_stackSize_offset);
>>   } else {
>>     return 0;
>>   }
>> }
>>
>>       jlong size =
>> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>>       // Allocate the C++ Thread structure and create the native
>> thread.  The
>>       // stack size retrieved from java is signed, but the
>> constructor takes
>>       // size_t (an unsigned type), so avoid passing negative values
>> which would
>>       // result in really large stacks.
>>       size_t sz = size > 0 ? (size_t) size : 0;
>>       native_thread = new JavaThread(&thread_entry, sz);
>>
>> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT),
>> which is 0xfffffc00 when cast to a size_t on a 32-bit system (close
>> to 32-bit MAX_UINT). Round it up to the 4k page size and you get 0,
>> which I guess pthread_attr_setstacksize() doesn't like. So I think
>> more processing of the size is needed here. Maybe in
>> os::create_thread() we should check for 0 after rounding up, and
>> subtract the os page size if it is 0. However, I think we should also
>> avoid truncating on 32-bit to what is basically some random number.
>> Maybe if "size" (a jlong) is greater than UINT_MAX, then just set
>> "sz" (a size_t) it to UINT_MAX.
>>
> Ok, I think I have this all worked out now. I've added fixes for
> unaligned stack sizes, 32-bit truncating of stack size, and the
> "aligning up to 0" problem. I also added a test. Here's the latest
> webrev:
>
> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>
> Here's what's changed since webrev.01:
>
> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up
> the stack size to be paged aligned. This fixes issues on Mac OS X
> (other platforms seem to be immune to this). Then check if the size is
> zero after rounding up to the page size. Subtract the page size in
> this case to produce the maximum stack size allowed. Surprisingly I
> got no complaint from gcc for subtracting from an unsigned value that
> is known to be 0.
>
> os_linux.cpp: In os::create_thread(), I also check here to make sure
> the size is not 0 after adding the guard page and aligning up, and
> subtract the os page size if it is 0.
>
> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is
> greater than UINT_MAX, then I set the size to UINT_MAX. Note it will
> later be rounded up to 0 in os::Posix::get_initial_stack_size(), which
> will result in subtracting the os page size to get the actual maximum
> allowed stack size.
>
> TooSmallStackSize.java: added test case for unaligned stack sizes.
>
> TestThreadStackSizes.java: New test. Creates new threads with every
> size up to 320k in 1k increments. Then creates threads with a few
> other sizes that can be problematic.
>
> thanks,
>
> Chris
>> Chris
>>
>>
>>>>
>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>                   long stackSize) {
>>>>>         init(group, target, name, stackSize);
>>>>>     }
>>>>>
>>>>> Fortunately we already force the stackSize to be at least
>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>> assert by creating a thread with size 257k.
>>>>
>>>> Note this means that OSX stack logic is broken because it will
>>>> currently be silently failing due to EINVAL!
>>> Yes, that is correct.
>>>
>>> Chris
>>>>
>>>>> I'll get another webrev out once I've made the needed fixes. I
>>>>> also have
>>>>> a new test I'd like to add.
>>>>
>>>> Ok.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Chris
>>>>>
>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>> Ok, time for a new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>
>>>>>>
>>>>>> The only thing that has changed since the first webrev are the
>>>>>> asserts
>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>> discuss
>>>>>> already:
>>>>>>
>>>>>>  - The assert should never happen due to the stack size being too
>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>> because we always align it to the page size.
>>>>>>  - Any assert would therefore be a VM bug and not due to user error.
>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>> that is
>>>>>> too small, hotspot will already detect this. If the user specifies a
>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>
>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>> figure
>>>>>> getting agreement on the changes first would be best before I bog
>>>>>> down
>>>>>> our testing resources.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>
>>>>>>>
>>>>>>> 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);
>>>>>>>
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by David Holmes
On 3/20/17 6:59 PM, David Holmes wrote:

> Hi Chris,
>
> <trimming>
>
> Getting closer :)
>
> Have you seen pthread_attr_setstacksize return EINVAL on very large
> values? Or does pthread_create just give a ENOMEM?
>
> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>
>> Here's what's changed since webrev.01:
>>
>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up the
>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>> platforms seem to be immune to this). Then check if the size is zero
>> after rounding up to the page size. Subtract the page size in this case
>> to produce the maximum stack size allowed. Surprisingly I got no
>> complaint from gcc for subtracting from an unsigned value that is known
>> to be 0.
>
> I'm a little surprised at that too. :)

Chris, would you mind checking with Kim Barrett on the
advisability of doing it this way?

Dan


>
>> os_linux.cpp: In os::create_thread(), I also check here to make sure the
>> size is not 0 after adding the guard page and aligning up, and subtract
>> the os page size if it is 0.
>
> What if adding the guard page and rounding up causes overflow so that
> we get a very small positive stack size?
>
>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>> result in subtracting the os page size to get the actual maximum allowed
>> stack size.
>
> Good catch!
>
> Nit: "-Avoid"  -> "- Avoid"
>
>>
>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>
> Ok.
>
>> TestThreadStackSizes.java: New test. Creates new threads with every size
>> up to 320k in 1k increments. Then creates threads with a few other sizes
>> that can be problematic.
>
> So this test never actually fails as long as the VM doesn't crash - is
> that right?
>
>   27  * @modules java.base/jdk.internal.misc
>   28  * @library /test/lib
>
> The above are not needed by this test.
>
> Thanks,
> David
>
>> thanks,
>>
>> Chris
>>> Chris
>>>
>>>
>>>>>
>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>                   long stackSize) {
>>>>>>         init(group, target, name, stackSize);
>>>>>>     }
>>>>>>
>>>>>> Fortunately we already force the stackSize to be at least
>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>> assert by creating a thread with size 257k.
>>>>>
>>>>> Note this means that OSX stack logic is broken because it will
>>>>> currently be silently failing due to EINVAL!
>>>> Yes, that is correct.
>>>>
>>>> Chris
>>>>>
>>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>> have
>>>>>> a new test I'd like to add.
>>>>>
>>>>> Ok.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>> Ok, time for a new webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>> asserts
>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>> discuss
>>>>>>> already:
>>>>>>>
>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>> because we always align it to the page size.
>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>> error.
>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>> that is
>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>> specifies a
>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>
>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>> figure
>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>> down
>>>>>>> our testing resources.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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);
>>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
On 3/21/17 12:53 PM, Daniel D. Daugherty wrote:

> On 3/20/17 6:59 PM, David Holmes wrote:
>> Hi Chris,
>>
>> <trimming>
>>
>> Getting closer :)
>>
>> Have you seen pthread_attr_setstacksize return EINVAL on very large
>> values? Or does pthread_create just give a ENOMEM?
>>
>> On 21/03/2017 9:29 AM, Chris Plummer wrote:
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>>
>>> Here's what's changed since webrev.01:
>>>
>>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up
>>> the
>>> stack size to be paged aligned. This fixes issues on Mac OS X (other
>>> platforms seem to be immune to this). Then check if the size is zero
>>> after rounding up to the page size. Subtract the page size in this case
>>> to produce the maximum stack size allowed. Surprisingly I got no
>>> complaint from gcc for subtracting from an unsigned value that is known
>>> to be 0.
>>
>> I'm a little surprised at that too. :)
>
> Chris, would you mind checking with Kim Barrett on the
> advisability of doing it this way?
Ok.

Chris

>
> Dan
>
>
>>
>>> os_linux.cpp: In os::create_thread(), I also check here to make sure
>>> the
>>> size is not 0 after adding the guard page and aligning up, and subtract
>>> the os page size if it is 0.
>>
>> What if adding the guard page and rounding up causes overflow so that
>> we get a very small positive stack size?
>>
>>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is greater
>>> than UINT_MAX, then I set the size to UINT_MAX. Note it will later be
>>> rounded up to 0 in os::Posix::get_initial_stack_size(), which will
>>> result in subtracting the os page size to get the actual maximum
>>> allowed
>>> stack size.
>>
>> Good catch!
>>
>> Nit: "-Avoid"  -> "- Avoid"
>>
>>>
>>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>
>> Ok.
>>
>>> TestThreadStackSizes.java: New test. Creates new threads with every
>>> size
>>> up to 320k in 1k increments. Then creates threads with a few other
>>> sizes
>>> that can be problematic.
>>
>> So this test never actually fails as long as the VM doesn't crash -
>> is that right?
>>
>>   27  * @modules java.base/jdk.internal.misc
>>   28  * @library /test/lib
>>
>> The above are not needed by this test.
>>
>> Thanks,
>> David
>>
>>> thanks,
>>>
>>> Chris
>>>> Chris
>>>>
>>>>
>>>>>>
>>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>>                   long stackSize) {
>>>>>>>         init(group, target, name, stackSize);
>>>>>>>     }
>>>>>>>
>>>>>>> Fortunately we already force the stackSize to be at least
>>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>>> assert by creating a thread with size 257k.
>>>>>>
>>>>>> Note this means that OSX stack logic is broken because it will
>>>>>> currently be silently failing due to EINVAL!
>>>>> Yes, that is correct.
>>>>>
>>>>> Chris
>>>>>>
>>>>>>> I'll get another webrev out once I've made the needed fixes. I also
>>>>>>> have
>>>>>>> a new test I'd like to add.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>>> Ok, time for a new webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>>> asserts
>>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>>> discuss
>>>>>>>> already:
>>>>>>>>
>>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>>> because we always align it to the page size.
>>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>>> error.
>>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>>> that is
>>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>>> specifies a
>>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>>> ignore any error (if the platform doth protest) and the user
>>>>>>>> gets a
>>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>>
>>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>>> figure
>>>>>>>> getting agreement on the changes first would be best before I bog
>>>>>>>> down
>>>>>>>> our testing resources.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please review the following:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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);
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Chris Plummer
In reply to this post by Daniel D. Daugherty
On 3/21/17 12:51 PM, Daniel D. Daugherty wrote:

> > http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>
> src/os/aix/vm/os_aix.cpp
>     No comments.
>
> src/os/bsd/vm/os_bsd.cpp
>     No comments.
>
> src/os/linux/vm/os_linux.cpp
>     L729:   // In that cast just subract the page size to get the
> maximum possible stack size.
>         Typo: 'cast' -> 'case'
>         Typo: 'subract' -> 'subtract' (Thomas also commented on it)
>
> src/os/posix/vm/os_posix.cpp
>     L263:   // aligning up could have resulted in the size being 0. In
> that case just subract the
>         Nit: 'aligning' -> 'Aligning' (since it's a sentence)
>         Typo: 'subract' -> 'subtract'
>
> src/os/solaris/vm/os_solaris.cpp
>     No comments.
>
> src/share/vm/prims/jvm.cpp
>     L2812:       // -Avoid truncating on 32-bit platforms if size is
> greater than UINT_MAX
>         Nit: needs a period at the end like L2813.
>
> test/runtime/Thread/TooSmallStackSize.java
>     No comments.
>
> test/runtime/Thread/TestThreadStackSizes.java
>     L26:  * @summary Test user threads with various stacks sizes.
>         Typo?: "stacks sizes" -> "stack sizes"
>
>     L36:         super(null, null, "TestThreadStackSizes"+stackSize,
> stackSize);
>         Nit: spaces around the "+".
>
>     L46:             TestThreadStackSizes testThreadStackSize  = new
> TestThreadStackSizes(stackSize);
>         Nit: extra space before '='.
>
>     So this test makes 326 createThread() calls... how long does
>     it take to run?
>
This is from the results page on Mac OS x log for all the tests in
runtime/Thread

1    runtime/Thread/CancellableThreadTest.java         7.033
2    runtime/Thread/Fibonacci.java         8.430
3    runtime/Thread/TestThreadDumpMonitorContention.java 34.322
4    runtime/Thread/ThreadPriorities.java         13.064
5    runtime/Thread/TooSmallStackSize.java         10.086

And 32-bit linux-arm:

1    runtime/Thread/CancellableThreadTest.java         9.359
2    runtime/Thread/Fibonacci.java         11.744
3    runtime/Thread/TestThreadDumpMonitorContention.java 00:01:04.370
4    runtime/Thread/ThreadPriorities.java         18.140
5    runtime/Thread/TooSmallStackSize.java         14.919

And windows-x64:

1    runtime/Thread/CancellableThreadTest.java         8.074
2    runtime/Thread/Fibonacci.java         10.238
3    runtime/Thread/TestThreadDumpMonitorContention.java 00:01:21.404
4    runtime/Thread/ThreadPriorities.java           23.134
5    runtime/Thread/TooSmallStackSize.java         24.160

>
> Thumbs up! I don't need to see another webrev if you choose to
> fix these minor typos...
They've all been fixed.

thanks for the review,

Chris

>
> Dan
>
>
> On 3/20/17 5:29 PM, Chris Plummer wrote:
>> On 3/17/17 11:37 PM, Chris Plummer wrote:
>>> On 3/17/17 8:17 PM, Chris Plummer wrote:
>>>> On 3/17/17 7:01 PM, David Holmes wrote:
>>>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>>>> Looks like this will need some more work since the added asserts are
>>>>>> triggering on mac os x (which is the only place we'd currently
>>>>>> expect
>>>>>> them to assert).
>>>>>>
>>>>>> The problem is that the code I found that rounds up to the page
>>>>>> size is
>>>>>> only applied to java threads created by the VM for which the java
>>>>>> user
>>>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>>>> rounded. The failure I saw was with
>>>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>>>> when is
>>>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit the
>>>>>> assert
>>>>>> with an EINVAL. The size is not aligned, but it could also be
>>>>>> complaining because it is too big. I haven't tried aligning it
>>>>>> yet to see.
>>>>>>
>>>>>> On Linux we do the following:
>>>>>>
>>>>>>   stack_size = align_size_up(stack_size +
>>>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>>>
>>>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>>>> problem. I'm not sure about adding the guard size. I'll need to
>>>>>> see if
>>>>>> mac os x has the same pthread bug as linux does.
>>>>>
>>>>> At this stage I would only deal with alignment issues. IIRC the
>>>>> guard issue only affected Linux.
>>>> Yes, that's what I eventually concluded. I put the fix in
>>>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did
>>>> the page aligning, not add the guard page. That way all Posix ports
>>>> are fixed in one place. It seems to be working.
>>>>>
>>>>>> BTW, did you know java users can specify the size of the a new
>>>>>> thread's
>>>>>> stack:
>>>>>
>>>>> Yes I mentioned that in another reply - wondering whether we
>>>>> suitably check and aligned such requests.
>>>> No we don't. Below I mentioned I was able to trigger the assert
>>>> with a 257k stack size. I guess I wasn't clear that I did that from
>>>> Java. I have a new test to add that will be testing this, plus the
>>>> 9007199254740991 stack size (which fails to create the thread with
>>>> an OOME, but that's acceptable). The fix I mention above in
>>>> os::Posix::get_initial_stack_size() takes care of this issue also.
>>> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>>>
>>> I should have clarified it's 9007199254740991 "K", which is
>>> 9223372036854774784. Unfortunately on 32bit systems that is
>>> asserting with EINVAL. I think we need to do a better job of dealing
>>> with 32-bit size_t values:
>>>
>>> jlong java_lang_Thread::stackSize(oop java_thread) {
>>>   if (_stackSize_offset > 0) {
>>>     return java_thread->long_field(_stackSize_offset);
>>>   } else {
>>>     return 0;
>>>   }
>>> }
>>>
>>>       jlong size =
>>> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>>>       // Allocate the C++ Thread structure and create the native
>>> thread.  The
>>>       // stack size retrieved from java is signed, but the
>>> constructor takes
>>>       // size_t (an unsigned type), so avoid passing negative values
>>> which would
>>>       // result in really large stacks.
>>>       size_t sz = size > 0 ? (size_t) size : 0;
>>>       native_thread = new JavaThread(&thread_entry, sz);
>>>
>>> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit MAX_INT),
>>> which is 0xfffffc00 when cast to a size_t on a 32-bit system (close
>>> to 32-bit MAX_UINT). Round it up to the 4k page size and you get 0,
>>> which I guess pthread_attr_setstacksize() doesn't like. So I think
>>> more processing of the size is needed here. Maybe in
>>> os::create_thread() we should check for 0 after rounding up, and
>>> subtract the os page size if it is 0. However, I think we should
>>> also avoid truncating on 32-bit to what is basically some random
>>> number. Maybe if "size" (a jlong) is greater than UINT_MAX, then
>>> just set "sz" (a size_t) it to UINT_MAX.
>>>
>> Ok, I think I have this all worked out now. I've added fixes for
>> unaligned stack sizes, 32-bit truncating of stack size, and the
>> "aligning up to 0" problem. I also added a test. Here's the latest
>> webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>
>> Here's what's changed since webrev.01:
>>
>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up
>> the stack size to be paged aligned. This fixes issues on Mac OS X
>> (other platforms seem to be immune to this). Then check if the size
>> is zero after rounding up to the page size. Subtract the page size in
>> this case to produce the maximum stack size allowed. Surprisingly I
>> got no complaint from gcc for subtracting from an unsigned value that
>> is known to be 0.
>>
>> os_linux.cpp: In os::create_thread(), I also check here to make sure
>> the size is not 0 after adding the guard page and aligning up, and
>> subtract the os page size if it is 0.
>>
>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is
>> greater than UINT_MAX, then I set the size to UINT_MAX. Note it will
>> later be rounded up to 0 in os::Posix::get_initial_stack_size(),
>> which will result in subtracting the os page size to get the actual
>> maximum allowed stack size.
>>
>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>
>> TestThreadStackSizes.java: New test. Creates new threads with every
>> size up to 320k in 1k increments. Then creates threads with a few
>> other sizes that can be problematic.
>>
>> thanks,
>>
>> Chris
>>> Chris
>>>
>>>
>>>>>
>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>                   long stackSize) {
>>>>>>         init(group, target, name, stackSize);
>>>>>>     }
>>>>>>
>>>>>> Fortunately we already force the stackSize to be at least
>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>> assert by creating a thread with size 257k.
>>>>>
>>>>> Note this means that OSX stack logic is broken because it will
>>>>> currently be silently failing due to EINVAL!
>>>> Yes, that is correct.
>>>>
>>>> Chris
>>>>>
>>>>>> I'll get another webrev out once I've made the needed fixes. I
>>>>>> also have
>>>>>> a new test I'd like to add.
>>>>>
>>>>> Ok.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>> Ok, time for a new webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>
>>>>>>>
>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>> asserts
>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>> discuss
>>>>>>> already:
>>>>>>>
>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>> because we always align it to the page size.
>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>> error.
>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>> that is
>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>> specifies a
>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>> ignore any error (if the platform doth protest) and the user gets a
>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>
>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>> figure
>>>>>>> getting agreement on the changes first would be best before I
>>>>>>> bog down
>>>>>>> our testing resources.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>> 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);
>>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
On 3/21/17 3:20 PM, Chris Plummer wrote:

> On 3/21/17 12:51 PM, Daniel D. Daugherty wrote:
>> > http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>
>> src/os/aix/vm/os_aix.cpp
>>     No comments.
>>
>> src/os/bsd/vm/os_bsd.cpp
>>     No comments.
>>
>> src/os/linux/vm/os_linux.cpp
>>     L729:   // In that cast just subract the page size to get the
>> maximum possible stack size.
>>         Typo: 'cast' -> 'case'
>>         Typo: 'subract' -> 'subtract' (Thomas also commented on it)
>>
>> src/os/posix/vm/os_posix.cpp
>>     L263:   // aligning up could have resulted in the size being 0.
>> In that case just subract the
>>         Nit: 'aligning' -> 'Aligning' (since it's a sentence)
>>         Typo: 'subract' -> 'subtract'
>>
>> src/os/solaris/vm/os_solaris.cpp
>>     No comments.
>>
>> src/share/vm/prims/jvm.cpp
>>     L2812:       // -Avoid truncating on 32-bit platforms if size is
>> greater than UINT_MAX
>>         Nit: needs a period at the end like L2813.
>>
>> test/runtime/Thread/TooSmallStackSize.java
>>     No comments.
>>
>> test/runtime/Thread/TestThreadStackSizes.java
>>     L26:  * @summary Test user threads with various stacks sizes.
>>         Typo?: "stacks sizes" -> "stack sizes"
>>
>>     L36:         super(null, null, "TestThreadStackSizes"+stackSize,
>> stackSize);
>>         Nit: spaces around the "+".
>>
>>     L46:             TestThreadStackSizes testThreadStackSize  = new
>> TestThreadStackSizes(stackSize);
>>         Nit: extra space before '='.
>>
>>     So this test makes 326 createThread() calls... how long does
>>     it take to run?
>>
> This is from the results page on Mac OS x log for all the tests in
> runtime/Thread
>
> 1    runtime/Thread/CancellableThreadTest.java         7.033
> 2    runtime/Thread/Fibonacci.java         8.430
> 3    runtime/Thread/TestThreadDumpMonitorContention.java 34.322
> 4    runtime/Thread/ThreadPriorities.java         13.064
> 5    runtime/Thread/TooSmallStackSize.java         10.086
>
> And 32-bit linux-arm:
>
> 1    runtime/Thread/CancellableThreadTest.java         9.359
> 2    runtime/Thread/Fibonacci.java         11.744
> 3    runtime/Thread/TestThreadDumpMonitorContention.java 00:01:04.370
> 4    runtime/Thread/ThreadPriorities.java         18.140
> 5    runtime/Thread/TooSmallStackSize.java         14.919
>
> And windows-x64:
>
> 1    runtime/Thread/CancellableThreadTest.java         8.074
> 2    runtime/Thread/Fibonacci.java         10.238
> 3    runtime/Thread/TestThreadDumpMonitorContention.java 00:01:21.404
> 4    runtime/Thread/ThreadPriorities.java           23.134
> 5    runtime/Thread/TooSmallStackSize.java         24.160

Maybe I'm missing it, but I don't see results for the new test
(TestThreadStackSizes.java)...

Dan


>
>>
>> Thumbs up! I don't need to see another webrev if you choose to
>> fix these minor typos...
> They've all been fixed.
>
> thanks for the review,
>
> Chris
>
>>
>> Dan
>>
>>
>> On 3/20/17 5:29 PM, Chris Plummer wrote:
>>> On 3/17/17 11:37 PM, Chris Plummer wrote:
>>>> On 3/17/17 8:17 PM, Chris Plummer wrote:
>>>>> On 3/17/17 7:01 PM, David Holmes wrote:
>>>>>> On 18/03/2017 9:11 AM, Chris Plummer wrote:
>>>>>>> Looks like this will need some more work since the added asserts
>>>>>>> are
>>>>>>> triggering on mac os x (which is the only place we'd currently
>>>>>>> expect
>>>>>>> them to assert).
>>>>>>>
>>>>>>> The problem is that the code I found that rounds up to the page
>>>>>>> size is
>>>>>>> only applied to java threads created by the VM for which the
>>>>>>> java user
>>>>>>> specified no stack size. The VM and Compiler thread sizes are not
>>>>>>> rounded. The failure I saw was with
>>>>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>>>>> when is
>>>>>>> specified -XX:CompilerThreadStackSize=9007199254740991. I hit
>>>>>>> the assert
>>>>>>> with an EINVAL. The size is not aligned, but it could also be
>>>>>>> complaining because it is too big. I haven't tried aligning it
>>>>>>> yet to see.
>>>>>>>
>>>>>>> On Linux we do the following:
>>>>>>>
>>>>>>>   stack_size = align_size_up(stack_size +
>>>>>>> os::Linux::default_guard_size(thr_type), vm_page_size());
>>>>>>>
>>>>>>> We don't do this on BSD. I think the same on BSD would solve this
>>>>>>> problem. I'm not sure about adding the guard size. I'll need to
>>>>>>> see if
>>>>>>> mac os x has the same pthread bug as linux does.
>>>>>>
>>>>>> At this stage I would only deal with alignment issues. IIRC the
>>>>>> guard issue only affected Linux.
>>>>> Yes, that's what I eventually concluded. I put the fix in
>>>>> os::Posix::get_initial_stack_size() in os_posix.cpp, and only did
>>>>> the page aligning, not add the guard page. That way all Posix
>>>>> ports are fixed in one place. It seems to be working.
>>>>>>
>>>>>>> BTW, did you know java users can specify the size of the a new
>>>>>>> thread's
>>>>>>> stack:
>>>>>>
>>>>>> Yes I mentioned that in another reply - wondering whether we
>>>>>> suitably check and aligned such requests.
>>>>> No we don't. Below I mentioned I was able to trigger the assert
>>>>> with a 257k stack size. I guess I wasn't clear that I did that
>>>>> from Java. I have a new test to add that will be testing this,
>>>>> plus the 9007199254740991 stack size (which fails to create the
>>>>> thread with an OOME, but that's acceptable). The fix I mention
>>>>> above in os::Posix::get_initial_stack_size() takes care of this
>>>>> issue also.
>>>> Rounding up triggers a new assert, this time on 32-bit x86 and arm.
>>>>
>>>> I should have clarified it's 9007199254740991 "K", which is
>>>> 9223372036854774784. Unfortunately on 32bit systems that is
>>>> asserting with EINVAL. I think we need to do a better job of
>>>> dealing with 32-bit size_t values:
>>>>
>>>> jlong java_lang_Thread::stackSize(oop java_thread) {
>>>>   if (_stackSize_offset > 0) {
>>>>     return java_thread->long_field(_stackSize_offset);
>>>>   } else {
>>>>     return 0;
>>>>   }
>>>> }
>>>>
>>>>       jlong size =
>>>> java_lang_Thread::stackSize(JNIHandles::resolve_non_null(jthread));
>>>>       // Allocate the C++ Thread structure and create the native
>>>> thread.  The
>>>>       // stack size retrieved from java is signed, but the
>>>> constructor takes
>>>>       // size_t (an unsigned type), so avoid passing negative
>>>> values which would
>>>>       // result in really large stacks.
>>>>       size_t sz = size > 0 ? (size_t) size : 0;
>>>>       native_thread = new JavaThread(&thread_entry, sz);
>>>>
>>>> 9223372036854774784 is 0x7ffffffffffffc00 (close to 64 bit
>>>> MAX_INT), which is 0xfffffc00 when cast to a size_t on a 32-bit
>>>> system (close to 32-bit MAX_UINT). Round it up to the 4k page size
>>>> and you get 0, which I guess pthread_attr_setstacksize() doesn't
>>>> like. So I think more processing of the size is needed here. Maybe
>>>> in os::create_thread() we should check for 0 after rounding up, and
>>>> subtract the os page size if it is 0. However, I think we should
>>>> also avoid truncating on 32-bit to what is basically some random
>>>> number. Maybe if "size" (a jlong) is greater than UINT_MAX, then
>>>> just set "sz" (a size_t) it to UINT_MAX.
>>>>
>>> Ok, I think I have this all worked out now. I've added fixes for
>>> unaligned stack sizes, 32-bit truncating of stack size, and the
>>> "aligning up to 0" problem. I also added a test. Here's the latest
>>> webrev:
>>>
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.02/webrev.hotspot
>>>
>>> Here's what's changed since webrev.01:
>>>
>>> os_posix.cpp: In os::Posix::get_initial_stack_size(), first round up
>>> the stack size to be paged aligned. This fixes issues on Mac OS X
>>> (other platforms seem to be immune to this). Then check if the size
>>> is zero after rounding up to the page size. Subtract the page size
>>> in this case to produce the maximum stack size allowed. Surprisingly
>>> I got no complaint from gcc for subtracting from an unsigned value
>>> that is known to be 0.
>>>
>>> os_linux.cpp: In os::create_thread(), I also check here to make sure
>>> the size is not 0 after adding the guard page and aligning up, and
>>> subtract the os page size if it is 0.
>>>
>>> jvm.c: In JVM_StartThread(), on 32-bit platforms if the size is
>>> greater than UINT_MAX, then I set the size to UINT_MAX. Note it will
>>> later be rounded up to 0 in os::Posix::get_initial_stack_size(),
>>> which will result in subtracting the os page size to get the actual
>>> maximum allowed stack size.
>>>
>>> TooSmallStackSize.java: added test case for unaligned stack sizes.
>>>
>>> TestThreadStackSizes.java: New test. Creates new threads with every
>>> size up to 320k in 1k increments. Then creates threads with a few
>>> other sizes that can be problematic.
>>>
>>> thanks,
>>>
>>> Chris
>>>> Chris
>>>>
>>>>
>>>>>>
>>>>>>>     public Thread(ThreadGroup group, Runnable target, String name,
>>>>>>>                   long stackSize) {
>>>>>>>         init(group, target, name, stackSize);
>>>>>>>     }
>>>>>>>
>>>>>>> Fortunately we already force the stackSize to be at least
>>>>>>> _java_thread_min_stack_allowed. However, we don't do any OS page
>>>>>>> rounding on Mac OS X as noted above, and I was able to trigger the
>>>>>>> assert by creating a thread with size 257k.
>>>>>>
>>>>>> Note this means that OSX stack logic is broken because it will
>>>>>> currently be silently failing due to EINVAL!
>>>>> Yes, that is correct.
>>>>>
>>>>> Chris
>>>>>>
>>>>>>> I'll get another webrev out once I've made the needed fixes. I
>>>>>>> also have
>>>>>>> a new test I'd like to add.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/16/17 9:27 PM, Chris Plummer wrote:
>>>>>>>> Ok, time for a new webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>> The only thing that has changed since the first webrev are the
>>>>>>>> asserts
>>>>>>>> added to os_linux.cpp and os_bsd.cpp. And to summarize what we
>>>>>>>> discuss
>>>>>>>> already:
>>>>>>>>
>>>>>>>>  - The assert should never happen due to the stack size being too
>>>>>>>> small since it will be at least PTHREAD_STACK_MIN.
>>>>>>>>  - The assert should never happen due to an unaligned stack size
>>>>>>>> because we always align it to the page size.
>>>>>>>>  - Any assert would therefore be a VM bug and not due to user
>>>>>>>> error.
>>>>>>>>  - No fixing the java launcher. If the user specifies a stack
>>>>>>>> that is
>>>>>>>> too small, hotspot will already detect this. If the user
>>>>>>>> specifies a
>>>>>>>> stack size that is large enough but not page aligned, then we just
>>>>>>>> ignore any error (if the platform doth protest) and the user
>>>>>>>> gets a
>>>>>>>> main thread with a stack size set to whatever the OS default is.
>>>>>>>>
>>>>>>>> I still need to retest (I only ran TooSmallStackSize.java), but
>>>>>>>> figure
>>>>>>>> getting agreement on the changes first would be best before I
>>>>>>>> bog down
>>>>>>>> our testing resources.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 3/15/17 10:03 PM, Chris Plummer wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please review the following:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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);
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

123