RFR(M): 8139864: Improve handling of stack protection zones.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Dec 17 20:59:15 UTC 2015
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
More information about the hotspot-runtime-dev
mailing list