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

[David Holmes]

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