[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    RE: RFR(M): 8139864: Improve handling of stack protection zones.
From:       "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date:       2015-11-27 9:28:17
Message-ID: 4295855A5C1DE049A61835A1887419CC41ED4CF2 () DEWDFEMB12A ! global ! corp ! sap
[Download RAW message or body]

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@oracle.com]
> Sent: Freitag, 27. November 2015 06:33
> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Coleen Phillimore
> (coleen.phillimore@oracle.com) <coleen.phillimore@oracle.com>; hotspot-
> runtime-dev@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.
> >
> >
> >
> >
> >
> >
> >
> >
> >
[prev in list] [next in list] [prev in thread] [next in thread]