RFR(M): 8139864: Improve handling of stack protection zones.
Frederic Parain
frederic.parain at oracle.com
Fri Dec 18 09:39:07 UTC 2015
Still looks good to me.
Thank you,
Fred
On 12/17/2015 09:59 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> Our test passed successful after I did two minor
> changes: remove the assertion that _stack_reserved_zone_size > 0
> and fix a windows build issue. I also did the change proposed
> by Frederic.
> New webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8139864-StackZones/webrev.04/
>
> Best regards,
> Goetz.
>
>
>
>> -----Original Message-----
>> From: Frederic Parain [mailto:frederic.parain at oracle.com]
>> Sent: Wednesday, December 16, 2015 10:42 AM
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Coleen Phillimore'
>> <coleen.phillimore at oracle.com>; David Holmes
>> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(M): 8139864: Improve handling of stack protection zones.
>>
>> Hi Goetz,
>>
>> Thank you for fixing this and thank you for having delayed
>> your work to let me push JDK-8046936.
>>
>> On 12/15/2015 11:17 PM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> I reworked my change fixing handling of stack zone sizes with
>>> pages > 4K to fit on top of "8046936 : JEP 270: Reserved Stack Areas for
>> Critical Sections"
>>> The functional core is still
>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
>> StackZones/webrev.00-basic/,
>>> now extended for StackReservedPages.
>>> All the other changes are replacing local computations by shared utility
>> functions.
>>>
>>> I slightly changed handling StackReservedPages in case of large pages:
>>> I now align to page size. Before, it was set to '1'.
>>>
>>> In addition, I did a few cleanups.
>>>
>>> 8046936 changed yellow zone accessors to handle both, yellow and
>> reseved zones.
>>> In some places, I need the pure functions for the yellow zone, so I had
>>> name clashes. Therefore I renamed all functions handling reserved and
>> yellow
>>> zone together to be named 'yellow_reserved'.
>>>
>>> I introduced stack_end(), as I found stack_base() - stack_size() in lots
>>> of places.
>>> I am using on_local_stack(addr) in many places where an address was
>> checked
>>> to be on the stack (actually, I think in_stack(addr) would be a cleaner
>>> name).
>>> Also, I introduced usage of existing stack_overflow_limit() where that
>> address
>>> was computed.
>>>
>>> I think StackReservedPages is missing in the following places:
>>> frame_aarch64.cpp
>>> frame_x86.cpp
>>> templateInterpreterGenerator_aarch64.cpp:476
>>> templateInterpreterGenerator_sparc.cpp
>>> stack_zero.inline.hpp (fixed)
>>> os_solaris.cpp:4462
>>> os_windows.cpp:4196
>>
>> The reserved area is not implemented on aarch64 and cannot be
>> currently implemented on Windows because of JDK-8067946.
>> In frame_x86.cpp the code is correct, when access to the reserved
>> zone has been granted, it is legal to have a stack pointer in this
>> zone.
>> The two issues on SPARC are real, I'm fixing them.
>>
>>> I did not adapt these places, this should go into an extra change
>>> if it is not left out on purpose.
>>>
>>> I left out changes to cppInterpreter_<cpu>.cpp files.
>>>
>>> Please review this change. I please need a sponsor.
>>> Tests are planned tomorrow night (I missed our today's deadline).
>>> http://cr.openjdk.java.net/~goetz/webrevs/8139864-
>> StackZones/webrev.03/
>>
>> thread.hpp:
>>
>> lines 1429-1436: commented code
>>
>> Otherwise, looks good to me.
>>
>> Regards,
>>
>> Fred
>>
>>>> -----Original Message-----
>>>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>>>> Sent: Friday, December 04, 2015 8:59 PM
>>>> To: David Holmes <david.holmes at oracle.com>; Lindenmaier, Goetz
>>>> <goetz.lindenmaier at sap.com>; hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: RFR(M): 8139864: Improve handling of stack protection zones.
>>>>
>>>>
>>>> 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>
>>
>> --
>> Frederic Parain - Oracle
>> Grenoble Engineering Center - France
>> Phone: +33 4 76 18 81 17
>> Email: Frederic.Parain at oracle.com
--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com
More information about the hotspot-runtime-dev
mailing list