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

[David Holmes]

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