RFR(M): 8139864: Improve handling of stack protection zones.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Dec 10 12:24:18 UTC 2015
Hi Coleen,
I don't see any progress on Frederics change.
It looks like it needs ppc adaptions, which I would have liked to
add, but it even does not apply to current hs-rt.
So shouldn't we proceed with this change? I don't think resolving
the conflicts is that hard in this case, as most edits are very straight
forward.
I made a new webrev including Thomas comments, and rebased it
to most recent hs-rt. Rebasing properly moved my edits from
templateInterpreter_x86_64.cpp to templateInterpreterGenerator_x86.cpp,
I think because you renamed the file with hg commands!
http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.02/
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