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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Review request 6983240: guarantee((Solaris::min_stack_allowed >= (StackYellowPages+StackRedPages...)
From:       Dmitriy.Samersoff () oracle ! com (Dmitriy Samersoff)
Date:       2010-10-07 19:06:47
Message-ID: 4CAE1A47.4030908 () oracle ! com
[Download RAW message or body]

Coleen,

Thank you for the explanation.

-Dmitry


On 2010-10-07 22:11, Coleen Phillimore wrote:
> 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
>>>>>
>>>>
>>>
>>
>>


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

[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic