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

[Dmitriy Samersoff]

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 ?

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


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.

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


-- 
Dmitry Samersoff
J2SE Sustaining team, SPB04
* Give Rabbit time and he'll always get the answer ...