RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

Frederic Parain frederic.parain at oracle.com
Mon Dec 14 15:19:41 UTC 2015 

Goetz,

On 14/12/2015 16:11, Lindenmaier, Goetz wrote:
> Hi Frederic,
>
> thanks for your fast reply.
>  From reading the code I guessed about what you explained, now
> I understood more details, thanks!
>
> I will not fiddle with handling the zones. My change is only about
> handling larger pages, i.e. the sizes of the zones, so there is no
> danger I will break the mechanism you implemented.
>
>> The take over is that the VM code should be able to manage
>> the reserved zone alone or the reserved zone and the yellow
>> zone together.
>> If you want to change it for a better name, more explicit,
>> I'm fine with that.
> Yes, I would like to change it to 'yellow_reserved' wherever
> both are handled at the same time.
> There's places where red+yellow+reserved are handled as
> one,  here I will use 'guard_zone'.  That's aliged with
> create_stack_guard_pages() which guards all (red, yellow,
> reserved) of them.

I believe that red+yellow+reserved are handled as one only
during stack initialization. Once configured, the only
moment when the red zone protection is changed is when the
VM is about to generate a crash dump.

Anyway, 'yellow_reserved' sounds good to me.

>>     stack_yellow_zone_enabled() -> stack_guards_enabled()
>>
>> Here, "guards" refers to the two guard zones: reserved
>> + yellow.
> Actually, it also includes that the red page is enabled, right?
>
> One question, in os_linux.cpp, you meant AMD64, not Itanium, right?
> 27.18+#if defined(IA32) || defined(IA64)
> I'll move that functionality to another place, so no need to fix it.

You're right, I meant AMD64.

Thanks,

Fred

>> -----Original Message-----
>> From: Frederic Parain [mailto:frederic.parain at oracle.com]
>> Sent: Montag, 14. Dezember 2015 15:44
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Karen Kinnear
>> <karen.kinnear at oracle.com>
>> Cc: hotspot-dev at openjdk.java.net; core-libs-dev <core-libs-
>> dev at openjdk.java.net>
>> Subject: Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical
>> Sections
>>
>> Goetz,
>>
>> The reserved zone is sometime considered as a subpart of
>> the yellow zone, and sometime as an independent entity.
>> Technically the reserved zone is placed just before the
>> yellow zone, but the way it is managed depends on the
>> context.
>>
>> In most cases, there's no specially annotated methods on
>> the thread's call stack. In this configuration the
>> reserved zone is considered as being part of the yellow
>> zone. This means that the protection of the reserved
>> zone is always the same as the protection of the yellow
>> zone.
>>
>> In some cases, a thread could have one or more methods
>> on its call stack with the ReservedStackAccess annotation.
>> In this configuration, the reserved zone is managed as
>> a separate entity. Initially protected like the yellow
>> zone, access to the reserved zone can be temporary granted
>> for the execution of some critical sections. This means
>> that the protection of the reserved zone can be removed
>> while the yellow zone is still protected.
>>
>> The take over is that the VM code should be able to manage
>> the reserved zone alone or the reserved zone and the yellow
>> zone together. I've already renamed a method because of
>> this change:
>>     stack_yellow_zone_enabled() -> stack_guards_enabled()
>>
>> Here, "guards" refers to the two guard zones: reserved
>> + yellow.
>>
>> If you want to change it for a better name, more explicit,
>> I'm fine with that. We just have to preserve the different
>> operations required for stack overflow management.
>>
>> Let's say R(x) and Y(x) represent the protection of
>> the reserved zone and the yellow zone respectively.
>> And let's say that x = P means "zone protected"
>> and x = G means "access granted"
>>
>> The different configurations are:
>>
>> (1) R(P) Y(P)  => initial configuration
>> (2) R(G) Y(P)  => first stack overflow with annotated
>>                     method on stack
>> (3) R(G) Y(G)  => stack overflow without annotated
>>                     method on stack, or second stack
>>                     overflow with annotated method
>>                     on stack
>>
>> And the transitions are:
>>
>> (1) -> (3) -> (1)
>> or
>> (1) -> (2) -> (1)
>> or
>> (1) -> (2) -> (3) -> (1)
>>
>> I hope this would clarify the semantic of the reserved zone.
>> If it doesn't, let me know, I'll try to explain it differently.
>>
>> Thanks,
>>
>> Fred
>>
>> On 14/12/2015 14:59, Lindenmaier, Goetz wrote:
>>> Hi Frederic,
>>>
>>> I'm now again working on my change "8139864: Improve handling of stack
>> protection zones."
>>> Coleen had asked me to wait until this change of yours is submitted.
>>>
>>> You changed the stack_yellow_zone accessor functions in thread.hpp to
>>> handle both zones, yellow and reserved.
>>> Therefore, reading the code, the reserved zone seems to be part of the
>> yellow zone.
>>>
>>> In the description of the bug, it says "The new zone defined by the
>> proposed
>>> solution is placed just before the yellow zone." This reads as if the zones
>> are
>>> separate.
>>>
>>> Would you mind if I rename the stack_yellow_zone accessor functions to
>>> stack_yellow_reserved_zone?  This would make clear in the code that this
>>> are two separate zones.
>>>
>>> I anyways have to adapt most of the calls to these accessors.
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On
>>>> Behalf Of Frederic Parain
>>>> Sent: Montag, 14. Dezember 2015 14:24
>>>> To: Karen Kinnear <karen.kinnear at oracle.com>
>>>> Cc: hotspot-dev at openjdk.java.net; core-libs-dev <core-libs-
>>>> dev at openjdk.java.net>
>>>> Subject: Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for
>> Critical
>>>> Sections
>>>>
>>>> Karen,
>>>>
>>>> Thank you for your review.
>>>>
>>>> Fred
>>>>
>>>> On 23/11/2015 20:10, Karen Kinnear wrote:
>>>>> Frederic,
>>>>>
>>>>> Looks good.
>>>>>
>>>>> Many thanks.
>>>>> Karen
>>>>>
>>>>>> On Nov 23, 2015, at 12:44 PM, Frederic Parain
>>>>>> <frederic.parain at oracle.com <mailto:frederic.parain at oracle.com>>
>>>> wrote:
>>>>>>
>>>>>> Karen,
>>>>>>
>>>>>> Thank you for your review, my answers are in-lined below.
>>>>>>
>>>>>> New Webrevs (including some fixes suggested by Paul Sandoz):
>>>>>>
>>>>>> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/
>>>>>> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/
>>>>>>
>>>>>> On 20/11/2015 19:44, Karen Kinnear wrote:
>>>>>>> Frederic,
>>>>>>>
>>>>>>> Code review for web revs you sent out.
>>>>>>> Code looks good. I am not as familiar with the compiler code.
>>>>>>>
>>>>>>> I realize you need to check in at least a subset of the
>>>>>>> java.util.concurrent changes for
>>>>>>> the test to work, so maybe I should not have asked Doug about his
>>>>>>> preference there.
>>>>>>> Sorry.
>>>>>>>
>>>>>>> I am impressed you found a way to make a reproducible test!
>>>>>>>
>>>>>>> Comments/questions:
>>>>>>> 1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”
>>>>>>
>>>>>> Fixed
>>>>>>
>>>>>>> 2. IIRC, due to another bug with windows handling of our guard pages,
>>>>>>> this
>>>>>>> is disabled for Windows. Would it be worth putting a comment in the
>>>>>>> bug , 8067946, that once this is fixed, the reserved stack logic on
>>>>>>> windows
>>>>>>> will need testing before enabling?
>>>>>>
>>>>>> More than testing, the implementation would have to be completed
>> on
>>>>>> Windows. I've added a comment to JDK-8067946.
>>>>>>
>>>>>>> 3. In get_frame_at_stack_banging_point on Linux, BSD and
>> solaris_x86 if
>>>>>>> this is in interpreter code, you explicitly return the Java sender
>>>>>>> of the current frame. I was expecting to see that on Solaris_sparc
>>>>>>> and Windows
>>>>>>> as well? I do see the assertion in caller that you do have a java frame.
>>>>>>
>>>>>> It doesn't make sense to check the current frame (no bytecode has
>> been
>>>>>> executed yet, so risk of partially executed critical section). I did the
>>>>>> change but not for all platforms. This is now fixed for Solaris_SPARC
>>>>>> and Windows too.
>>>>>>
>>>>>>> 4. test line 83 “writtent” -> “written”
>>>>>>
>>>>>> Fixed
>>>>>>
>>>>>>> 5. I like the way you set up the preallocated exception and then set
>>>>>>> the message - we may
>>>>>>> try that for default methods in future.
>>>>>>>
>>>>>>> 6. I had a memory that you had found a bug in safe_for_sender - did
>>>>>>> you already check that in?
>>>>>>
>>>>>> I've fixed x86 platforms in JDK-8068655.
>>>>>> I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
>>>>>> I had hoped it would have been silently accepted :-)
>>>>>>
>>>>>>
>>>>>>> 7. I see the change in trace.xml, and I see an include added to
>>>>>>> SharedRuntime.cpp,
>>>>>>> but I didn’t see where it was used - did I just miss it?
>>>>>>
>>>>>> trace.xml changes define a new event.
>>>>>> This event is created at sharedRuntime.cpp::3006 and it is used
>>>>>> in the next 3 lines.
>>>>> Thanks. I must have mistyped when I searched for it.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Fred
>>>>>>
>>>>>> --
>>>>>> Frederic Parain - Oracle
>>>>>> Grenoble Engineering Center - France
>>>>>> Phone: +33 4 76 18 81 17
>>>>>> Email:Frederic.Parain at oracle.com
>> <mailto:Frederic.Parain at oracle.com>
>>>>>
>>>>
>>>> --
>>>> Frederic Parain - Oracle
>>>> Grenoble Engineering Center - France
>>>> Phone: +33 4 76 18 81 17
>>>> Email: Frederic.Parain at oracle.com
>>
>> --
>> Frederic Parain - Oracle
>> Grenoble Engineering Center - France
>> Phone: +33 4 76 18 81 17
>> Email: Frederic.Parain at oracle.com

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com

More information about the core-libs-dev mailing list