RFR(M): 8139864: Improve handling of stack protection zones.

Wed Dec 9 11:43:04 UTC 2015

Hi Thomas,

thanks for the review! Sorry I come up with my answer that late, 
but the change is blocked anyways.
See my comments inline below.  I'll make a new webrev once the 
issue with Frederics change is resolved.

Best regards,
  Goetz.

> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] 
> Sent: Samstag, 5. Dezember 2015 10:49
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: 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 Goetz,
>
> thanks for this change! I'm a bit late to review this, so feel free to work in my suggestions or not. They are not terribly important, mostly just cosmetics:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/share/vm/runtime/thread.hpp.udiff.html
>
> There is no reason to publish set_stack_xxxx_zone_size(). They could be file local in thread.cpp (if _stack_xxx_zone_size were made file local too) or at least made class private.
It’s called in os::init_before_ergo(), so it must be public.

> ---
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp.udiff.html
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/cpu/sparc/vm/macroAssembler_sparc.cpp.udiff.html
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/cpu/sparc/vm/sharedRuntime_sparc.cpp.udiff.html
> etc
>
> All places where you calculate the number of real shadow pages by (JavaThread::stack_shadow_zone_size() / os::vm_page_size()) could be made more readable with > another method like "JavaThread::num_stack_shadow_pages()", which would return the number of real stack shadow pages.
The reason in dividing by page size in in the algorithm implemented, not in the fact that the zone has a page size. So it should be explicit
in those places you mention.

> ----
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/cpu/aarch64/vm/templateInterpreter_aarch64.cpp.udiff.html
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/cpu/x86/vm/cppInterpreter_x86.cpp.udiff.html
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/cpu/x86/vm/templateInterpreter_x86_32.cpp.udiff.html
>
> etc.
>
> All places where max_bang_size is calculated could be made more readable by using MAX2.
Good point, fixed.

> ----
>
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.01/src/share/vm/runtime/globals.hpp.udiff.html
>missing period after "If pages are bigger yellow zone is aligned up" on StackYellowPages/StackRedPages.
>missing period after "This should exceed the depth of the VM and native call stack" for StackShadowPages.
I thought it's supposed to be without period.  I added them.

> ----
>
> Apart from that, I would second Coleen's request for a compile-time check against using the old StackXXXPages parameters directly.
I can't set the flag to -1 or the like because of the range checks, which are the cause for this change.
Any other change would require major changes to the flag handling.

> Kind Regards, Thomas

On Tue, Dec 1, 2015 at 5:10 PM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> 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 please need a second reviewer and a sponsor.

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.
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >