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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Mon Dec 14 15:11:38 UTC 2015 

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.

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

Best regards,
  Goetz.



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

More information about the core-libs-dev mailing list