Review request 6983240: guarantee((Solaris::min_stack_allowed >= (StackYellowPages+StackRedPages...) wrong

Thu Oct 7 11:11:58 PDT 2010

I just checked it in...  sorry I didn't wait.

Dmitriy Samersoff wrote:
> Coleen,
>
> Code looks good for me but I have a few questions (below):
>
> 1.
>
> Is it possible to create
>
> static inline size_t min_stack_allowed(){
>     return
>     (size_t)(StackYellowPages+StackRedPages+StackShadowPages+ 
>  2*BytesPerWord COMPILER2_PRESENT(+1)) * os::vm_page_size();
>
> in os.hpp ?
>
This would have been better but the constant value for min_stack_allowed 
had some comment about being documented, so I didn't want to mess with 
that.  I don't actually know where it's documented though.
> 1".
> If not:
>
>   os_linux.cpp   uses  Linux::page_size()
>   os_solaris.cpp uses  page_size
>   os_windows.cpp uses  os::vm_page_size()
>
> I believe all three returns a correct value but is it possible to keep 
> the same semantics across OSes i.e. always use either 
> {Linux,Solaris,Windows}::page_size or os::vm_page_size() ?
>
This does need cleanup.  They should all use os::vm_page_size().  I 
don't know why there was ever a linux variant.  On solaris, you can have 
bigger page sizes so the semantics are more complicated.
>
> 2. We don't store calculated min_stack_allowed under windows.
>    I'm not a windows expert - do we have a reason for it?
>      And (a nit) I'm a little bit worried about cast from size_t
>    to int here.
It apparently was never in the documentation so I didn't want to add 
it.  Windows default page size is really big.  The windows didn't 
compile this final version so I had to change the local variable 
min_stack_allowed to a size_t to get it through jprt, so the cast is gone.

Thanks,
Coleen
>
> -Dmitry
>
>
>
> On 2010-10-06 22:53, Coleen Phillimore wrote:
>> -------- Original Message --------
>> Subject:     Re: Review request 6983240:
>> guarantee((Solaris::min_stack_allowed >=
>> (StackYellowPages+StackRedPages...) wrong
>> Date:     Wed, 06 Oct 2010 11:19:13 -0700
>> From:     Vladimir Kozlov <vladimir.kozlov at oracle.com>
>> To:     Coleen Phillimore <coleen.phillimore at oracle.com>
>> CC:     hotspot-runtime-dev at openjdk.java.net
>> References:     <4CAA58F4.5030100 at oracle.com> 
>> <4CAA743C.3060905 at oracle.com>
>> <4CAB43A0.60001 at oracle.com> <4CACB80D.6060708 at oracle.com>
>>
>>
>>
>> Looks good for me.
>>
>> Vladimir
>>
>> Coleen Phillimore wrote:
>>>
>>>  Can I get one more review?
>>>
>>>  webrev athttp://cr.openjdk.java.net/~coleenp/6983240_2/
>>>
>>>  Thanks,
>>>  Coleen
>>>
>>>
>>>
>>>  On 10/05/10 11:26, Coleen Phillimore wrote:
>>>>
>>>>  Thanks David - some comments inline...
>>>>
>>>>  On 10/04/10 20:41, David Holmes wrote:
>>>>>  Hi Coleen,
>>>>>
>>>>>  In os_solaris.cpp this comment should now refer to 2*BytesPerWord
>>>>  Okay.
>>>>>
>>>>>    // Add in BytesPerWord times page size to account for VM stack 
>>>>> during
>>>>>    // class initialization depending on 32 or 64 bit VM.
>>>>>
>>>>>  That said this formulation (not your I know) is confusing. What does
>>>>>  bytesPerWord have to do with the amount of stack needed? Is this 
>>>>> just
>>>>>  some weird way of getting twice the amount of stack on 64-bit as
>>>>>  32-bit? Are the pages sizes even the same in that case?
>>>>  Yeah, I believe it's just to account for 64 vs. 32 bit.  The page
>>>>  sizes are the same if 32 or 64 bit.  I have
>>>>>
>>>>>  Also in os_solaris.cpp isn't this guarantee always false:
>>>>>
>>>>>  !   guarantee(os::Solaris::min_stack_allowed>= thr_min_stack(),
>>>>>  !             "need to increase min_stack_allowed");
>>>>  No, it doesn't assert, because in your next mail you found that
>>>>  thr_min_stack() is smaller than our min_stack_allowed.  I could 
>>>> revert
>>>>  that change but min_stack_allowed() is for the VM to get initialized
>>>>  rather than this thr_min_stack() which is for the OS to initialize 
>>>> the
>>>>  thread.  I would assume the latter is always smaller than the former
>>>>  or something is really wrong.  Changing it back probably doesn't hurt
>>>>  anything.
>>>>
>>>>  Thanks,
>>>>  Coleen
>>>>>
>>>>>  AFAICS min_stack_allowed is set to one of these values:
>>>>>
>>>>>  ./os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp:size_t
>>>>>  os::Solaris::min_stack_allowed = 128*K;
>>>>>  ./os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp:size_t
>>>>>  os::Solaris::min_stack_allowed = 96*K;
>>>>>  ./os_cpu/solaris_x86/vm/os_solaris_x86.cpp:size_t
>>>>>  os::Solaris::min_stack_allowed = 224*K;
>>>>>  ./os_cpu/solaris_x86/vm/os_solaris_x86.cpp:size_t
>>>>>  os::Solaris::min_stack_allowed = 64*K;
>>>>>
>>>>>  but the manpage for thr_min_stack states:
>>>>>
>>>>>      thr_min_stack() will return the unsigned int  THR_MIN_STACK,
>>>>>      which is the minimum-allowable size for a thread's stack.
>>>>>
>>>>>      In this implementation the default size for a  user-thread's
>>>>>      stack   is   one   mega-byte.
>>>>>
>>>>>  1MB>  all the above. ???
>>>>>
>>>>>  Cheers,
>>>>>  David
>>>>>
>>>>>  Coleen Phillimore said the following on 10/05/10 08:45:
>>>>>>  Summary: min_stack_allowed is a compile time constant and
>>>>>>  Stack*Pages are settable
>>>>>>
>>>>>>  Also stress tested various combinations of -Xss and
>>>>>>  -XX:StackShadowPages=n.  This also fixes a bug
>>>>>>  6346701: stack overflow in native method results in segfault, not a
>>>>>>  StackOverflowError
>>>>>>  or rather it fixes the observed SEGVs with different settings of
>>>>>>  -Xss and -XX:StackShadowPages in the evaluation.  We were getting
>>>>>>  stack overflow before the stack overflow exception was initialized,
>>>>>>  causing infinite recusion trying to initialize it.
>>>>>>
>>>>>>  Also fixed a random g++ compilation error.
>>>>>>
>>>>>>  open webrev athttp://cr.openjdk.java.net/~coleenp/6983240/
>>>>>>  bug link athttp://bugs.sun.com/view_bug.do?bug_id=6983240
>>>>>>
>>>>>>  Thanks,
>>>>>>  Coleen
>>>>
>>>
>>
>
>