RFR(M): 8139864: Improve handling of stack protection zones.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Dec 18 09:42:25 UTC 2015
HI,
Coleen, is this fine with you, too?
Could someone please sponsor the change?
Thanks,
Goetz.
> -----Original Message-----
> From: Frederic Parain [mailto:frederic.parain at oracle.com]
> Sent: Freitag, 18. Dezember 2015 10:39
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Coleen Phillimore'
> <coleen.phillimore at oracle.com>; David Holmes
> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(M): 8139864: Improve handling of stack protection zones.
>
> Still looks good to me.
>
> Thank you,
>
> Fred
>
> On 12/17/2015 09:59 PM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > Our test passed successful after I did two minor
> > changes: remove the assertion that _stack_reserved_zone_size > 0
> > and fix a windows build issue. I also did the change proposed
> > by Frederic.
> > New webrev:
> > http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> StackZones/webrev.04/
> >
> > Best regards,
> > Goetz.
> >
> >
> >
> >> -----Original Message-----
> >> From: Frederic Parain [mailto:frederic.parain at oracle.com]
> >> Sent: Wednesday, December 16, 2015 10:42 AM
> >> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Coleen
> Phillimore'
> >> <coleen.phillimore at oracle.com>; David Holmes
> >> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> >> Subject: Re: RFR(M): 8139864: Improve handling of stack protection
> zones.
> >>
> >> Hi Goetz,
> >>
> >> Thank you for fixing this and thank you for having delayed
> >> your work to let me push JDK-8046936.
> >>
> >> On 12/15/2015 11:17 PM, Lindenmaier, Goetz wrote:
> >>> Hi,
> >>>
> >>> I reworked my change fixing handling of stack zone sizes with
> >>> pages > 4K to fit on top of "8046936 : JEP 270: Reserved Stack Areas for
> >> Critical Sections"
> >>> The functional core is still
> >> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >> StackZones/webrev.00-basic/,
> >>> now extended for StackReservedPages.
> >>> All the other changes are replacing local computations by shared utility
> >> functions.
> >>>
> >>> I slightly changed handling StackReservedPages in case of large pages:
> >>> I now align to page size. Before, it was set to '1'.
> >>>
> >>> In addition, I did a few cleanups.
> >>>
> >>> 8046936 changed yellow zone accessors to handle both, yellow and
> >> reseved zones.
> >>> In some places, I need the pure functions for the yellow zone, so I had
> >>> name clashes. Therefore I renamed all functions handling reserved and
> >> yellow
> >>> zone together to be named 'yellow_reserved'.
> >>>
> >>> I introduced stack_end(), as I found stack_base() - stack_size() in lots
> >>> of places.
> >>> I am using on_local_stack(addr) in many places where an address was
> >> checked
> >>> to be on the stack (actually, I think in_stack(addr) would be a cleaner
> >>> name).
> >>> Also, I introduced usage of existing stack_overflow_limit() where that
> >> address
> >>> was computed.
> >>>
> >>> I think StackReservedPages is missing in the following places:
> >>> frame_aarch64.cpp
> >>> frame_x86.cpp
> >>> templateInterpreterGenerator_aarch64.cpp:476
> >>> templateInterpreterGenerator_sparc.cpp
> >>> stack_zero.inline.hpp (fixed)
> >>> os_solaris.cpp:4462
> >>> os_windows.cpp:4196
> >>
> >> The reserved area is not implemented on aarch64 and cannot be
> >> currently implemented on Windows because of JDK-8067946.
> >> In frame_x86.cpp the code is correct, when access to the reserved
> >> zone has been granted, it is legal to have a stack pointer in this
> >> zone.
> >> The two issues on SPARC are real, I'm fixing them.
> >>
> >>> I did not adapt these places, this should go into an extra change
> >>> if it is not left out on purpose.
> >>>
> >>> I left out changes to cppInterpreter_<cpu>.cpp files.
> >>>
> >>> Please review this change. I please need a sponsor.
> >>> Tests are planned tomorrow night (I missed our today's deadline).
> >>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >> StackZones/webrev.03/
> >>
> >> thread.hpp:
> >>
> >> lines 1429-1436: commented code
> >>
> >> Otherwise, looks good to me.
> >>
> >> Regards,
> >>
> >> Fred
> >>
> >>>> -----Original Message-----
> >>>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
> >>>> Sent: Friday, December 04, 2015 8:59 PM
> >>>> To: David Holmes <david.holmes at oracle.com>; Lindenmaier, Goetz
> >>>> <goetz.lindenmaier at sap.com>; hotspot-runtime-
> dev at openjdk.java.net
> >>>> Subject: Re: RFR(M): 8139864: Improve handling of stack protection
> zones.
> >>>>
> >>>>
> >>>> Hi Goetz,
> >>>>
> >>>> I've reviewed this and I think it looks really good, but it is going to
> >>>> merge conflict with with Frederic's change for RFR(L): JDK-8046936 : JEP
> >>>> 270: Reserved Stack Areas for Critical Sections, which he's checking in
> >>>> early next week.
> >>>>
> >>>> Fred has added a ReservedStackArea for extra protection for certain
> >>>> methods, ie these methods can use this extra stack. Fred's change is
> >>>> almost ready to integrate (sorry we didn't get to this before the
> >>>> conflict arose). Also, I think the reserved pages will have to be
> >>>> changed to follow this model too.
> >>>>
> >>>> Two things about this change also that worry me. This should force
> some
> >>>> sort of compilation error or assertion if new code uses
> >>>> Stack{Red,Yellow,Shadow}Pages. Maybe the values should be
> poisoned
> >> to
> >>>> be something, although I don't know what.
> >>>>
> >>>> The second thing is that I think it would be nice to move all this stack
> >>>> handling stuff out into it's own class (move Thread::_stack_base and
> >>>> Thread::_stack_size out too) into threadStack.hpp/cpp. That could be a
> >>>> follow-on cleanup.
> >>>>
> >>>> This change also has to merge with my interpreter change which you
> were
> >>>> so nice to review so quickly.
> >>>>
> >>>> Lastly, making StackShadowPages mean 4K pages might affect some
> >>>> customers that have tuned this or because they have large pages set
> the
> >>>> value to 1. Unfortunately, I think this should have an internal
> >>>> compatibility request, release note and alert our sustaining team. Most
> >>>> of our public command line options get this treatment when they
> change.
> >>>> I will take care of this also early next week.
> >>>>
> >>>> Thanks,
> >>>> Coleen
> >>>>
> >>>> On 12/4/15 2:39 AM, David Holmes wrote:
> >>>>> On 2/12/2015 2:10 AM, Lindenmaier, Goetz wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> I ran the change through our night builds - all jtreg tests are clean.
> >>>>>> (and the other tests as well.)
> >>>>>> I prepared a complete and - from my side - final webrev:
> >>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >>>> StackZones/webrev.01/
> >>>>>> This includes the fixes proposed by David as well as some minor fixes
> to
> >>>>>> get it compiling on windows.
> >>>>>>
> >>>>>> So to capture the change still the split webrevs might be a good
> >>>>>> starting point:
> >>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >>>> StackZones/webrev.00-basic/
> >>>>>>
> >>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >>>> StackZones/webrev.00-spread/
> >>>>>>
> >>>>>>
> >>>>>> David, can I consider this reviewed?
> >>>>>
> >>>>> I will put a small 'r' review on it. :)
> >>>>>
> >>>>>> I please need a second reviewer and a sponsor.
> >>>>>
> >>>>> Pinging Coleen for Review.
> >>>>>
> >>>>> Thanks,
> >>>>> David
> >>>>>
> >>>>>
> >>>>>> Best regards,
> >>>>>> Goetz.
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> >>>>>>> bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
> >>>>>>> Sent: Freitag, 27. November 2015 10:28
> >>>>>>> To: David Holmes <david.holmes at oracle.com>; Coleen Phillimore
> >>>>>>> (coleen.phillimore at oracle.com) <coleen.phillimore at oracle.com>;
> >>>> hotspot-
> >>>>>>> runtime-dev at openjdk.java.net
> >>>>>>> Subject: RE: RFR(M): 8139864: Improve handling of stack protection
> >>>>>>> zones.
> >>>>>>>
> >>>>>>> Hi David,
> >>>>>>>
> >>>>>>> thanks for looking at this change!
> >>>>>>> I edited the thinks you remarked, thanks for spotting that.
> >>>>>>>
> >>>>>>> I'll run the change through our testing to assure I catch all
> >>>>>>> issues with jtreg. We have some systems with other page sizes.
> >>>>>>>
> >>>>>>> Best regards,
> >>>>>>> Goetz.
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>>>>>>> Sent: Freitag, 27. November 2015 06:33
> >>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Coleen
> >>>> Phillimore
> >>>>>>>> (coleen.phillimore at oracle.com)
> <coleen.phillimore at oracle.com>;
> >>>>>>> hotspot-
> >>>>>>>> runtime-dev at openjdk.java.net
> >>>>>>>> Subject: Re: RFR(M): 8139864: Improve handling of stack
> protection
> >>>>>>>> zones.
> >>>>>>>>
> >>>>>>>> Hi Goetz,
> >>>>>>>>
> >>>>>>>> On 26/11/2015 1:55 AM, Lindenmaier, Goetz wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> For a description of the problem see
> >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8139864
> >>>>>>>>>
> >>>>>>>>> This is a first version of my implementation of the proposal '3'
> >>>>>>>>> that was
> >>>>>>>> discussed here:
> >>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-
> >> dev/2015-
> >>>>>>>> October/016085.html
> >>>>>>>>> which is continued here
> >>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-
> >> dev/2015-
> >>>>>>>> November/016805.html
> >>>>>>>>>
> >>>>>>>>> Opposed to my original proposal, I did not introduce new flags. I
> >>>>>>>> documented the
> >>>>>>>>> old flags to specify 4K pages, and save the ergonomic values in
> >>>>>>>>> static
> >>>>>>> fields
> >>>>>>>> of JavaThread.
> >>>>>>>>> This avoids two sets of similar flags.
> >>>>>>>>
> >>>>>>>> That seems like a good compromise. Though not sure about
> >> placement
> >>>> in
> >>>>>>>> JavaThread when everything else stack-related seems to be in os
> >> class.
> >>>>>>>>
> >>>>>>>>> To simplify reviewing, I made two webrevs for the first round:
> >>>>>>>>> The basic changes to determining the sizes of the zones to
> protect
> >>>>>>>>> are in
> >>>>>>>> this partial webrev:
> >>>>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >>>>>>>> StackZones/webrev.00-basic/
> >>>>>>>>> This contains the functional change.
> >>>>>>>>
> >>>>>>>> This seems okay to me - though I don't claim expertise in this area.
> >>>>>>>>
> >>>>>>>> A couple of minor nits:
> >>>>>>>>
> >>>>>>>> src/share/vm/runtime/thread.hpp
> >>>>>>>>
> >>>>>>>> 1364 static size_t stack_yellow_zone_size() {
> >>>>>>>> 1365 assert(_stack_red_zone_size > 0, "Don't call this before
> the
> >>>>>>>> field is initialized.");
> >>>>>>>>
> >>>>>>>> The assert should check yellow size
> >>>>>>>>
> >>>>>>>> 1383 static size_t stack_shadow_zone_size() {
> >>>>>>>> 1384 assert(_stack_red_zone_size > 0, "Don't call this before
> the
> >>>>>>>> field is initialized.");
> >>>>>>>>
> >>>>>>>> The assert should check shadow size.
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> src/share/vm/runtime/globals.hpp
> >>>>>>>>
> >>>>>>>> kB -> KB
> >>>>>>>>
> >>>>>>>>> I also fixed the bounds of ThreadStackSize and
> >>>>>>>>> CompilerThreadStackSize
> >>>>>>>> which
> >>>>>>>>> allowed to specify stacks >> max_intx.
> >>>>>>>>
> >>>>>>>> Ok - not sure how this was missed given we do the same for
> >>>>>>>> VMThreadStackSize :)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> If we agree on this, I need to replace all occurrences of the
> >>>>>>>>> three flags by
> >>>>>>>> accessor calls.
> >>>>>>>>> This allows to simplify all the platform code which computed the
> >>>>>>>>> space
> >>>>>>>> required
> >>>>>>>>> over and over again:
> >>>>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
> >>>>>>>> StackZones/webrev.00-spread/
> >>>>>>>>
> >>>>>>>> That all seems okay to me.
> >>>>>>>>
> >>>>>>>>> Actually I think there is no need that the shadow zone is page
> >>>>>>>>> aligned.
> >>>>>>>> There are no
> >>>>>>>>> pages to allocate or protect. The size of this zone is only used for
> >>>>>>> banging.
> >>>>>>>> In most
> >>>>>>>>> places only the upper bound of the zone is banged. In few
> places,
> >>>>>>>>> all
> >>>>>>> pages
> >>>>>>>> within
> >>>>>>>>> this zone are banged. But this code also does not depend on a
> >> page
> >>>>>>>> aligned size.
> >>>>>>>>> Removing the rounding to page sizes here could free up stack
> >> space
> >>>>>>>>> for
> >>>>>>> real
> >>>>>>>> usage.
> >>>>>>>>> But I think that should be done in an extra change.
> >>>>>>>>>
> >>>>>>>>> I also fixed the bounds of ThreadStackSize and
> >>>>>>>>> CompilerThreadStackSize
> >>>>>>>> which
> >>>>>>>>> allowed to specify stacks >> max_intx.
> >>>>>>>>>
> >>>>>>>>> If this is agreed on, I will check for jtreg tests that fail on
> >>>>>>>>> systems with
> >>>>>>> stack
> >>>>>>>>> pages > 4K and add the fixes to this change.
> >>>>>>>>
> >>>>>>>> You may also need to check the tests for valid command-line args
> >> given
> >>>>>>>> the range adjustments on the stack sizes.
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> David
> >>>>>>>> -----
> >>>>>>>>
> >>>>>>>>> Best regards,
> >>>>>>>>> Goetz.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>
> >>
> >> --
> >> 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 hotspot-runtime-dev
mailing list