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

Fri Dec 4 19:59:18 UTC 2015

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