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.par...@oracle.com] > Sent: Montag, 14. Dezember 2015 15:44 > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Karen Kinnear > <karen.kinn...@oracle.com> > Cc: hotspot-...@openjdk.java.net; core-libs-dev <core-libs- > d...@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-boun...@openjdk.java.net] On > >> Behalf Of Frederic Parain > >> Sent: Montag, 14. Dezember 2015 14:24 > >> To: Karen Kinnear <karen.kinn...@oracle.com> > >> Cc: hotspot-...@openjdk.java.net; core-libs-dev <core-libs- > >> d...@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.par...@oracle.com <mailto:frederic.par...@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.par...@oracle.com > <mailto:frederic.par...@oracle.com> > >>> > >> > >> -- > >> Frederic Parain - Oracle > >> Grenoble Engineering Center - France > >> Phone: +33 4 76 18 81 17 > >> Email: frederic.par...@oracle.com > > -- > Frederic Parain - Oracle > Grenoble Engineering Center - France > Phone: +33 4 76 18 81 17 > Email: frederic.par...@oracle.com
