(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
|

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

Chris Plummer
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,

On 16/03/2017 3: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

Change looks good.

> While working on 8175342 I noticed our stack size on xgene was 8mb even
> though I was specifying -Xss72k. It turns out the following code was
> failing:
>
>       pthread_attr_setstacksize(&attr, stack_size);

So these really should be checking return values, at least in debug
builds. But we can leave that until we refactor the thread startup code
into os_posix.cpp.

Thanks,
David
-----

> Although we computed a minimum stack size of 72k, so -Xss72k should be
> fine, pthreads on this platform requires the stack be at least 128k, so
> it failed the pthread_attr_setstacksize() call. The end result is
> pthread_attr_setstacksize() had no impact on the thread's stack size,
> and we ended up with the platform default of 8mb. The fix is to round up
> the following variables to PTHREAD_STACK_MIN after computing their new
> values:
>
>       _java_thread_min_stack_allowed
>       _compiler_thread_min_stack_allowed
>       _vm_internal_thread_min_stack_allowed
>
> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
> #defined, and this needs to be done before including OS header files. I
> noticed that on solaris we were using thr_min_stack() elsewhere instead
> of PTHREAD_STACK_MIN, so I decided to do the same with this fix. Either
> way is ugly (the #define or using thr_min_stack()).
>
> And speaking of the existing use of thr_min_stack(), I deleted it. It
> was being applied before any adjustments to the stack sizes had been
> made (rounding and adding red, yellow, and shadow zones). This mean the
> stack ended up being larger than necessary. With the above fix in place,
> we are now applying thr_min_stack() after recomputing the minimum stack
> sizes. If for any reason one of those stack sizes is now too small, the
> correct fix is to adjust the initial stack sizes, not apply
> thr_min_stack() to the initial stack sizes. However, it looks like no
> adjustment is needed. I did something close to our nightly testing on
> all affect platforms, and no new problems turned up.
>
> thanks,
>
> Chris
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/15/17 10:23 PM, David Holmes wrote:

> Hi Chris,
>
> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>> Hello,
>>
>> Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8176768
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>
> Change looks good.
>
>> While working on 8175342 I noticed our stack size on xgene was 8mb even
>> though I was specifying -Xss72k. It turns out the following code was
>> failing:
>>
>>       pthread_attr_setstacksize(&attr, stack_size);
>
> So these really should be checking return values, at least in debug
> builds. But we can leave that until we refactor the thread startup
> code into os_posix.cpp.
I considered adding checks. I wasn't sure if we should abort or just
print a warning if it failed. What refactoring is planned?

Chris

>
> Thanks,
> David
> -----
>
>> Although we computed a minimum stack size of 72k, so -Xss72k should be
>> fine, pthreads on this platform requires the stack be at least 128k, so
>> it failed the pthread_attr_setstacksize() call. The end result is
>> pthread_attr_setstacksize() had no impact on the thread's stack size,
>> and we ended up with the platform default of 8mb. The fix is to round up
>> the following variables to PTHREAD_STACK_MIN after computing their new
>> values:
>>
>>       _java_thread_min_stack_allowed
>>       _compiler_thread_min_stack_allowed
>>       _vm_internal_thread_min_stack_allowed
>>
>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>> #defined, and this needs to be done before including OS header files. I
>> noticed that on solaris we were using thr_min_stack() elsewhere instead
>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix. Either
>> way is ugly (the #define or using thr_min_stack()).
>>
>> And speaking of the existing use of thr_min_stack(), I deleted it. It
>> was being applied before any adjustments to the stack sizes had been
>> made (rounding and adding red, yellow, and shadow zones). This mean the
>> stack ended up being larger than necessary. With the above fix in place,
>> we are now applying thr_min_stack() after recomputing the minimum stack
>> sizes. If for any reason one of those stack sizes is now too small, the
>> correct fix is to adjust the initial stack sizes, not apply
>> thr_min_stack() to the initial stack sizes. However, it looks like no
>> adjustment is needed. I did something close to our nightly testing on
>> all affect platforms, and no new problems turned up.
>>
>> thanks,
>>
>> Chris


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 16/03/2017 3:51 PM, Chris Plummer wrote:

> On 3/15/17 10:23 PM, David Holmes wrote:
>> Hi Chris,
>>
>> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>> Hello,
>>>
>>> Please review the following:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>
>> Change looks good.
>>
>>> While working on 8175342 I noticed our stack size on xgene was 8mb even
>>> though I was specifying -Xss72k. It turns out the following code was
>>> failing:
>>>
>>>       pthread_attr_setstacksize(&attr, stack_size);
>>
>> So these really should be checking return values, at least in debug
>> builds. But we can leave that until we refactor the thread startup
>> code into os_posix.cpp.
> I considered adding checks. I wasn't sure if we should abort or just
> print a warning if it failed.

When we check pthread lib routines we use:

    int status = pthread_mutex_lock(_mutex);
    assert_status(status == 0, status, "mutex_lock");

This is for things that should only fail if we have a programming error.

> What refactoring is planned?

"Planned" might be a bit strong :) I was thinking of a number of
os_posix related cleanups for which issues exist, but also forgot that
some of our general clean-up RFE's have been closed as WNF :( I may do
some of them after hours anyway :)

David
-----

> Chris
>>
>> Thanks,
>> David
>> -----
>>
>>> Although we computed a minimum stack size of 72k, so -Xss72k should be
>>> fine, pthreads on this platform requires the stack be at least 128k, so
>>> it failed the pthread_attr_setstacksize() call. The end result is
>>> pthread_attr_setstacksize() had no impact on the thread's stack size,
>>> and we ended up with the platform default of 8mb. The fix is to round up
>>> the following variables to PTHREAD_STACK_MIN after computing their new
>>> values:
>>>
>>>       _java_thread_min_stack_allowed
>>>       _compiler_thread_min_stack_allowed
>>>       _vm_internal_thread_min_stack_allowed
>>>
>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>>> #defined, and this needs to be done before including OS header files. I
>>> noticed that on solaris we were using thr_min_stack() elsewhere instead
>>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix. Either
>>> way is ugly (the #define or using thr_min_stack()).
>>>
>>> And speaking of the existing use of thr_min_stack(), I deleted it. It
>>> was being applied before any adjustments to the stack sizes had been
>>> made (rounding and adding red, yellow, and shadow zones). This mean the
>>> stack ended up being larger than necessary. With the above fix in place,
>>> we are now applying thr_min_stack() after recomputing the minimum stack
>>> sizes. If for any reason one of those stack sizes is now too small, the
>>> correct fix is to adjust the initial stack sizes, not apply
>>> thr_min_stack() to the initial stack sizes. However, it looks like no
>>> adjustment is needed. I did something close to our nightly testing on
>>> all affect platforms, and no new problems turned up.
>>>
>>> thanks,
>>>
>>> Chris
>
>
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/15/17 11:11 PM, David Holmes wrote:

> On 16/03/2017 3:51 PM, Chris Plummer wrote:
>> On 3/15/17 10:23 PM, David Holmes wrote:
>>> Hi Chris,
>>>
>>> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>>> Hello,
>>>>
>>>> Please review the following:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>
>>> Change looks good.
>>>
>>>> While working on 8175342 I noticed our stack size on xgene was 8mb
>>>> even
>>>> though I was specifying -Xss72k. It turns out the following code was
>>>> failing:
>>>>
>>>>       pthread_attr_setstacksize(&attr, stack_size);
>>>
>>> So these really should be checking return values, at least in debug
>>> builds. But we can leave that until we refactor the thread startup
>>> code into os_posix.cpp.
>> I considered adding checks. I wasn't sure if we should abort or just
>> print a warning if it failed.
>
> When we check pthread lib routines we use:
>
>    int status = pthread_mutex_lock(_mutex);
>    assert_status(status == 0, status, "mutex_lock");
>
> This is for things that should only fail if we have a programming error.
Ok, but this is in the launcher, so I'll need to just use the built-in
assert(). I'll add that if want.

Chris

>
>> What refactoring is planned?
>
> "Planned" might be a bit strong :) I was thinking of a number of
> os_posix related cleanups for which issues exist, but also forgot that
> some of our general clean-up RFE's have been closed as WNF :( I may do
> some of them after hours anyway :)
>
> David
> -----
>
>> Chris
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Although we computed a minimum stack size of 72k, so -Xss72k should be
>>>> fine, pthreads on this platform requires the stack be at least
>>>> 128k, so
>>>> it failed the pthread_attr_setstacksize() call. The end result is
>>>> pthread_attr_setstacksize() had no impact on the thread's stack size,
>>>> and we ended up with the platform default of 8mb. The fix is to
>>>> round up
>>>> the following variables to PTHREAD_STACK_MIN after computing their new
>>>> values:
>>>>
>>>>       _java_thread_min_stack_allowed
>>>>       _compiler_thread_min_stack_allowed
>>>>       _vm_internal_thread_min_stack_allowed
>>>>
>>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>>>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>>>> #defined, and this needs to be done before including OS header
>>>> files. I
>>>> noticed that on solaris we were using thr_min_stack() elsewhere
>>>> instead
>>>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix.
>>>> Either
>>>> way is ugly (the #define or using thr_min_stack()).
>>>>
>>>> And speaking of the existing use of thr_min_stack(), I deleted it. It
>>>> was being applied before any adjustments to the stack sizes had been
>>>> made (rounding and adding red, yellow, and shadow zones). This mean
>>>> the
>>>> stack ended up being larger than necessary. With the above fix in
>>>> place,
>>>> we are now applying thr_min_stack() after recomputing the minimum
>>>> stack
>>>> sizes. If for any reason one of those stack sizes is now too small,
>>>> the
>>>> correct fix is to adjust the initial stack sizes, not apply
>>>> thr_min_stack() to the initial stack sizes. However, it looks like no
>>>> adjustment is needed. I did something close to our nightly testing on
>>>> all affect platforms, and no new problems turned up.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>
>>

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 16/03/2017 4:14 PM, Chris Plummer wrote:

> On 3/15/17 11:11 PM, David Holmes wrote:
>> On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>> On 3/15/17 10:23 PM, David Holmes wrote:
>>>> Hi Chris,
>>>>
>>>> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>>>> Hello,
>>>>>
>>>>> Please review the following:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>
>>>> Change looks good.
>>>>
>>>>> While working on 8175342 I noticed our stack size on xgene was 8mb
>>>>> even
>>>>> though I was specifying -Xss72k. It turns out the following code was
>>>>> failing:
>>>>>
>>>>>       pthread_attr_setstacksize(&attr, stack_size);
>>>>
>>>> So these really should be checking return values, at least in debug
>>>> builds. But we can leave that until we refactor the thread startup
>>>> code into os_posix.cpp.
>>> I considered adding checks. I wasn't sure if we should abort or just
>>> print a warning if it failed.
>>
>> When we check pthread lib routines we use:
>>
>>    int status = pthread_mutex_lock(_mutex);
>>    assert_status(status == 0, status, "mutex_lock");
>>
>> This is for things that should only fail if we have a programming error.
> Ok, but this is in the launcher, so I'll need to just use the built-in
> assert(). I'll add that if want.

Oops! I was forgetting that. Need to be consistent with launcher error
checking or lack thereof. And ignore refactoring comments - not relevant.

David

> Chris
>>
>>> What refactoring is planned?
>>
>> "Planned" might be a bit strong :) I was thinking of a number of
>> os_posix related cleanups for which issues exist, but also forgot that
>> some of our general clean-up RFE's have been closed as WNF :( I may do
>> some of them after hours anyway :)
>>
>> David
>> -----
>>
>>> Chris
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Although we computed a minimum stack size of 72k, so -Xss72k should be
>>>>> fine, pthreads on this platform requires the stack be at least
>>>>> 128k, so
>>>>> it failed the pthread_attr_setstacksize() call. The end result is
>>>>> pthread_attr_setstacksize() had no impact on the thread's stack size,
>>>>> and we ended up with the platform default of 8mb. The fix is to
>>>>> round up
>>>>> the following variables to PTHREAD_STACK_MIN after computing their new
>>>>> values:
>>>>>
>>>>>       _java_thread_min_stack_allowed
>>>>>       _compiler_thread_min_stack_allowed
>>>>>       _vm_internal_thread_min_stack_allowed
>>>>>
>>>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>>>>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>>>>> #defined, and this needs to be done before including OS header
>>>>> files. I
>>>>> noticed that on solaris we were using thr_min_stack() elsewhere
>>>>> instead
>>>>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix.
>>>>> Either
>>>>> way is ugly (the #define or using thr_min_stack()).
>>>>>
>>>>> And speaking of the existing use of thr_min_stack(), I deleted it. It
>>>>> was being applied before any adjustments to the stack sizes had been
>>>>> made (rounding and adding red, yellow, and shadow zones). This mean
>>>>> the
>>>>> stack ended up being larger than necessary. With the above fix in
>>>>> place,
>>>>> we are now applying thr_min_stack() after recomputing the minimum
>>>>> stack
>>>>> sizes. If for any reason one of those stack sizes is now too small,
>>>>> the
>>>>> correct fix is to adjust the initial stack sizes, not apply
>>>>> thr_min_stack() to the initial stack sizes. However, it looks like no
>>>>> adjustment is needed. I did something close to our nightly testing on
>>>>> all affect platforms, and no new problems turned up.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>
>>>
>
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/15/17 11:18 PM, David Holmes wrote:

> On 16/03/2017 4:14 PM, Chris Plummer wrote:
>> On 3/15/17 11:11 PM, David Holmes wrote:
>>> On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>> On 3/15/17 10:23 PM, David Holmes wrote:
>>>>> Hi Chris,
>>>>>
>>>>> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>
>>>>>
>>>>> Change looks good.
>>>>>
>>>>>> While working on 8175342 I noticed our stack size on xgene was 8mb
>>>>>> even
>>>>>> though I was specifying -Xss72k. It turns out the following code was
>>>>>> failing:
>>>>>>
>>>>>>       pthread_attr_setstacksize(&attr, stack_size);
>>>>>
>>>>> So these really should be checking return values, at least in debug
>>>>> builds. But we can leave that until we refactor the thread startup
>>>>> code into os_posix.cpp.
>>>> I considered adding checks. I wasn't sure if we should abort or just
>>>> print a warning if it failed.
>>>
>>> When we check pthread lib routines we use:
>>>
>>>    int status = pthread_mutex_lock(_mutex);
>>>    assert_status(status == 0, status, "mutex_lock");
>>>
>>> This is for things that should only fail if we have a programming
>>> error.
>> Ok, but this is in the launcher, so I'll need to just use the built-in
>> assert(). I'll add that if want.
>
> Oops! I was forgetting that. Need to be consistent with launcher error
> checking or lack thereof. And ignore refactoring comments - not relevant.
So don't add the error check?

>
> David
>
>> Chris
>>>
>>>> What refactoring is planned?
>>>
>>> "Planned" might be a bit strong :) I was thinking of a number of
>>> os_posix related cleanups for which issues exist, but also forgot that
>>> some of our general clean-up RFE's have been closed as WNF :( I may do
>>> some of them after hours anyway :)
>>>
>>> David
>>> -----
>>>
>>>> Chris
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Although we computed a minimum stack size of 72k, so -Xss72k
>>>>>> should be
>>>>>> fine, pthreads on this platform requires the stack be at least
>>>>>> 128k, so
>>>>>> it failed the pthread_attr_setstacksize() call. The end result is
>>>>>> pthread_attr_setstacksize() had no impact on the thread's stack
>>>>>> size,
>>>>>> and we ended up with the platform default of 8mb. The fix is to
>>>>>> round up
>>>>>> the following variables to PTHREAD_STACK_MIN after computing
>>>>>> their new
>>>>>> values:
>>>>>>
>>>>>>       _java_thread_min_stack_allowed
>>>>>>       _compiler_thread_min_stack_allowed
>>>>>>       _vm_internal_thread_min_stack_allowed
>>>>>>
>>>>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>>>>>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>>>>>> #defined, and this needs to be done before including OS header
>>>>>> files. I
>>>>>> noticed that on solaris we were using thr_min_stack() elsewhere
>>>>>> instead
>>>>>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix.
>>>>>> Either
>>>>>> way is ugly (the #define or using thr_min_stack()).
>>>>>>
>>>>>> And speaking of the existing use of thr_min_stack(), I deleted
>>>>>> it. It
>>>>>> was being applied before any adjustments to the stack sizes had been
>>>>>> made (rounding and adding red, yellow, and shadow zones). This mean
>>>>>> the
>>>>>> stack ended up being larger than necessary. With the above fix in
>>>>>> place,
>>>>>> we are now applying thr_min_stack() after recomputing the minimum
>>>>>> stack
>>>>>> sizes. If for any reason one of those stack sizes is now too small,
>>>>>> the
>>>>>> correct fix is to adjust the initial stack sizes, not apply
>>>>>> thr_min_stack() to the initial stack sizes. However, it looks
>>>>>> like no
>>>>>> adjustment is needed. I did something close to our nightly
>>>>>> testing on
>>>>>> all affect platforms, and no new problems turned up.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>
>>>>
>>

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 16/03/2017 4:33 PM, Chris Plummer wrote:

> On 3/15/17 11:18 PM, David Holmes wrote:
>> On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>> On 3/15/17 11:11 PM, David Holmes wrote:
>>>> On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>> On 3/15/17 10:23 PM, David Holmes wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>>>>
>>>>>>
>>>>>> Change looks good.
>>>>>>
>>>>>>> While working on 8175342 I noticed our stack size on xgene was 8mb
>>>>>>> even
>>>>>>> though I was specifying -Xss72k. It turns out the following code was
>>>>>>> failing:
>>>>>>>
>>>>>>>       pthread_attr_setstacksize(&attr, stack_size);
>>>>>>
>>>>>> So these really should be checking return values, at least in debug
>>>>>> builds. But we can leave that until we refactor the thread startup
>>>>>> code into os_posix.cpp.
>>>>> I considered adding checks. I wasn't sure if we should abort or just
>>>>> print a warning if it failed.
>>>>
>>>> When we check pthread lib routines we use:
>>>>
>>>>    int status = pthread_mutex_lock(_mutex);
>>>>    assert_status(status == 0, status, "mutex_lock");
>>>>
>>>> This is for things that should only fail if we have a programming
>>>> error.
>>> Ok, but this is in the launcher, so I'll need to just use the built-in
>>> assert(). I'll add that if want.
>>
>> Oops! I was forgetting that. Need to be consistent with launcher error
>> checking or lack thereof. And ignore refactoring comments - not relevant.
> So don't add the error check?

Given there is no error checking, or assertions, in those files I
reluctantly have to say leave it out.

Thanks,
David
-----

>>
>> David
>>
>>> Chris
>>>>
>>>>> What refactoring is planned?
>>>>
>>>> "Planned" might be a bit strong :) I was thinking of a number of
>>>> os_posix related cleanups for which issues exist, but also forgot that
>>>> some of our general clean-up RFE's have been closed as WNF :( I may do
>>>> some of them after hours anyway :)
>>>>
>>>> David
>>>> -----
>>>>
>>>>> Chris
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> Although we computed a minimum stack size of 72k, so -Xss72k
>>>>>>> should be
>>>>>>> fine, pthreads on this platform requires the stack be at least
>>>>>>> 128k, so
>>>>>>> it failed the pthread_attr_setstacksize() call. The end result is
>>>>>>> pthread_attr_setstacksize() had no impact on the thread's stack
>>>>>>> size,
>>>>>>> and we ended up with the platform default of 8mb. The fix is to
>>>>>>> round up
>>>>>>> the following variables to PTHREAD_STACK_MIN after computing
>>>>>>> their new
>>>>>>> values:
>>>>>>>
>>>>>>>       _java_thread_min_stack_allowed
>>>>>>>       _compiler_thread_min_stack_allowed
>>>>>>>       _vm_internal_thread_min_stack_allowed
>>>>>>>
>>>>>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>>>>>>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>>>>>>> #defined, and this needs to be done before including OS header
>>>>>>> files. I
>>>>>>> noticed that on solaris we were using thr_min_stack() elsewhere
>>>>>>> instead
>>>>>>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix.
>>>>>>> Either
>>>>>>> way is ugly (the #define or using thr_min_stack()).
>>>>>>>
>>>>>>> And speaking of the existing use of thr_min_stack(), I deleted
>>>>>>> it. It
>>>>>>> was being applied before any adjustments to the stack sizes had been
>>>>>>> made (rounding and adding red, yellow, and shadow zones). This mean
>>>>>>> the
>>>>>>> stack ended up being larger than necessary. With the above fix in
>>>>>>> place,
>>>>>>> we are now applying thr_min_stack() after recomputing the minimum
>>>>>>> stack
>>>>>>> sizes. If for any reason one of those stack sizes is now too small,
>>>>>>> the
>>>>>>> correct fix is to adjust the initial stack sizes, not apply
>>>>>>> thr_min_stack() to the initial stack sizes. However, it looks
>>>>>>> like no
>>>>>>> adjustment is needed. I did something close to our nightly
>>>>>>> testing on
>>>>>>> all affect platforms, and no new problems turned up.
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>
>>>>>
>>>
>
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, David,

the change looks good.

I see that in the launcher we require a minimum stack size across all
platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust for
PTHREAD_STACK_MIN) there?

I do not understand, why does error checking in the hotspot have to be
consistent with the launcher? What prevents us from asserting in the
hotspot - or at least print a warning? Note that in the hotspot, there is
already UL logging ("os", "thread") after pthread_create() in the platform
files, so the least we could do is add a warning log output case
ppthread_attr_setstacksize
fails.

If we ever refactor this coding, could we rename the variables holding the
base stack size requirement for java frames - in all its incarnations in
all the os_cpu files - to be renamed to something different? It is a bit
confusing to have a variable which at different times in VM life means
different things (before and after the call to
os::Posix::set_minimum_stack_sizes()).
Or, at least, rename "set_minimum_stack_sizes" to something like
"adjust_minimum_stack_sizes" which makes the intent clearer.

Kind Regards, Thomas

On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <[hidden email]>
wrote:

> On 16/03/2017 4:33 PM, Chris Plummer wrote:
>
>> On 3/15/17 11:18 PM, David Holmes wrote:
>>
>>> On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>
>>>> On 3/15/17 11:11 PM, David Holmes wrote:
>>>>
>>>>> On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>>
>>>>>> On 3/15/17 10:23 PM, David Holmes wrote:
>>>>>>
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webr
>>>>>>>> ev.hotspot
>>>>>>>>
>>>>>>>>
>>>>>>> Change looks good.
>>>>>>>
>>>>>>> While working on 8175342 I noticed our stack size on xgene was 8mb
>>>>>>>> even
>>>>>>>> though I was specifying -Xss72k. It turns out the following code was
>>>>>>>> failing:
>>>>>>>>
>>>>>>>>       pthread_attr_setstacksize(&attr, stack_size);
>>>>>>>>
>>>>>>>
>>>>>>> So these really should be checking return values, at least in debug
>>>>>>> builds. But we can leave that until we refactor the thread startup
>>>>>>> code into os_posix.cpp.
>>>>>>>
>>>>>> I considered adding checks. I wasn't sure if we should abort or just
>>>>>> print a warning if it failed.
>>>>>>
>>>>>
>>>>> When we check pthread lib routines we use:
>>>>>
>>>>>    int status = pthread_mutex_lock(_mutex);
>>>>>    assert_status(status == 0, status, "mutex_lock");
>>>>>
>>>>> This is for things that should only fail if we have a programming
>>>>> error.
>>>>>
>>>> Ok, but this is in the launcher, so I'll need to just use the built-in
>>>> assert(). I'll add that if want.
>>>>
>>>
>>> Oops! I was forgetting that. Need to be consistent with launcher error
>>> checking or lack thereof. And ignore refactoring comments - not relevant.
>>>
>> So don't add the error check?
>>
>
> Given there is no error checking, or assertions, in those files I
> reluctantly have to say leave it out.
>
> Thanks,
> David
> -----
>
>
>
>>> David
>>>
>>> Chris
>>>>
>>>>>
>>>>> What refactoring is planned?
>>>>>>
>>>>>
>>>>> "Planned" might be a bit strong :) I was thinking of a number of
>>>>> os_posix related cleanups for which issues exist, but also forgot that
>>>>> some of our general clean-up RFE's have been closed as WNF :( I may do
>>>>> some of them after hours anyway :)
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>> Chris
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>> Although we computed a minimum stack size of 72k, so -Xss72k
>>>>>>>> should be
>>>>>>>> fine, pthreads on this platform requires the stack be at least
>>>>>>>> 128k, so
>>>>>>>> it failed the pthread_attr_setstacksize() call. The end result is
>>>>>>>> pthread_attr_setstacksize() had no impact on the thread's stack
>>>>>>>> size,
>>>>>>>> and we ended up with the platform default of 8mb. The fix is to
>>>>>>>> round up
>>>>>>>> the following variables to PTHREAD_STACK_MIN after computing
>>>>>>>> their new
>>>>>>>> values:
>>>>>>>>
>>>>>>>>       _java_thread_min_stack_allowed
>>>>>>>>       _compiler_thread_min_stack_allowed
>>>>>>>>       _vm_internal_thread_min_stack_allowed
>>>>>>>>
>>>>>>>> For solaris, there was an issue using PTHREAD_STACK_MIN. You need to
>>>>>>>> #define _POSIX_C_SOURCE >= 199506L in order to get PTHREAD_STACK_MIN
>>>>>>>> #defined, and this needs to be done before including OS header
>>>>>>>> files. I
>>>>>>>> noticed that on solaris we were using thr_min_stack() elsewhere
>>>>>>>> instead
>>>>>>>> of PTHREAD_STACK_MIN, so I decided to do the same with this fix.
>>>>>>>> Either
>>>>>>>> way is ugly (the #define or using thr_min_stack()).
>>>>>>>>
>>>>>>>> And speaking of the existing use of thr_min_stack(), I deleted
>>>>>>>> it. It
>>>>>>>> was being applied before any adjustments to the stack sizes had been
>>>>>>>> made (rounding and adding red, yellow, and shadow zones). This mean
>>>>>>>> the
>>>>>>>> stack ended up being larger than necessary. With the above fix in
>>>>>>>> place,
>>>>>>>> we are now applying thr_min_stack() after recomputing the minimum
>>>>>>>> stack
>>>>>>>> sizes. If for any reason one of those stack sizes is now too small,
>>>>>>>> the
>>>>>>>> correct fix is to adjust the initial stack sizes, not apply
>>>>>>>> thr_min_stack() to the initial stack sizes. However, it looks
>>>>>>>> like no
>>>>>>>> adjustment is needed. I did something close to our nightly
>>>>>>>> testing on
>>>>>>>> all affect platforms, and no new problems turned up.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
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 16/03/2017 6:30 PM, Thomas Stüfe wrote:

> Hi Chris, David,
>
> the change looks good.
>
> I see that in the launcher we require a minimum stack size across all
> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust for
> PTHREAD_STACK_MIN) there?
>
> I do not understand, why does error checking in the hotspot have to be
> consistent with the launcher? What prevents us from asserting in the
> hotspot - or at least print a warning? Note that in the hotspot, there
> is already UL logging ("os", "thread") after pthread_create() in the
> platform files, so the least we could do is add a warning log output
> case ppthread_attr_setstacksize fails.

Sorry I'm getting this group of bugs all muddled up.

Chris: this issue does affect hotspot and the launcher (potentially).

Ideally both should be checking for failures in the pthread calls but
neither do so. Hotspot at least does so in some places but not in a lot
of others.

pthread_create is different in hotspot because failure can happen easily
and we need to detect it and report it (via an exception and also via
UL). The other pthread calls are not expected to fail under "normal"
conditions but only due to a programming error. Those calls should at
least be checked in debug builds as we already do in places with
assert_status.

The launcher code doesn't do any error checking at all (but again
pthread_create is a special case).

David
-----

> If we ever refactor this coding, could we rename the variables holding
> the base stack size requirement for java frames - in all its
> incarnations in all the os_cpu files - to be renamed to something
> different? It is a bit confusing to have a variable which at different
> times in VM life means different things (before and after the call
> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
> "set_minimum_stack_sizes" to something like "adjust_minimum_stack_sizes"
> which makes the intent clearer.
>
> Kind Regards, Thomas
>
> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>
>         On 3/15/17 11:18 PM, David Holmes wrote:
>
>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>
>                 On 3/15/17 11:11 PM, David Holmes wrote:
>
>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>
>                         On 3/15/17 10:23 PM, David Holmes wrote:
>
>                             Hi Chris,
>
>                             On 16/03/2017 3:03 PM, Chris Plummer wrote:
>
>                                 Hello,
>
>                                 Please review the following:
>
>                                 https://bugs.openjdk.java.net/browse/JDK-8176768
>                                 <https://bugs.openjdk.java.net/browse/JDK-8176768>
>                                 http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>                                 <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>
>
>                             Change looks good.
>
>                                 While working on 8175342 I noticed our
>                                 stack size on xgene was 8mb
>                                 even
>                                 though I was specifying -Xss72k. It
>                                 turns out the following code was
>                                 failing:
>
>                                       pthread_attr_setstacksize(&attr,
>                                 stack_size);
>
>
>                             So these really should be checking return
>                             values, at least in debug
>                             builds. But we can leave that until we
>                             refactor the thread startup
>                             code into os_posix.cpp.
>
>                         I considered adding checks. I wasn't sure if we
>                         should abort or just
>                         print a warning if it failed.
>
>
>                     When we check pthread lib routines we use:
>
>                        int status = pthread_mutex_lock(_mutex);
>                        assert_status(status == 0, status, "mutex_lock");
>
>                     This is for things that should only fail if we have
>                     a programming
>                     error.
>
>                 Ok, but this is in the launcher, so I'll need to just
>                 use the built-in
>                 assert(). I'll add that if want.
>
>
>             Oops! I was forgetting that. Need to be consistent with
>             launcher error
>             checking or lack thereof. And ignore refactoring comments -
>             not relevant.
>
>         So don't add the error check?
>
>
>     Given there is no error checking, or assertions, in those files I
>     reluctantly have to say leave it out.
>
>     Thanks,
>     David
>     -----
>
>
>
>             David
>
>                 Chris
>
>
>                         What refactoring is planned?
>
>
>                     "Planned" might be a bit strong :) I was thinking of
>                     a number of
>                     os_posix related cleanups for which issues exist,
>                     but also forgot that
>                     some of our general clean-up RFE's have been closed
>                     as WNF :( I may do
>                     some of them after hours anyway :)
>
>                     David
>                     -----
>
>                         Chris
>
>
>                             Thanks,
>                             David
>                             -----
>
>                                 Although we computed a minimum stack
>                                 size of 72k, so -Xss72k
>                                 should be
>                                 fine, pthreads on this platform requires
>                                 the stack be at least
>                                 128k, so
>                                 it failed the
>                                 pthread_attr_setstacksize() call. The
>                                 end result is
>                                 pthread_attr_setstacksize() had no
>                                 impact on the thread's stack
>                                 size,
>                                 and we ended up with the platform
>                                 default of 8mb. The fix is to
>                                 round up
>                                 the following variables to
>                                 PTHREAD_STACK_MIN after computing
>                                 their new
>                                 values:
>
>                                       _java_thread_min_stack_allowed
>                                       _compiler_thread_min_stack_allowed
>                                       _vm_internal_thread_min_stack_allowed
>
>                                 For solaris, there was an issue using
>                                 PTHREAD_STACK_MIN. You need to
>                                 #define _POSIX_C_SOURCE >= 199506L in
>                                 order to get PTHREAD_STACK_MIN
>                                 #defined, and this needs to be done
>                                 before including OS header
>                                 files. I
>                                 noticed that on solaris we were using
>                                 thr_min_stack() elsewhere
>                                 instead
>                                 of PTHREAD_STACK_MIN, so I decided to do
>                                 the same with this fix.
>                                 Either
>                                 way is ugly (the #define or using
>                                 thr_min_stack()).
>
>                                 And speaking of the existing use of
>                                 thr_min_stack(), I deleted
>                                 it. It
>                                 was being applied before any adjustments
>                                 to the stack sizes had been
>                                 made (rounding and adding red, yellow,
>                                 and shadow zones). This mean
>                                 the
>                                 stack ended up being larger than
>                                 necessary. With the above fix in
>                                 place,
>                                 we are now applying thr_min_stack()
>                                 after recomputing the minimum
>                                 stack
>                                 sizes. If for any reason one of those
>                                 stack sizes is now too small,
>                                 the
>                                 correct fix is to adjust the initial
>                                 stack sizes, not apply
>                                 thr_min_stack() to the initial stack
>                                 sizes. However, it looks
>                                 like no
>                                 adjustment is needed. I did something
>                                 close to our nightly
>                                 testing on
>                                 all affect platforms, and no new
>                                 problems turned up.
>
>                                 thanks,
>
>                                 Chris
>
>
>
>
>
>
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 2:16 AM, David Holmes wrote:

> On 16/03/2017 6:30 PM, Thomas Stüfe wrote:
>> Hi Chris, David,
>>
>> the change looks good.
>>
>> I see that in the launcher we require a minimum stack size across all
>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust for
>> PTHREAD_STACK_MIN) there?
>>
>> I do not understand, why does error checking in the hotspot have to be
>> consistent with the launcher? What prevents us from asserting in the
>> hotspot - or at least print a warning? Note that in the hotspot, there
>> is already UL logging ("os", "thread") after pthread_create() in the
>> platform files, so the least we could do is add a warning log output
>> case ppthread_attr_setstacksize fails.
>
> Sorry I'm getting this group of bugs all muddled up.
>
> Chris: this issue does affect hotspot and the launcher (potentially).
>
> Ideally both should be checking for failures in the pthread calls but
> neither do so. Hotspot at least does so in some places but not in a
> lot of others.
>
> pthread_create is different in hotspot because failure can happen
> easily and we need to detect it and report it (via an exception and
> also via UL). The other pthread calls are not expected to fail under
> "normal" conditions but only due to a programming error. Those calls
> should at least be checked in debug builds as we already do in places
> with assert_status.
>
> The launcher code doesn't do any error checking at all (but again
> pthread_create is a special case).
Are you just referring to the pthread related error checking? It does do
other error checking.

Chris

>
> David
> -----
>
>> If we ever refactor this coding, could we rename the variables holding
>> the base stack size requirement for java frames - in all its
>> incarnations in all the os_cpu files - to be renamed to something
>> different? It is a bit confusing to have a variable which at different
>> times in VM life means different things (before and after the call
>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>> "set_minimum_stack_sizes" to something like "adjust_minimum_stack_sizes"
>> which makes the intent clearer.
>>
>> Kind Regards, Thomas
>>
>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>
>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>
>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>
>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>
>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>
>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>
>>                             Hi Chris,
>>
>>                             On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>
>>                                 Hello,
>>
>>                                 Please review the following:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8176768
>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>
>>
>>                             Change looks good.
>>
>>                                 While working on 8175342 I noticed our
>>                                 stack size on xgene was 8mb
>>                                 even
>>                                 though I was specifying -Xss72k. It
>>                                 turns out the following code was
>>                                 failing:
>>
>> pthread_attr_setstacksize(&attr,
>>                                 stack_size);
>>
>>
>>                             So these really should be checking return
>>                             values, at least in debug
>>                             builds. But we can leave that until we
>>                             refactor the thread startup
>>                             code into os_posix.cpp.
>>
>>                         I considered adding checks. I wasn't sure if we
>>                         should abort or just
>>                         print a warning if it failed.
>>
>>
>>                     When we check pthread lib routines we use:
>>
>>                        int status = pthread_mutex_lock(_mutex);
>>                        assert_status(status == 0, status, "mutex_lock");
>>
>>                     This is for things that should only fail if we have
>>                     a programming
>>                     error.
>>
>>                 Ok, but this is in the launcher, so I'll need to just
>>                 use the built-in
>>                 assert(). I'll add that if want.
>>
>>
>>             Oops! I was forgetting that. Need to be consistent with
>>             launcher error
>>             checking or lack thereof. And ignore refactoring comments -
>>             not relevant.
>>
>>         So don't add the error check?
>>
>>
>>     Given there is no error checking, or assertions, in those files I
>>     reluctantly have to say leave it out.
>>
>>     Thanks,
>>     David
>>     -----
>>
>>
>>
>>             David
>>
>>                 Chris
>>
>>
>>                         What refactoring is planned?
>>
>>
>>                     "Planned" might be a bit strong :) I was thinking of
>>                     a number of
>>                     os_posix related cleanups for which issues exist,
>>                     but also forgot that
>>                     some of our general clean-up RFE's have been closed
>>                     as WNF :( I may do
>>                     some of them after hours anyway :)
>>
>>                     David
>>                     -----
>>
>>                         Chris
>>
>>
>>                             Thanks,
>>                             David
>>                             -----
>>
>>                                 Although we computed a minimum stack
>>                                 size of 72k, so -Xss72k
>>                                 should be
>>                                 fine, pthreads on this platform requires
>>                                 the stack be at least
>>                                 128k, so
>>                                 it failed the
>>                                 pthread_attr_setstacksize() call. The
>>                                 end result is
>>                                 pthread_attr_setstacksize() had no
>>                                 impact on the thread's stack
>>                                 size,
>>                                 and we ended up with the platform
>>                                 default of 8mb. The fix is to
>>                                 round up
>>                                 the following variables to
>>                                 PTHREAD_STACK_MIN after computing
>>                                 their new
>>                                 values:
>>
>> _java_thread_min_stack_allowed
>> _compiler_thread_min_stack_allowed
>> _vm_internal_thread_min_stack_allowed
>>
>>                                 For solaris, there was an issue using
>>                                 PTHREAD_STACK_MIN. You need to
>>                                 #define _POSIX_C_SOURCE >= 199506L in
>>                                 order to get PTHREAD_STACK_MIN
>>                                 #defined, and this needs to be done
>>                                 before including OS header
>>                                 files. I
>>                                 noticed that on solaris we were using
>>                                 thr_min_stack() elsewhere
>>                                 instead
>>                                 of PTHREAD_STACK_MIN, so I decided to do
>>                                 the same with this fix.
>>                                 Either
>>                                 way is ugly (the #define or using
>>                                 thr_min_stack()).
>>
>>                                 And speaking of the existing use of
>>                                 thr_min_stack(), I deleted
>>                                 it. It
>>                                 was being applied before any adjustments
>>                                 to the stack sizes had been
>>                                 made (rounding and adding red, yellow,
>>                                 and shadow zones). This mean
>>                                 the
>>                                 stack ended up being larger than
>>                                 necessary. With the above fix in
>>                                 place,
>>                                 we are now applying thr_min_stack()
>>                                 after recomputing the minimum
>>                                 stack
>>                                 sizes. If for any reason one of those
>>                                 stack sizes is now too small,
>>                                 the
>>                                 correct fix is to adjust the initial
>>                                 stack sizes, not apply
>>                                 thr_min_stack() to the initial stack
>>                                 sizes. However, it looks
>>                                 like no
>>                                 adjustment is needed. I did something
>>                                 close to our nightly
>>                                 testing on
>>                                 all affect platforms, and no new
>>                                 problems turned up.
>>
>>                                 thanks,
>>
>>                                 Chris
>>
>>
>>
>>
>>
>>


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 17/03/2017 3:49 AM, Chris Plummer wrote:

> On 3/16/17 2:16 AM, David Holmes wrote:
>> On 16/03/2017 6:30 PM, Thomas Stüfe wrote:
>>> Hi Chris, David,
>>>
>>> the change looks good.
>>>
>>> I see that in the launcher we require a minimum stack size across all
>>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust for
>>> PTHREAD_STACK_MIN) there?
>>>
>>> I do not understand, why does error checking in the hotspot have to be
>>> consistent with the launcher? What prevents us from asserting in the
>>> hotspot - or at least print a warning? Note that in the hotspot, there
>>> is already UL logging ("os", "thread") after pthread_create() in the
>>> platform files, so the least we could do is add a warning log output
>>> case ppthread_attr_setstacksize fails.
>>
>> Sorry I'm getting this group of bugs all muddled up.
>>
>> Chris: this issue does affect hotspot and the launcher (potentially).
>>
>> Ideally both should be checking for failures in the pthread calls but
>> neither do so. Hotspot at least does so in some places but not in a
>> lot of others.
>>
>> pthread_create is different in hotspot because failure can happen
>> easily and we need to detect it and report it (via an exception and
>> also via UL). The other pthread calls are not expected to fail under
>> "normal" conditions but only due to a programming error. Those calls
>> should at least be checked in debug builds as we already do in places
>> with assert_status.
>>
>> The launcher code doesn't do any error checking at all (but again
>> pthread_create is a special case).
> Are you just referring to the pthread related error checking? It does do
> other error checking.

pthread error checking.

So trying to think this through ...

If the user specifies a too small, or  unaligned-to-page-size, -Xss
value the pthread_setstacksize() in the launcher will silently fail and
the main thread will get the default stack of 8M. It will then load the
VM which will then check the -Xss value, which will do its own validity
checking.

That seems like quite a reasonable position for the launcher to take.

David
-----

>
> Chris
>>
>> David
>> -----
>>
>>> If we ever refactor this coding, could we rename the variables holding
>>> the base stack size requirement for java frames - in all its
>>> incarnations in all the os_cpu files - to be renamed to something
>>> different? It is a bit confusing to have a variable which at different
>>> times in VM life means different things (before and after the call
>>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>>> "set_minimum_stack_sizes" to something like "adjust_minimum_stack_sizes"
>>> which makes the intent clearer.
>>>
>>> Kind Regards, Thomas
>>>
>>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>>
>>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>>
>>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>
>>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>>
>>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>
>>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>>
>>>                             Hi Chris,
>>>
>>>                             On 16/03/2017 3:03 PM, Chris Plummer wrote:
>>>
>>>                                 Hello,
>>>
>>>                                 Please review the following:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>>
>>>
>>>                             Change looks good.
>>>
>>>                                 While working on 8175342 I noticed our
>>>                                 stack size on xgene was 8mb
>>>                                 even
>>>                                 though I was specifying -Xss72k. It
>>>                                 turns out the following code was
>>>                                 failing:
>>>
>>> pthread_attr_setstacksize(&attr,
>>>                                 stack_size);
>>>
>>>
>>>                             So these really should be checking return
>>>                             values, at least in debug
>>>                             builds. But we can leave that until we
>>>                             refactor the thread startup
>>>                             code into os_posix.cpp.
>>>
>>>                         I considered adding checks. I wasn't sure if we
>>>                         should abort or just
>>>                         print a warning if it failed.
>>>
>>>
>>>                     When we check pthread lib routines we use:
>>>
>>>                        int status = pthread_mutex_lock(_mutex);
>>>                        assert_status(status == 0, status, "mutex_lock");
>>>
>>>                     This is for things that should only fail if we have
>>>                     a programming
>>>                     error.
>>>
>>>                 Ok, but this is in the launcher, so I'll need to just
>>>                 use the built-in
>>>                 assert(). I'll add that if want.
>>>
>>>
>>>             Oops! I was forgetting that. Need to be consistent with
>>>             launcher error
>>>             checking or lack thereof. And ignore refactoring comments -
>>>             not relevant.
>>>
>>>         So don't add the error check?
>>>
>>>
>>>     Given there is no error checking, or assertions, in those files I
>>>     reluctantly have to say leave it out.
>>>
>>>     Thanks,
>>>     David
>>>     -----
>>>
>>>
>>>
>>>             David
>>>
>>>                 Chris
>>>
>>>
>>>                         What refactoring is planned?
>>>
>>>
>>>                     "Planned" might be a bit strong :) I was thinking of
>>>                     a number of
>>>                     os_posix related cleanups for which issues exist,
>>>                     but also forgot that
>>>                     some of our general clean-up RFE's have been closed
>>>                     as WNF :( I may do
>>>                     some of them after hours anyway :)
>>>
>>>                     David
>>>                     -----
>>>
>>>                         Chris
>>>
>>>
>>>                             Thanks,
>>>                             David
>>>                             -----
>>>
>>>                                 Although we computed a minimum stack
>>>                                 size of 72k, so -Xss72k
>>>                                 should be
>>>                                 fine, pthreads on this platform requires
>>>                                 the stack be at least
>>>                                 128k, so
>>>                                 it failed the
>>>                                 pthread_attr_setstacksize() call. The
>>>                                 end result is
>>>                                 pthread_attr_setstacksize() had no
>>>                                 impact on the thread's stack
>>>                                 size,
>>>                                 and we ended up with the platform
>>>                                 default of 8mb. The fix is to
>>>                                 round up
>>>                                 the following variables to
>>>                                 PTHREAD_STACK_MIN after computing
>>>                                 their new
>>>                                 values:
>>>
>>> _java_thread_min_stack_allowed
>>> _compiler_thread_min_stack_allowed
>>> _vm_internal_thread_min_stack_allowed
>>>
>>>                                 For solaris, there was an issue using
>>>                                 PTHREAD_STACK_MIN. You need to
>>>                                 #define _POSIX_C_SOURCE >= 199506L in
>>>                                 order to get PTHREAD_STACK_MIN
>>>                                 #defined, and this needs to be done
>>>                                 before including OS header
>>>                                 files. I
>>>                                 noticed that on solaris we were using
>>>                                 thr_min_stack() elsewhere
>>>                                 instead
>>>                                 of PTHREAD_STACK_MIN, so I decided to do
>>>                                 the same with this fix.
>>>                                 Either
>>>                                 way is ugly (the #define or using
>>>                                 thr_min_stack()).
>>>
>>>                                 And speaking of the existing use of
>>>                                 thr_min_stack(), I deleted
>>>                                 it. It
>>>                                 was being applied before any adjustments
>>>                                 to the stack sizes had been
>>>                                 made (rounding and adding red, yellow,
>>>                                 and shadow zones). This mean
>>>                                 the
>>>                                 stack ended up being larger than
>>>                                 necessary. With the above fix in
>>>                                 place,
>>>                                 we are now applying thr_min_stack()
>>>                                 after recomputing the minimum
>>>                                 stack
>>>                                 sizes. If for any reason one of those
>>>                                 stack sizes is now too small,
>>>                                 the
>>>                                 correct fix is to adjust the initial
>>>                                 stack sizes, not apply
>>>                                 thr_min_stack() to the initial stack
>>>                                 sizes. However, it looks
>>>                                 like no
>>>                                 adjustment is needed. I did something
>>>                                 close to our nightly
>>>                                 testing on
>>>                                 all affect platforms, and no new
>>>                                 problems turned up.
>>>
>>>                                 thanks,
>>>
>>>                                 Chris
>>>
>>>
>>>
>>>
>>>
>>>
>
>
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 2:35 PM, David Holmes wrote:

> On 17/03/2017 3:49 AM, Chris Plummer wrote:
>> On 3/16/17 2:16 AM, David Holmes wrote:
>>> On 16/03/2017 6:30 PM, Thomas Stüfe wrote:
>>>> Hi Chris, David,
>>>>
>>>> the change looks good.
>>>>
>>>> I see that in the launcher we require a minimum stack size across all
>>>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust
>>>> for
>>>> PTHREAD_STACK_MIN) there?
>>>>
>>>> I do not understand, why does error checking in the hotspot have to be
>>>> consistent with the launcher? What prevents us from asserting in the
>>>> hotspot - or at least print a warning? Note that in the hotspot, there
>>>> is already UL logging ("os", "thread") after pthread_create() in the
>>>> platform files, so the least we could do is add a warning log output
>>>> case ppthread_attr_setstacksize fails.
>>>
>>> Sorry I'm getting this group of bugs all muddled up.
>>>
>>> Chris: this issue does affect hotspot and the launcher (potentially).
>>>
>>> Ideally both should be checking for failures in the pthread calls but
>>> neither do so. Hotspot at least does so in some places but not in a
>>> lot of others.
>>>
>>> pthread_create is different in hotspot because failure can happen
>>> easily and we need to detect it and report it (via an exception and
>>> also via UL). The other pthread calls are not expected to fail under
>>> "normal" conditions but only due to a programming error. Those calls
>>> should at least be checked in debug builds as we already do in places
>>> with assert_status.
>>>
>>> The launcher code doesn't do any error checking at all (but again
>>> pthread_create is a special case).
>> Are you just referring to the pthread related error checking? It does do
>> other error checking.
>
> pthread error checking.
>
> So trying to think this through ...
>
> If the user specifies a too small, or  unaligned-to-page-size, -Xss
> value the pthread_setstacksize() in the launcher will silently fail
> and the main thread will get the default stack of 8M. It will then
> load the VM which will then check the -Xss value, which will do its
> own validity checking.
>
Close, except there is still a potential issue if the size is bigger
than the minimum hotspot requires, but is not page size aligned.
pthread_setstacksize *could* fail in this case, and there would be no
"stack size too small" rejection from the hotspot. However,
pthread_setstacksize did not fail on the two platforms I tried unaligned
stack sizes on.

Chris

> That seems like quite a reasonable position for the launcher to take.
>
> David
> -----
>
>>
>> Chris
>>>
>>> David
>>> -----
>>>
>>>> If we ever refactor this coding, could we rename the variables holding
>>>> the base stack size requirement for java frames - in all its
>>>> incarnations in all the os_cpu files - to be renamed to something
>>>> different? It is a bit confusing to have a variable which at different
>>>> times in VM life means different things (before and after the call
>>>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>>>> "set_minimum_stack_sizes" to something like
>>>> "adjust_minimum_stack_sizes"
>>>> which makes the intent clearer.
>>>>
>>>> Kind Regards, Thomas
>>>>
>>>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>>>
>>>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>>>
>>>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>>
>>>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>>>
>>>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>
>>>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>>>
>>>>                             Hi Chris,
>>>>
>>>>                             On 16/03/2017 3:03 PM, Chris Plummer
>>>> wrote:
>>>>
>>>>                                 Hello,
>>>>
>>>>                                 Please review the following:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>>>
>>>>
>>>>
>>>>                             Change looks good.
>>>>
>>>>                                 While working on 8175342 I noticed our
>>>>                                 stack size on xgene was 8mb
>>>>                                 even
>>>>                                 though I was specifying -Xss72k. It
>>>>                                 turns out the following code was
>>>>                                 failing:
>>>>
>>>> pthread_attr_setstacksize(&attr,
>>>>                                 stack_size);
>>>>
>>>>
>>>>                             So these really should be checking return
>>>>                             values, at least in debug
>>>>                             builds. But we can leave that until we
>>>>                             refactor the thread startup
>>>>                             code into os_posix.cpp.
>>>>
>>>>                         I considered adding checks. I wasn't sure
>>>> if we
>>>>                         should abort or just
>>>>                         print a warning if it failed.
>>>>
>>>>
>>>>                     When we check pthread lib routines we use:
>>>>
>>>>                        int status = pthread_mutex_lock(_mutex);
>>>>                        assert_status(status == 0, status,
>>>> "mutex_lock");
>>>>
>>>>                     This is for things that should only fail if we
>>>> have
>>>>                     a programming
>>>>                     error.
>>>>
>>>>                 Ok, but this is in the launcher, so I'll need to just
>>>>                 use the built-in
>>>>                 assert(). I'll add that if want.
>>>>
>>>>
>>>>             Oops! I was forgetting that. Need to be consistent with
>>>>             launcher error
>>>>             checking or lack thereof. And ignore refactoring
>>>> comments -
>>>>             not relevant.
>>>>
>>>>         So don't add the error check?
>>>>
>>>>
>>>>     Given there is no error checking, or assertions, in those files I
>>>>     reluctantly have to say leave it out.
>>>>
>>>>     Thanks,
>>>>     David
>>>>     -----
>>>>
>>>>
>>>>
>>>>             David
>>>>
>>>>                 Chris
>>>>
>>>>
>>>>                         What refactoring is planned?
>>>>
>>>>
>>>>                     "Planned" might be a bit strong :) I was
>>>> thinking of
>>>>                     a number of
>>>>                     os_posix related cleanups for which issues exist,
>>>>                     but also forgot that
>>>>                     some of our general clean-up RFE's have been
>>>> closed
>>>>                     as WNF :( I may do
>>>>                     some of them after hours anyway :)
>>>>
>>>>                     David
>>>>                     -----
>>>>
>>>>                         Chris
>>>>
>>>>
>>>>                             Thanks,
>>>>                             David
>>>>                             -----
>>>>
>>>>                                 Although we computed a minimum stack
>>>>                                 size of 72k, so -Xss72k
>>>>                                 should be
>>>>                                 fine, pthreads on this platform
>>>> requires
>>>>                                 the stack be at least
>>>>                                 128k, so
>>>>                                 it failed the
>>>>                                 pthread_attr_setstacksize() call. The
>>>>                                 end result is
>>>>                                 pthread_attr_setstacksize() had no
>>>>                                 impact on the thread's stack
>>>>                                 size,
>>>>                                 and we ended up with the platform
>>>>                                 default of 8mb. The fix is to
>>>>                                 round up
>>>>                                 the following variables to
>>>>                                 PTHREAD_STACK_MIN after computing
>>>>                                 their new
>>>>                                 values:
>>>>
>>>> _java_thread_min_stack_allowed
>>>> _compiler_thread_min_stack_allowed
>>>> _vm_internal_thread_min_stack_allowed
>>>>
>>>>                                 For solaris, there was an issue using
>>>>                                 PTHREAD_STACK_MIN. You need to
>>>>                                 #define _POSIX_C_SOURCE >= 199506L in
>>>>                                 order to get PTHREAD_STACK_MIN
>>>>                                 #defined, and this needs to be done
>>>>                                 before including OS header
>>>>                                 files. I
>>>>                                 noticed that on solaris we were using
>>>>                                 thr_min_stack() elsewhere
>>>>                                 instead
>>>>                                 of PTHREAD_STACK_MIN, so I decided
>>>> to do
>>>>                                 the same with this fix.
>>>>                                 Either
>>>>                                 way is ugly (the #define or using
>>>>                                 thr_min_stack()).
>>>>
>>>>                                 And speaking of the existing use of
>>>>                                 thr_min_stack(), I deleted
>>>>                                 it. It
>>>>                                 was being applied before any
>>>> adjustments
>>>>                                 to the stack sizes had been
>>>>                                 made (rounding and adding red, yellow,
>>>>                                 and shadow zones). This mean
>>>>                                 the
>>>>                                 stack ended up being larger than
>>>>                                 necessary. With the above fix in
>>>>                                 place,
>>>>                                 we are now applying thr_min_stack()
>>>>                                 after recomputing the minimum
>>>>                                 stack
>>>>                                 sizes. If for any reason one of those
>>>>                                 stack sizes is now too small,
>>>>                                 the
>>>>                                 correct fix is to adjust the initial
>>>>                                 stack sizes, not apply
>>>>                                 thr_min_stack() to the initial stack
>>>>                                 sizes. However, it looks
>>>>                                 like no
>>>>                                 adjustment is needed. I did something
>>>>                                 close to our nightly
>>>>                                 testing on
>>>>                                 all affect platforms, and no new
>>>>                                 problems turned up.
>>>>
>>>>                                 thanks,
>>>>
>>>>                                 Chris
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>
>>


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 17/03/2017 7:43 AM, Chris Plummer wrote:

> On 3/16/17 2:35 PM, David Holmes wrote:
>> On 17/03/2017 3:49 AM, Chris Plummer wrote:
>>> On 3/16/17 2:16 AM, David Holmes wrote:
>>>> On 16/03/2017 6:30 PM, Thomas Stüfe wrote:
>>>>> Hi Chris, David,
>>>>>
>>>>> the change looks good.
>>>>>
>>>>> I see that in the launcher we require a minimum stack size across all
>>>>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust
>>>>> for
>>>>> PTHREAD_STACK_MIN) there?
>>>>>
>>>>> I do not understand, why does error checking in the hotspot have to be
>>>>> consistent with the launcher? What prevents us from asserting in the
>>>>> hotspot - or at least print a warning? Note that in the hotspot, there
>>>>> is already UL logging ("os", "thread") after pthread_create() in the
>>>>> platform files, so the least we could do is add a warning log output
>>>>> case ppthread_attr_setstacksize fails.
>>>>
>>>> Sorry I'm getting this group of bugs all muddled up.
>>>>
>>>> Chris: this issue does affect hotspot and the launcher (potentially).
>>>>
>>>> Ideally both should be checking for failures in the pthread calls but
>>>> neither do so. Hotspot at least does so in some places but not in a
>>>> lot of others.
>>>>
>>>> pthread_create is different in hotspot because failure can happen
>>>> easily and we need to detect it and report it (via an exception and
>>>> also via UL). The other pthread calls are not expected to fail under
>>>> "normal" conditions but only due to a programming error. Those calls
>>>> should at least be checked in debug builds as we already do in places
>>>> with assert_status.
>>>>
>>>> The launcher code doesn't do any error checking at all (but again
>>>> pthread_create is a special case).
>>> Are you just referring to the pthread related error checking? It does do
>>> other error checking.
>>
>> pthread error checking.
>>
>> So trying to think this through ...
>>
>> If the user specifies a too small, or  unaligned-to-page-size, -Xss
>> value the pthread_setstacksize() in the launcher will silently fail
>> and the main thread will get the default stack of 8M. It will then
>> load the VM which will then check the -Xss value, which will do its
>> own validity checking.
>>
> Close, except there is still a potential issue if the size is bigger
> than the minimum hotspot requires, but is not page size aligned.
> pthread_setstacksize *could* fail in this case, and there would be no
> "stack size too small" rejection from the hotspot. However,
> pthread_setstacksize did not fail on the two platforms I tried unaligned
> stack sizes on.

Perhaps because that is not specified by POSIX. For POSIX we only have:

[EINVAL]
     The value of stacksize is less than {PTHREAD_STACK_MIN} or exceeds
a system-imposed limit.

Anyway that is a check that hotspot could perform if
pthread_attr_setstacksize fails. Though that then makes me wonder if we
do any rounding when the stack size set on a per thread basis via the
java.lang.Thread constructor?

I think imposing the PTHREAD_STACK_MIN in hotspot, with an assert
checking pthread_attr_setstacksize succeeded (in hotspot) would suffice
here.

David
-----

> Chris
>> That seems like quite a reasonable position for the launcher to take.
>>
>> David
>> -----
>>
>>>
>>> Chris
>>>>
>>>> David
>>>> -----
>>>>
>>>>> If we ever refactor this coding, could we rename the variables holding
>>>>> the base stack size requirement for java frames - in all its
>>>>> incarnations in all the os_cpu files - to be renamed to something
>>>>> different? It is a bit confusing to have a variable which at different
>>>>> times in VM life means different things (before and after the call
>>>>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>>>>> "set_minimum_stack_sizes" to something like
>>>>> "adjust_minimum_stack_sizes"
>>>>> which makes the intent clearer.
>>>>>
>>>>> Kind Regards, Thomas
>>>>>
>>>>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes <[hidden email]
>>>>> <mailto:[hidden email]>> wrote:
>>>>>
>>>>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>>>>
>>>>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>>>>
>>>>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>>>
>>>>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>>>>
>>>>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>>
>>>>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>>>>
>>>>>                             Hi Chris,
>>>>>
>>>>>                             On 16/03/2017 3:03 PM, Chris Plummer
>>>>> wrote:
>>>>>
>>>>>                                 Hello,
>>>>>
>>>>>                                 Please review the following:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>>>>
>>>>>
>>>>>
>>>>>                             Change looks good.
>>>>>
>>>>>                                 While working on 8175342 I noticed our
>>>>>                                 stack size on xgene was 8mb
>>>>>                                 even
>>>>>                                 though I was specifying -Xss72k. It
>>>>>                                 turns out the following code was
>>>>>                                 failing:
>>>>>
>>>>> pthread_attr_setstacksize(&attr,
>>>>>                                 stack_size);
>>>>>
>>>>>
>>>>>                             So these really should be checking return
>>>>>                             values, at least in debug
>>>>>                             builds. But we can leave that until we
>>>>>                             refactor the thread startup
>>>>>                             code into os_posix.cpp.
>>>>>
>>>>>                         I considered adding checks. I wasn't sure
>>>>> if we
>>>>>                         should abort or just
>>>>>                         print a warning if it failed.
>>>>>
>>>>>
>>>>>                     When we check pthread lib routines we use:
>>>>>
>>>>>                        int status = pthread_mutex_lock(_mutex);
>>>>>                        assert_status(status == 0, status,
>>>>> "mutex_lock");
>>>>>
>>>>>                     This is for things that should only fail if we
>>>>> have
>>>>>                     a programming
>>>>>                     error.
>>>>>
>>>>>                 Ok, but this is in the launcher, so I'll need to just
>>>>>                 use the built-in
>>>>>                 assert(). I'll add that if want.
>>>>>
>>>>>
>>>>>             Oops! I was forgetting that. Need to be consistent with
>>>>>             launcher error
>>>>>             checking or lack thereof. And ignore refactoring
>>>>> comments -
>>>>>             not relevant.
>>>>>
>>>>>         So don't add the error check?
>>>>>
>>>>>
>>>>>     Given there is no error checking, or assertions, in those files I
>>>>>     reluctantly have to say leave it out.
>>>>>
>>>>>     Thanks,
>>>>>     David
>>>>>     -----
>>>>>
>>>>>
>>>>>
>>>>>             David
>>>>>
>>>>>                 Chris
>>>>>
>>>>>
>>>>>                         What refactoring is planned?
>>>>>
>>>>>
>>>>>                     "Planned" might be a bit strong :) I was
>>>>> thinking of
>>>>>                     a number of
>>>>>                     os_posix related cleanups for which issues exist,
>>>>>                     but also forgot that
>>>>>                     some of our general clean-up RFE's have been
>>>>> closed
>>>>>                     as WNF :( I may do
>>>>>                     some of them after hours anyway :)
>>>>>
>>>>>                     David
>>>>>                     -----
>>>>>
>>>>>                         Chris
>>>>>
>>>>>
>>>>>                             Thanks,
>>>>>                             David
>>>>>                             -----
>>>>>
>>>>>                                 Although we computed a minimum stack
>>>>>                                 size of 72k, so -Xss72k
>>>>>                                 should be
>>>>>                                 fine, pthreads on this platform
>>>>> requires
>>>>>                                 the stack be at least
>>>>>                                 128k, so
>>>>>                                 it failed the
>>>>>                                 pthread_attr_setstacksize() call. The
>>>>>                                 end result is
>>>>>                                 pthread_attr_setstacksize() had no
>>>>>                                 impact on the thread's stack
>>>>>                                 size,
>>>>>                                 and we ended up with the platform
>>>>>                                 default of 8mb. The fix is to
>>>>>                                 round up
>>>>>                                 the following variables to
>>>>>                                 PTHREAD_STACK_MIN after computing
>>>>>                                 their new
>>>>>                                 values:
>>>>>
>>>>> _java_thread_min_stack_allowed
>>>>> _compiler_thread_min_stack_allowed
>>>>> _vm_internal_thread_min_stack_allowed
>>>>>
>>>>>                                 For solaris, there was an issue using
>>>>>                                 PTHREAD_STACK_MIN. You need to
>>>>>                                 #define _POSIX_C_SOURCE >= 199506L in
>>>>>                                 order to get PTHREAD_STACK_MIN
>>>>>                                 #defined, and this needs to be done
>>>>>                                 before including OS header
>>>>>                                 files. I
>>>>>                                 noticed that on solaris we were using
>>>>>                                 thr_min_stack() elsewhere
>>>>>                                 instead
>>>>>                                 of PTHREAD_STACK_MIN, so I decided
>>>>> to do
>>>>>                                 the same with this fix.
>>>>>                                 Either
>>>>>                                 way is ugly (the #define or using
>>>>>                                 thr_min_stack()).
>>>>>
>>>>>                                 And speaking of the existing use of
>>>>>                                 thr_min_stack(), I deleted
>>>>>                                 it. It
>>>>>                                 was being applied before any
>>>>> adjustments
>>>>>                                 to the stack sizes had been
>>>>>                                 made (rounding and adding red, yellow,
>>>>>                                 and shadow zones). This mean
>>>>>                                 the
>>>>>                                 stack ended up being larger than
>>>>>                                 necessary. With the above fix in
>>>>>                                 place,
>>>>>                                 we are now applying thr_min_stack()
>>>>>                                 after recomputing the minimum
>>>>>                                 stack
>>>>>                                 sizes. If for any reason one of those
>>>>>                                 stack sizes is now too small,
>>>>>                                 the
>>>>>                                 correct fix is to adjust the initial
>>>>>                                 stack sizes, not apply
>>>>>                                 thr_min_stack() to the initial stack
>>>>>                                 sizes. However, it looks
>>>>>                                 like no
>>>>>                                 adjustment is needed. I did something
>>>>>                                 close to our nightly
>>>>>                                 testing on
>>>>>                                 all affect platforms, and no new
>>>>>                                 problems turned up.
>>>>>
>>>>>                                 thanks,
>>>>>
>>>>>                                 Chris
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
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 3:01 PM, David Holmes wrote:

> On 17/03/2017 7:43 AM, Chris Plummer wrote:
>> On 3/16/17 2:35 PM, David Holmes wrote:
>>> On 17/03/2017 3:49 AM, Chris Plummer wrote:
>>>> On 3/16/17 2:16 AM, David Holmes wrote:
>>>>> On 16/03/2017 6:30 PM, Thomas St�fe wrote:
>>>>>> Hi Chris, David,
>>>>>>
>>>>>> the change looks good.
>>>>>>
>>>>>> I see that in the launcher we require a minimum stack size across
>>>>>> all
>>>>>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust
>>>>>> for
>>>>>> PTHREAD_STACK_MIN) there?
>>>>>>
>>>>>> I do not understand, why does error checking in the hotspot have
>>>>>> to be
>>>>>> consistent with the launcher? What prevents us from asserting in the
>>>>>> hotspot - or at least print a warning? Note that in the hotspot,
>>>>>> there
>>>>>> is already UL logging ("os", "thread") after pthread_create() in the
>>>>>> platform files, so the least we could do is add a warning log output
>>>>>> case ppthread_attr_setstacksize fails.
>>>>>
>>>>> Sorry I'm getting this group of bugs all muddled up.
>>>>>
>>>>> Chris: this issue does affect hotspot and the launcher (potentially).
>>>>>
>>>>> Ideally both should be checking for failures in the pthread calls but
>>>>> neither do so. Hotspot at least does so in some places but not in a
>>>>> lot of others.
>>>>>
>>>>> pthread_create is different in hotspot because failure can happen
>>>>> easily and we need to detect it and report it (via an exception and
>>>>> also via UL). The other pthread calls are not expected to fail under
>>>>> "normal" conditions but only due to a programming error. Those calls
>>>>> should at least be checked in debug builds as we already do in places
>>>>> with assert_status.
>>>>>
>>>>> The launcher code doesn't do any error checking at all (but again
>>>>> pthread_create is a special case).
>>>> Are you just referring to the pthread related error checking? It
>>>> does do
>>>> other error checking.
>>>
>>> pthread error checking.
>>>
>>> So trying to think this through ...
>>>
>>> If the user specifies a too small, or  unaligned-to-page-size, -Xss
>>> value the pthread_setstacksize() in the launcher will silently fail
>>> and the main thread will get the default stack of 8M. It will then
>>> load the VM which will then check the -Xss value, which will do its
>>> own validity checking.
>>>
>> Close, except there is still a potential issue if the size is bigger
>> than the minimum hotspot requires, but is not page size aligned.
>> pthread_setstacksize *could* fail in this case, and there would be no
>> "stack size too small" rejection from the hotspot. However,
>> pthread_setstacksize did not fail on the two platforms I tried unaligned
>> stack sizes on.
>
> Perhaps because that is not specified by POSIX. For POSIX we only have:
>
> [EINVAL]
>     The value of stacksize is less than {PTHREAD_STACK_MIN} or exceeds
> a system-imposed limit.
The man page on my linux host also adds the warning about "some hosts
can fail if the stack size is not a multiple of the system page size."
Is the man page documenting something different?
>
> Anyway that is a check that hotspot could perform if
> pthread_attr_setstacksize fails. Though that then makes me wonder if
> we do any rounding when the stack size set on a per thread basis via
> the java.lang.Thread constructor?
>
> I think imposing the PTHREAD_STACK_MIN in hotspot, with an assert
> checking pthread_attr_setstacksize succeeded (in hotspot) would
> suffice here.
If you are certain that the assert would be a programming error and not
a user error, then I can see doing that. However, shouldn't we be
consistent in the launcher and do the same there also? We can skip
imposing PTHREAD_STACK_MIN since hotspot will already do this, but
unless the user creates another java thread there will be no hotspot
assert for pthread_attr_setstacksize failing.

Chris

>
> David
> -----
>
>> Chris
>>> That seems like quite a reasonable position for the launcher to take.
>>>
>>> David
>>> -----
>>>
>>>>
>>>> Chris
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> If we ever refactor this coding, could we rename the variables
>>>>>> holding
>>>>>> the base stack size requirement for java frames - in all its
>>>>>> incarnations in all the os_cpu files - to be renamed to something
>>>>>> different? It is a bit confusing to have a variable which at
>>>>>> different
>>>>>> times in VM life means different things (before and after the call
>>>>>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>>>>>> "set_minimum_stack_sizes" to something like
>>>>>> "adjust_minimum_stack_sizes"
>>>>>> which makes the intent clearer.
>>>>>>
>>>>>> Kind Regards, Thomas
>>>>>>
>>>>>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes
>>>>>> <[hidden email]
>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>
>>>>>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>>>>>
>>>>>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>>>>>
>>>>>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>>>>
>>>>>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>>>>>
>>>>>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>>>
>>>>>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>>>>>
>>>>>>                             Hi Chris,
>>>>>>
>>>>>>                             On 16/03/2017 3:03 PM, Chris Plummer
>>>>>> wrote:
>>>>>>
>>>>>>                                 Hello,
>>>>>>
>>>>>>                                 Please review the following:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>
>>>>>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>                             Change looks good.
>>>>>>
>>>>>>                                 While working on 8175342 I
>>>>>> noticed our
>>>>>>                                 stack size on xgene was 8mb
>>>>>>                                 even
>>>>>>                                 though I was specifying -Xss72k. It
>>>>>>                                 turns out the following code was
>>>>>>                                 failing:
>>>>>>
>>>>>> pthread_attr_setstacksize(&attr,
>>>>>>                                 stack_size);
>>>>>>
>>>>>>
>>>>>>                             So these really should be checking
>>>>>> return
>>>>>>                             values, at least in debug
>>>>>>                             builds. But we can leave that until we
>>>>>>                             refactor the thread startup
>>>>>>                             code into os_posix.cpp.
>>>>>>
>>>>>>                         I considered adding checks. I wasn't sure
>>>>>> if we
>>>>>>                         should abort or just
>>>>>>                         print a warning if it failed.
>>>>>>
>>>>>>
>>>>>>                     When we check pthread lib routines we use:
>>>>>>
>>>>>>                        int status = pthread_mutex_lock(_mutex);
>>>>>>                        assert_status(status == 0, status,
>>>>>> "mutex_lock");
>>>>>>
>>>>>>                     This is for things that should only fail if we
>>>>>> have
>>>>>>                     a programming
>>>>>>                     error.
>>>>>>
>>>>>>                 Ok, but this is in the launcher, so I'll need to
>>>>>> just
>>>>>>                 use the built-in
>>>>>>                 assert(). I'll add that if want.
>>>>>>
>>>>>>
>>>>>>             Oops! I was forgetting that. Need to be consistent with
>>>>>>             launcher error
>>>>>>             checking or lack thereof. And ignore refactoring
>>>>>> comments -
>>>>>>             not relevant.
>>>>>>
>>>>>>         So don't add the error check?
>>>>>>
>>>>>>
>>>>>>     Given there is no error checking, or assertions, in those
>>>>>> files I
>>>>>>     reluctantly have to say leave it out.
>>>>>>
>>>>>>     Thanks,
>>>>>>     David
>>>>>>     -----
>>>>>>
>>>>>>
>>>>>>
>>>>>>             David
>>>>>>
>>>>>>                 Chris
>>>>>>
>>>>>>
>>>>>>                         What refactoring is planned?
>>>>>>
>>>>>>
>>>>>>                     "Planned" might be a bit strong :) I was
>>>>>> thinking of
>>>>>>                     a number of
>>>>>>                     os_posix related cleanups for which issues
>>>>>> exist,
>>>>>>                     but also forgot that
>>>>>>                     some of our general clean-up RFE's have been
>>>>>> closed
>>>>>>                     as WNF :( I may do
>>>>>>                     some of them after hours anyway :)
>>>>>>
>>>>>>                     David
>>>>>>                     -----
>>>>>>
>>>>>>                         Chris
>>>>>>
>>>>>>
>>>>>>                             Thanks,
>>>>>>                             David
>>>>>>                             -----
>>>>>>
>>>>>>                                 Although we computed a minimum stack
>>>>>>                                 size of 72k, so -Xss72k
>>>>>>                                 should be
>>>>>>                                 fine, pthreads on this platform
>>>>>> requires
>>>>>>                                 the stack be at least
>>>>>>                                 128k, so
>>>>>>                                 it failed the
>>>>>> pthread_attr_setstacksize() call. The
>>>>>>                                 end result is
>>>>>> pthread_attr_setstacksize() had no
>>>>>>                                 impact on the thread's stack
>>>>>>                                 size,
>>>>>>                                 and we ended up with the platform
>>>>>>                                 default of 8mb. The fix is to
>>>>>>                                 round up
>>>>>>                                 the following variables to
>>>>>>                                 PTHREAD_STACK_MIN after computing
>>>>>>                                 their new
>>>>>>                                 values:
>>>>>>
>>>>>> _java_thread_min_stack_allowed
>>>>>> _compiler_thread_min_stack_allowed
>>>>>> _vm_internal_thread_min_stack_allowed
>>>>>>
>>>>>>                                 For solaris, there was an issue
>>>>>> using
>>>>>>                                 PTHREAD_STACK_MIN. You need to
>>>>>>                                 #define _POSIX_C_SOURCE >=
>>>>>> 199506L in
>>>>>>                                 order to get PTHREAD_STACK_MIN
>>>>>>                                 #defined, and this needs to be done
>>>>>>                                 before including OS header
>>>>>>                                 files. I
>>>>>>                                 noticed that on solaris we were
>>>>>> using
>>>>>>                                 thr_min_stack() elsewhere
>>>>>>                                 instead
>>>>>>                                 of PTHREAD_STACK_MIN, so I decided
>>>>>> to do
>>>>>>                                 the same with this fix.
>>>>>>                                 Either
>>>>>>                                 way is ugly (the #define or using
>>>>>>                                 thr_min_stack()).
>>>>>>
>>>>>>                                 And speaking of the existing use of
>>>>>>                                 thr_min_stack(), I deleted
>>>>>>                                 it. It
>>>>>>                                 was being applied before any
>>>>>> adjustments
>>>>>>                                 to the stack sizes had been
>>>>>>                                 made (rounding and adding red,
>>>>>> yellow,
>>>>>>                                 and shadow zones). This mean
>>>>>>                                 the
>>>>>>                                 stack ended up being larger than
>>>>>>                                 necessary. With the above fix in
>>>>>>                                 place,
>>>>>>                                 we are now applying thr_min_stack()
>>>>>>                                 after recomputing the minimum
>>>>>>                                 stack
>>>>>>                                 sizes. If for any reason one of
>>>>>> those
>>>>>>                                 stack sizes is now too small,
>>>>>>                                 the
>>>>>>                                 correct fix is to adjust the initial
>>>>>>                                 stack sizes, not apply
>>>>>>                                 thr_min_stack() to the initial stack
>>>>>>                                 sizes. However, it looks
>>>>>>                                 like no
>>>>>>                                 adjustment is needed. I did
>>>>>> something
>>>>>>                                 close to our nightly
>>>>>>                                 testing on
>>>>>>                                 all affect platforms, and no new
>>>>>>                                 problems turned up.
>>>>>>
>>>>>>                                 thanks,
>>>>>>
>>>>>>                                 Chris
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>


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,

On 17/03/2017 8:14 AM, Chris Plummer wrote:

> On 3/16/17 3:01 PM, David Holmes wrote:
>> On 17/03/2017 7:43 AM, Chris Plummer wrote:
>>> On 3/16/17 2:35 PM, David Holmes wrote:
>>>> On 17/03/2017 3:49 AM, Chris Plummer wrote:
>>>>> On 3/16/17 2:16 AM, David Holmes wrote:
>>>>>> On 16/03/2017 6:30 PM, Thomas St�fe wrote:
>>>>>>> Hi Chris, David,
>>>>>>>
>>>>>>> the change looks good.
>>>>>>>
>>>>>>> I see that in the launcher we require a minimum stack size across
>>>>>>> all
>>>>>>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix (adjust
>>>>>>> for
>>>>>>> PTHREAD_STACK_MIN) there?
>>>>>>>
>>>>>>> I do not understand, why does error checking in the hotspot have
>>>>>>> to be
>>>>>>> consistent with the launcher? What prevents us from asserting in the
>>>>>>> hotspot - or at least print a warning? Note that in the hotspot,
>>>>>>> there
>>>>>>> is already UL logging ("os", "thread") after pthread_create() in the
>>>>>>> platform files, so the least we could do is add a warning log output
>>>>>>> case ppthread_attr_setstacksize fails.
>>>>>>
>>>>>> Sorry I'm getting this group of bugs all muddled up.
>>>>>>
>>>>>> Chris: this issue does affect hotspot and the launcher (potentially).
>>>>>>
>>>>>> Ideally both should be checking for failures in the pthread calls but
>>>>>> neither do so. Hotspot at least does so in some places but not in a
>>>>>> lot of others.
>>>>>>
>>>>>> pthread_create is different in hotspot because failure can happen
>>>>>> easily and we need to detect it and report it (via an exception and
>>>>>> also via UL). The other pthread calls are not expected to fail under
>>>>>> "normal" conditions but only due to a programming error. Those calls
>>>>>> should at least be checked in debug builds as we already do in places
>>>>>> with assert_status.
>>>>>>
>>>>>> The launcher code doesn't do any error checking at all (but again
>>>>>> pthread_create is a special case).
>>>>> Are you just referring to the pthread related error checking? It
>>>>> does do
>>>>> other error checking.
>>>>
>>>> pthread error checking.
>>>>
>>>> So trying to think this through ...
>>>>
>>>> If the user specifies a too small, or  unaligned-to-page-size, -Xss
>>>> value the pthread_setstacksize() in the launcher will silently fail
>>>> and the main thread will get the default stack of 8M. It will then
>>>> load the VM which will then check the -Xss value, which will do its
>>>> own validity checking.
>>>>
>>> Close, except there is still a potential issue if the size is bigger
>>> than the minimum hotspot requires, but is not page size aligned.
>>> pthread_setstacksize *could* fail in this case, and there would be no
>>> "stack size too small" rejection from the hotspot. However,
>>> pthread_setstacksize did not fail on the two platforms I tried unaligned
>>> stack sizes on.
>>
>> Perhaps because that is not specified by POSIX. For POSIX we only have:
>>
>> [EINVAL]
>>     The value of stacksize is less than {PTHREAD_STACK_MIN} or exceeds
>> a system-imposed limit.
> The man page on my linux host also adds the warning about "some hosts
> can fail if the stack size is not a multiple of the system page size."
> Is the man page documenting something different?

Yes it's documenting that Linux doesn't follow POSIX very well a lot of
the time. :( However the source code seems different:

http://code.metager.de/source/xref/gnu/glibc/nptl/pthreadP.h

628 static inline int
629 check_stacksize_attr (size_t st)
630 {
631  if (st >= PTHREAD_STACK_MIN)
632    return 0;
633
634  return EINVAL;
635 }

That said, it seems that OSX will return EINVAL:

https://opensource.apple.com/source/libpthread/libpthread-218.1.3/src/pthread.c.auto.html

        int ret = EINVAL;
        if (attr->sig == _PTHREAD_ATTR_SIG &&
            (stacksize % vm_page_size) == 0 &&
            stacksize >= PTHREAD_STACK_MIN) {
                attr->stacksize = stacksize;
                ret = 0;
        }
        return ret;

>>
>> Anyway that is a check that hotspot could perform if
>> pthread_attr_setstacksize fails. Though that then makes me wonder if
>> we do any rounding when the stack size set on a per thread basis via
>> the java.lang.Thread constructor?
>>
>> I think imposing the PTHREAD_STACK_MIN in hotspot, with an assert
>> checking pthread_attr_setstacksize succeeded (in hotspot) would
>> suffice here.
> If you are certain that the assert would be a programming error and not
> a user error, then I can see doing that. However, shouldn't we be
> consistent in the launcher and do the same there also? We can skip
> imposing PTHREAD_STACK_MIN since hotspot will already do this, but
> unless the user creates another java thread there will be no hotspot
> assert for pthread_attr_setstacksize failing.

The launcher has its own policy regarding errors here - it ignores them.
As I said before if you pass an invalid -Xss value the launcher will
create the main thread with the default stack size. That is not an
unreasonable position to take. If you feel strongly about it you could
file a bug against the launcher, but I would not try to fix it in this CR.

Inside hotspot I think we already do various roundings on the value
eventually passed to pthread_attr_setstacksize, don't we? So any EINVAL
after that should be effectively impossible when combined with the
PTHREAD_STACK_MIN check.

Thanks,
David

> Chris
>>
>> David
>> -----
>>
>>> Chris
>>>> That seems like quite a reasonable position for the launcher to take.
>>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Chris
>>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> If we ever refactor this coding, could we rename the variables
>>>>>>> holding
>>>>>>> the base stack size requirement for java frames - in all its
>>>>>>> incarnations in all the os_cpu files - to be renamed to something
>>>>>>> different? It is a bit confusing to have a variable which at
>>>>>>> different
>>>>>>> times in VM life means different things (before and after the call
>>>>>>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>>>>>>> "set_minimum_stack_sizes" to something like
>>>>>>> "adjust_minimum_stack_sizes"
>>>>>>> which makes the intent clearer.
>>>>>>>
>>>>>>> Kind Regards, Thomas
>>>>>>>
>>>>>>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes
>>>>>>> <[hidden email]
>>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>>
>>>>>>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>>>>>>
>>>>>>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>>>>>>
>>>>>>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>>>>>
>>>>>>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>>>>>>
>>>>>>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>>>>
>>>>>>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>>>>>>
>>>>>>>                             Hi Chris,
>>>>>>>
>>>>>>>                             On 16/03/2017 3:03 PM, Chris Plummer
>>>>>>> wrote:
>>>>>>>
>>>>>>>                                 Hello,
>>>>>>>
>>>>>>>                                 Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot
>>>>>>>
>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                             Change looks good.
>>>>>>>
>>>>>>>                                 While working on 8175342 I
>>>>>>> noticed our
>>>>>>>                                 stack size on xgene was 8mb
>>>>>>>                                 even
>>>>>>>                                 though I was specifying -Xss72k. It
>>>>>>>                                 turns out the following code was
>>>>>>>                                 failing:
>>>>>>>
>>>>>>> pthread_attr_setstacksize(&attr,
>>>>>>>                                 stack_size);
>>>>>>>
>>>>>>>
>>>>>>>                             So these really should be checking
>>>>>>> return
>>>>>>>                             values, at least in debug
>>>>>>>                             builds. But we can leave that until we
>>>>>>>                             refactor the thread startup
>>>>>>>                             code into os_posix.cpp.
>>>>>>>
>>>>>>>                         I considered adding checks. I wasn't sure
>>>>>>> if we
>>>>>>>                         should abort or just
>>>>>>>                         print a warning if it failed.
>>>>>>>
>>>>>>>
>>>>>>>                     When we check pthread lib routines we use:
>>>>>>>
>>>>>>>                        int status = pthread_mutex_lock(_mutex);
>>>>>>>                        assert_status(status == 0, status,
>>>>>>> "mutex_lock");
>>>>>>>
>>>>>>>                     This is for things that should only fail if we
>>>>>>> have
>>>>>>>                     a programming
>>>>>>>                     error.
>>>>>>>
>>>>>>>                 Ok, but this is in the launcher, so I'll need to
>>>>>>> just
>>>>>>>                 use the built-in
>>>>>>>                 assert(). I'll add that if want.
>>>>>>>
>>>>>>>
>>>>>>>             Oops! I was forgetting that. Need to be consistent with
>>>>>>>             launcher error
>>>>>>>             checking or lack thereof. And ignore refactoring
>>>>>>> comments -
>>>>>>>             not relevant.
>>>>>>>
>>>>>>>         So don't add the error check?
>>>>>>>
>>>>>>>
>>>>>>>     Given there is no error checking, or assertions, in those
>>>>>>> files I
>>>>>>>     reluctantly have to say leave it out.
>>>>>>>
>>>>>>>     Thanks,
>>>>>>>     David
>>>>>>>     -----
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>             David
>>>>>>>
>>>>>>>                 Chris
>>>>>>>
>>>>>>>
>>>>>>>                         What refactoring is planned?
>>>>>>>
>>>>>>>
>>>>>>>                     "Planned" might be a bit strong :) I was
>>>>>>> thinking of
>>>>>>>                     a number of
>>>>>>>                     os_posix related cleanups for which issues
>>>>>>> exist,
>>>>>>>                     but also forgot that
>>>>>>>                     some of our general clean-up RFE's have been
>>>>>>> closed
>>>>>>>                     as WNF :( I may do
>>>>>>>                     some of them after hours anyway :)
>>>>>>>
>>>>>>>                     David
>>>>>>>                     -----
>>>>>>>
>>>>>>>                         Chris
>>>>>>>
>>>>>>>
>>>>>>>                             Thanks,
>>>>>>>                             David
>>>>>>>                             -----
>>>>>>>
>>>>>>>                                 Although we computed a minimum stack
>>>>>>>                                 size of 72k, so -Xss72k
>>>>>>>                                 should be
>>>>>>>                                 fine, pthreads on this platform
>>>>>>> requires
>>>>>>>                                 the stack be at least
>>>>>>>                                 128k, so
>>>>>>>                                 it failed the
>>>>>>> pthread_attr_setstacksize() call. The
>>>>>>>                                 end result is
>>>>>>> pthread_attr_setstacksize() had no
>>>>>>>                                 impact on the thread's stack
>>>>>>>                                 size,
>>>>>>>                                 and we ended up with the platform
>>>>>>>                                 default of 8mb. The fix is to
>>>>>>>                                 round up
>>>>>>>                                 the following variables to
>>>>>>>                                 PTHREAD_STACK_MIN after computing
>>>>>>>                                 their new
>>>>>>>                                 values:
>>>>>>>
>>>>>>> _java_thread_min_stack_allowed
>>>>>>> _compiler_thread_min_stack_allowed
>>>>>>> _vm_internal_thread_min_stack_allowed
>>>>>>>
>>>>>>>                                 For solaris, there was an issue
>>>>>>> using
>>>>>>>                                 PTHREAD_STACK_MIN. You need to
>>>>>>>                                 #define _POSIX_C_SOURCE >=
>>>>>>> 199506L in
>>>>>>>                                 order to get PTHREAD_STACK_MIN
>>>>>>>                                 #defined, and this needs to be done
>>>>>>>                                 before including OS header
>>>>>>>                                 files. I
>>>>>>>                                 noticed that on solaris we were
>>>>>>> using
>>>>>>>                                 thr_min_stack() elsewhere
>>>>>>>                                 instead
>>>>>>>                                 of PTHREAD_STACK_MIN, so I decided
>>>>>>> to do
>>>>>>>                                 the same with this fix.
>>>>>>>                                 Either
>>>>>>>                                 way is ugly (the #define or using
>>>>>>>                                 thr_min_stack()).
>>>>>>>
>>>>>>>                                 And speaking of the existing use of
>>>>>>>                                 thr_min_stack(), I deleted
>>>>>>>                                 it. It
>>>>>>>                                 was being applied before any
>>>>>>> adjustments
>>>>>>>                                 to the stack sizes had been
>>>>>>>                                 made (rounding and adding red,
>>>>>>> yellow,
>>>>>>>                                 and shadow zones). This mean
>>>>>>>                                 the
>>>>>>>                                 stack ended up being larger than
>>>>>>>                                 necessary. With the above fix in
>>>>>>>                                 place,
>>>>>>>                                 we are now applying thr_min_stack()
>>>>>>>                                 after recomputing the minimum
>>>>>>>                                 stack
>>>>>>>                                 sizes. If for any reason one of
>>>>>>> those
>>>>>>>                                 stack sizes is now too small,
>>>>>>>                                 the
>>>>>>>                                 correct fix is to adjust the initial
>>>>>>>                                 stack sizes, not apply
>>>>>>>                                 thr_min_stack() to the initial stack
>>>>>>>                                 sizes. However, it looks
>>>>>>>                                 like no
>>>>>>>                                 adjustment is needed. I did
>>>>>>> something
>>>>>>>                                 close to our nightly
>>>>>>>                                 testing on
>>>>>>>                                 all affect platforms, and no new
>>>>>>>                                 problems turned up.
>>>>>>>
>>>>>>>                                 thanks,
>>>>>>>
>>>>>>>                                 Chris
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
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 5:04 PM, David Holmes wrote:

> Hi Chris,
>
> On 17/03/2017 8:14 AM, Chris Plummer wrote:
>> On 3/16/17 3:01 PM, David Holmes wrote:
>>> On 17/03/2017 7:43 AM, Chris Plummer wrote:
>>>> On 3/16/17 2:35 PM, David Holmes wrote:
>>>>> On 17/03/2017 3:49 AM, Chris Plummer wrote:
>>>>>> On 3/16/17 2:16 AM, David Holmes wrote:
>>>>>>> On 16/03/2017 6:30 PM, Thomas St�fe wrote:
>>>>>>>> Hi Chris, David,
>>>>>>>>
>>>>>>>> the change looks good.
>>>>>>>>
>>>>>>>> I see that in the launcher we require a minimum stack size across
>>>>>>>> all
>>>>>>>> platforms ("STACK_SIZE_MINIMUM"), should we do the same fix
>>>>>>>> (adjust
>>>>>>>> for
>>>>>>>> PTHREAD_STACK_MIN) there?
>>>>>>>>
>>>>>>>> I do not understand, why does error checking in the hotspot have
>>>>>>>> to be
>>>>>>>> consistent with the launcher? What prevents us from asserting
>>>>>>>> in the
>>>>>>>> hotspot - or at least print a warning? Note that in the hotspot,
>>>>>>>> there
>>>>>>>> is already UL logging ("os", "thread") after pthread_create()
>>>>>>>> in the
>>>>>>>> platform files, so the least we could do is add a warning log
>>>>>>>> output
>>>>>>>> case ppthread_attr_setstacksize fails.
>>>>>>>
>>>>>>> Sorry I'm getting this group of bugs all muddled up.
>>>>>>>
>>>>>>> Chris: this issue does affect hotspot and the launcher
>>>>>>> (potentially).
>>>>>>>
>>>>>>> Ideally both should be checking for failures in the pthread
>>>>>>> calls but
>>>>>>> neither do so. Hotspot at least does so in some places but not in a
>>>>>>> lot of others.
>>>>>>>
>>>>>>> pthread_create is different in hotspot because failure can happen
>>>>>>> easily and we need to detect it and report it (via an exception and
>>>>>>> also via UL). The other pthread calls are not expected to fail
>>>>>>> under
>>>>>>> "normal" conditions but only due to a programming error. Those
>>>>>>> calls
>>>>>>> should at least be checked in debug builds as we already do in
>>>>>>> places
>>>>>>> with assert_status.
>>>>>>>
>>>>>>> The launcher code doesn't do any error checking at all (but again
>>>>>>> pthread_create is a special case).
>>>>>> Are you just referring to the pthread related error checking? It
>>>>>> does do
>>>>>> other error checking.
>>>>>
>>>>> pthread error checking.
>>>>>
>>>>> So trying to think this through ...
>>>>>
>>>>> If the user specifies a too small, or unaligned-to-page-size, -Xss
>>>>> value the pthread_setstacksize() in the launcher will silently fail
>>>>> and the main thread will get the default stack of 8M. It will then
>>>>> load the VM which will then check the -Xss value, which will do its
>>>>> own validity checking.
>>>>>
>>>> Close, except there is still a potential issue if the size is bigger
>>>> than the minimum hotspot requires, but is not page size aligned.
>>>> pthread_setstacksize *could* fail in this case, and there would be no
>>>> "stack size too small" rejection from the hotspot. However,
>>>> pthread_setstacksize did not fail on the two platforms I tried
>>>> unaligned
>>>> stack sizes on.
>>>
>>> Perhaps because that is not specified by POSIX. For POSIX we only have:
>>>
>>> [EINVAL]
>>>     The value of stacksize is less than {PTHREAD_STACK_MIN} or exceeds
>>> a system-imposed limit.
>> The man page on my linux host also adds the warning about "some hosts
>> can fail if the stack size is not a multiple of the system page size."
>> Is the man page documenting something different?
>
> Yes it's documenting that Linux doesn't follow POSIX very well a lot
> of the time. :( However the source code seems different:
>
> http://code.metager.de/source/xref/gnu/glibc/nptl/pthreadP.h
>
> 628 static inline int
> 629 check_stacksize_attr (size_t st)
> 630 {
> 631  if (st >= PTHREAD_STACK_MIN)
> 632    return 0;
> 633
> 634  return EINVAL;
> 635 }
>
> That said, it seems that OSX will return EINVAL:
>
> https://opensource.apple.com/source/libpthread/libpthread-218.1.3/src/pthread.c.auto.html 
>
>
>     int ret = EINVAL;
>     if (attr->sig == _PTHREAD_ATTR_SIG &&
>         (stacksize % vm_page_size) == 0 &&
>         stacksize >= PTHREAD_STACK_MIN) {
>         attr->stacksize = stacksize;
>         ret = 0;
>     }
>     return ret;
Ok. Thanks for finding this. I had found the Mac OS X man page and it
too mentioned that unaligned sizes might cause EINVAL.

>
>>>
>>> Anyway that is a check that hotspot could perform if
>>> pthread_attr_setstacksize fails. Though that then makes me wonder if
>>> we do any rounding when the stack size set on a per thread basis via
>>> the java.lang.Thread constructor?
>>>
>>> I think imposing the PTHREAD_STACK_MIN in hotspot, with an assert
>>> checking pthread_attr_setstacksize succeeded (in hotspot) would
>>> suffice here.
>> If you are certain that the assert would be a programming error and not
>> a user error, then I can see doing that. However, shouldn't we be
>> consistent in the launcher and do the same there also? We can skip
>> imposing PTHREAD_STACK_MIN since hotspot will already do this, but
>> unless the user creates another java thread there will be no hotspot
>> assert for pthread_attr_setstacksize failing.
>
> The launcher has its own policy regarding errors here - it ignores
> them. As I said before if you pass an invalid -Xss value the launcher
> will create the main thread with the default stack size. That is not
> an unreasonable position to take. If you feel strongly about it you
> could file a bug against the launcher, but I would not try to fix it
> in this CR.
Ok. At least it's not fatal.
>
>
> Inside hotspot I think we already do various roundings on the value
> eventually passed to pthread_attr_setstacksize, don't we? So any
> EINVAL after that should be effectively impossible when combined with
> the PTHREAD_STACK_MIN check.
>
Yes:

   size_t stack_size_in_bytes = ThreadStackSize * K;
...
   // Make the stack size a multiple of the page size so that
   // the yellow/red zones can be guarded.
   JavaThread::set_stack_size_at_create(round_to(stack_size_in_bytes,
vm_page_size()));

Maybe I should add to the comment "...and also so
pthread_attr_setstacksize won't fail".

Chris

> Thanks,
> David
>
>> Chris
>>>
>>> David
>>> -----
>>>
>>>> Chris
>>>>> That seems like quite a reasonable position for the launcher to take.
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> Chris
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> If we ever refactor this coding, could we rename the variables
>>>>>>>> holding
>>>>>>>> the base stack size requirement for java frames - in all its
>>>>>>>> incarnations in all the os_cpu files - to be renamed to something
>>>>>>>> different? It is a bit confusing to have a variable which at
>>>>>>>> different
>>>>>>>> times in VM life means different things (before and after the call
>>>>>>>> to os::Posix::set_minimum_stack_sizes()). Or, at least, rename
>>>>>>>> "set_minimum_stack_sizes" to something like
>>>>>>>> "adjust_minimum_stack_sizes"
>>>>>>>> which makes the intent clearer.
>>>>>>>>
>>>>>>>> Kind Regards, Thomas
>>>>>>>>
>>>>>>>> On Thu, Mar 16, 2017 at 7:50 AM, David Holmes
>>>>>>>> <[hidden email]
>>>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>>>
>>>>>>>>     On 16/03/2017 4:33 PM, Chris Plummer wrote:
>>>>>>>>
>>>>>>>>         On 3/15/17 11:18 PM, David Holmes wrote:
>>>>>>>>
>>>>>>>>             On 16/03/2017 4:14 PM, Chris Plummer wrote:
>>>>>>>>
>>>>>>>>                 On 3/15/17 11:11 PM, David Holmes wrote:
>>>>>>>>
>>>>>>>>                     On 16/03/2017 3:51 PM, Chris Plummer wrote:
>>>>>>>>
>>>>>>>>                         On 3/15/17 10:23 PM, David Holmes wrote:
>>>>>>>>
>>>>>>>>                             Hi Chris,
>>>>>>>>
>>>>>>>>                             On 16/03/2017 3:03 PM, Chris Plummer
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>                                 Hello,
>>>>>>>>
>>>>>>>>                                 Please review the following:
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176768
>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8176768>
>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot 
>>>>>>>>
>>>>>>>>
>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8176768/webrev.00/webrev.hotspot>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                             Change looks good.
>>>>>>>>
>>>>>>>>                                 While working on 8175342 I
>>>>>>>> noticed our
>>>>>>>>                                 stack size on xgene was 8mb
>>>>>>>>                                 even
>>>>>>>>                                 though I was specifying
>>>>>>>> -Xss72k. It
>>>>>>>>                                 turns out the following code was
>>>>>>>>                                 failing:
>>>>>>>>
>>>>>>>> pthread_attr_setstacksize(&attr,
>>>>>>>>                                 stack_size);
>>>>>>>>
>>>>>>>>
>>>>>>>>                             So these really should be checking
>>>>>>>> return
>>>>>>>>                             values, at least in debug
>>>>>>>>                             builds. But we can leave that until we
>>>>>>>>                             refactor the thread startup
>>>>>>>>                             code into os_posix.cpp.
>>>>>>>>
>>>>>>>>                         I considered adding checks. I wasn't sure
>>>>>>>> if we
>>>>>>>>                         should abort or just
>>>>>>>>                         print a warning if it failed.
>>>>>>>>
>>>>>>>>
>>>>>>>>                     When we check pthread lib routines we use:
>>>>>>>>
>>>>>>>>                        int status = pthread_mutex_lock(_mutex);
>>>>>>>>                        assert_status(status == 0, status,
>>>>>>>> "mutex_lock");
>>>>>>>>
>>>>>>>>                     This is for things that should only fail if we
>>>>>>>> have
>>>>>>>>                     a programming
>>>>>>>>                     error.
>>>>>>>>
>>>>>>>>                 Ok, but this is in the launcher, so I'll need to
>>>>>>>> just
>>>>>>>>                 use the built-in
>>>>>>>>                 assert(). I'll add that if want.
>>>>>>>>
>>>>>>>>
>>>>>>>>             Oops! I was forgetting that. Need to be consistent
>>>>>>>> with
>>>>>>>>             launcher error
>>>>>>>>             checking or lack thereof. And ignore refactoring
>>>>>>>> comments -
>>>>>>>>             not relevant.
>>>>>>>>
>>>>>>>>         So don't add the error check?
>>>>>>>>
>>>>>>>>
>>>>>>>>     Given there is no error checking, or assertions, in those
>>>>>>>> files I
>>>>>>>>     reluctantly have to say leave it out.
>>>>>>>>
>>>>>>>>     Thanks,
>>>>>>>>     David
>>>>>>>>     -----
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>             David
>>>>>>>>
>>>>>>>>                 Chris
>>>>>>>>
>>>>>>>>
>>>>>>>>                         What refactoring is planned?
>>>>>>>>
>>>>>>>>
>>>>>>>>                     "Planned" might be a bit strong :) I was
>>>>>>>> thinking of
>>>>>>>>                     a number of
>>>>>>>>                     os_posix related cleanups for which issues
>>>>>>>> exist,
>>>>>>>>                     but also forgot that
>>>>>>>>                     some of our general clean-up RFE's have been
>>>>>>>> closed
>>>>>>>>                     as WNF :( I may do
>>>>>>>>                     some of them after hours anyway :)
>>>>>>>>
>>>>>>>>                     David
>>>>>>>>                     -----
>>>>>>>>
>>>>>>>>                         Chris
>>>>>>>>
>>>>>>>>
>>>>>>>>                             Thanks,
>>>>>>>>                             David
>>>>>>>>                             -----
>>>>>>>>
>>>>>>>>                                 Although we computed a minimum
>>>>>>>> stack
>>>>>>>>                                 size of 72k, so -Xss72k
>>>>>>>>                                 should be
>>>>>>>>                                 fine, pthreads on this platform
>>>>>>>> requires
>>>>>>>>                                 the stack be at least
>>>>>>>>                                 128k, so
>>>>>>>>                                 it failed the
>>>>>>>> pthread_attr_setstacksize() call. The
>>>>>>>>                                 end result is
>>>>>>>> pthread_attr_setstacksize() had no
>>>>>>>>                                 impact on the thread's stack
>>>>>>>>                                 size,
>>>>>>>>                                 and we ended up with the platform
>>>>>>>>                                 default of 8mb. The fix is to
>>>>>>>>                                 round up
>>>>>>>>                                 the following variables to
>>>>>>>>                                 PTHREAD_STACK_MIN after computing
>>>>>>>>                                 their new
>>>>>>>>                                 values:
>>>>>>>>
>>>>>>>> _java_thread_min_stack_allowed
>>>>>>>> _compiler_thread_min_stack_allowed
>>>>>>>> _vm_internal_thread_min_stack_allowed
>>>>>>>>
>>>>>>>>                                 For solaris, there was an issue
>>>>>>>> using
>>>>>>>>                                 PTHREAD_STACK_MIN. You need to
>>>>>>>>                                 #define _POSIX_C_SOURCE >=
>>>>>>>> 199506L in
>>>>>>>>                                 order to get PTHREAD_STACK_MIN
>>>>>>>>                                 #defined, and this needs to be
>>>>>>>> done
>>>>>>>>                                 before including OS header
>>>>>>>>                                 files. I
>>>>>>>>                                 noticed that on solaris we were
>>>>>>>> using
>>>>>>>>                                 thr_min_stack() elsewhere
>>>>>>>>                                 instead
>>>>>>>>                                 of PTHREAD_STACK_MIN, so I decided
>>>>>>>> to do
>>>>>>>>                                 the same with this fix.
>>>>>>>>                                 Either
>>>>>>>>                                 way is ugly (the #define or using
>>>>>>>>                                 thr_min_stack()).
>>>>>>>>
>>>>>>>>                                 And speaking of the existing
>>>>>>>> use of
>>>>>>>>                                 thr_min_stack(), I deleted
>>>>>>>>                                 it. It
>>>>>>>>                                 was being applied before any
>>>>>>>> adjustments
>>>>>>>>                                 to the stack sizes had been
>>>>>>>>                                 made (rounding and adding red,
>>>>>>>> yellow,
>>>>>>>>                                 and shadow zones). This mean
>>>>>>>>                                 the
>>>>>>>>                                 stack ended up being larger than
>>>>>>>>                                 necessary. With the above fix in
>>>>>>>>                                 place,
>>>>>>>>                                 we are now applying
>>>>>>>> thr_min_stack()
>>>>>>>>                                 after recomputing the minimum
>>>>>>>>                                 stack
>>>>>>>>                                 sizes. If for any reason one of
>>>>>>>> those
>>>>>>>>                                 stack sizes is now too small,
>>>>>>>>                                 the
>>>>>>>>                                 correct fix is to adjust the
>>>>>>>> initial
>>>>>>>>                                 stack sizes, not apply
>>>>>>>>                                 thr_min_stack() to the initial
>>>>>>>> stack
>>>>>>>>                                 sizes. However, it looks
>>>>>>>>                                 like no
>>>>>>>>                                 adjustment is needed. I did
>>>>>>>> something
>>>>>>>>                                 close to our nightly
>>>>>>>>                                 testing on
>>>>>>>>                                 all affect platforms, and no new
>>>>>>>>                                 problems turned up.
>>>>>>>>
>>>>>>>>                                 thanks,
>>>>>>>>
>>>>>>>>                                 Chris
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>


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
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 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.

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

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

please find my answers inline.

On Fri, Mar 17, 2017 at 5:27 AM, Chris Plummer <[hidden email]>
wrote:

> Ok, time for a new webrev:
>
> http://cr.openjdk.java.net/~cjplummer/8176768/webrev.01/webrev.hotspot
>
>
The changes look good.


> 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.
>

As a side note, on AIX we have a complicated page scheme where thread
stacks may have a different page size from the java heap or from the
primordial thread. But using os::vm_page_size() should be ok, we ensure
that os::vm_page_size is always the same or a multiple of the thread stack
page size.

Would you mind adding the assert to os_aix.cpp too?


>  - 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
>
>
I am all fine with the changes.

Kind Regards, Thomas


>
> 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