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