Request for review: 6978641 ( was Re: Request for approval: 6929067: Stack guard pages should be removed when thread is detached)

[Daniel D. Daugherty]

Andrew,

My e-mail archive shows the fix for this bug as pushed a long time ago.
Please see the attached changeset notification.

Dan



On 10/19/11 11:18 AM, Andrew Haley wrote:
> What is the status of this?  6978641 is marked as
>
> 11-Closed, Unverified, request for enhancement
>
> Andrew.
>
>
>
> On 08/23/2010 10:10 PM, John Coomes wrote:
>> David Holmes (David.Holmes at oracle.com) wrote:
>>> Here's the official OpenJKDK webrev for this:
>>>
>>> http://cr.openjdk.java.net/~dholmes/6978641/
>> Hi David,
>>
>> Looks good to me, aside from one syntax nit:  I find it hard to read
>> when control flow is embedded in macros like DEBUG_ONLY(),
>> PRODUCT_ONLY(), etc.  Using a separate variable is easier to read and
>> also can be useful in the debugger, e.g.,
>>
>>     bool chk_bounds = NOT_DEBUG(os::Linux::is_initial_thread()) DEBUG_ONLY(true);
>>     if (chk_bounds&&  get_stack_bounds(&stack_extent,&stack_base)) {
>>       assert(...);
>>
>> -John
>>
>>> If I can add you both as reviewers that would be great.
>>>
>>> Coleen can you send me details on which release/build I should use in
>>> the CR and also if there is any special "magic" I need to submit via JPRT.
>>>
>>> Thanks,
>>> David
>>>
>>> Coleen Phillimore said the following on 08/20/10 23:15:
>>>> This change looks fine to me and yes, thanks for pointing out that I did
>>>> add the test.
>>>>
>>>> Coleen
>>>>
>>>> Andrew Haley wrote:
>>>>> On 08/19/2010 04:49 AM, David Holmes wrote:
>>>>>
>>>>>
>>>>>> diff -r f707b4c0c36f src/os/linux/vm/os_linux.cpp
>>>>>> --- a/src/os/linux/vm/os_linux.cpp
>>>>>> +++ b/src/os/linux/vm/os_linux.cpp
>>>>>> @@ -2628,10 +2628,12 @@ get_stack_bounds(uintptr_t *bottom, uint
>>>>>>   // where we're going to put our guard pages, truncate the mapping at
>>>>>>   // that point by munmap()ping it.  This ensures that when we later
>>>>>>   // munmap() the guard pages we don't leave a hole in the stack
>>>>>> -// mapping.
>>>>>> +// mapping. This only affects the main/initial thread, but guard
>>>>>> +// against future OS changes
>>>>>>   bool os::create_stack_guard_pages(char* addr, size_t size) {
>>>>>>     uintptr_t stack_extent, stack_base;
>>>>>> -  if (get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>> +  if (NOT_DEBUG(os::Linux::is_initial_thread()&&)
>>>>>> get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>> +      assert(os::Linux::is_initial_thread(), "growable stack in
>>>>>> non-initial thread");
>>>>>>       if (stack_extent<  (uintptr_t)addr)
>>>>>>         ::munmap((void*)stack_extent, (uintptr_t)addr - stack_extent);
>>>>>>     }
>>>>>>
>>>>> Is stack_extent initialized here in NOT_DEBUG mode?
>>>>>
>>>>>
>>>>>> @@ -2640,10 +2642,13 @@ bool os::create_stack_guard_pages(char*
>>>>>>   }
>>>>>>
>>>>>>   // If this is a growable mapping, remove the guard pages entirely by
>>>>>> -// munmap()ping them.  If not, just call uncommit_memory().
>>>>>> +// munmap()ping them.  If not, just call uncommit_memory(). This only
>>>>>> +// affects the main/initial thread, but guard against future OS changes
>>>>>>   bool os::remove_stack_guard_pages(char* addr, size_t size) {
>>>>>>     uintptr_t stack_extent, stack_base;
>>>>>> -  if (get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>> +  if (NOT_DEBUG(os::Linux::is_initial_thread()&&)
>>>>>> get_stack_bounds(&stack_extent,&stack_base)) {
>>>>>> +      assert(os::Linux::is_initial_thread(), "growable stack in
>>>>>> non-initial thread");
>>>>>> +
>>>>>>       return ::munmap(addr, size) == 0;
>>>>>>     }
>>>>> This is basically a good compromise, I think.
>>>>>
>>>>> Andrew.
>>>>>
>
-------------- next part --------------
An embedded message was scrubbed...
From: david.holmes at oracle.com
Subject: hg: jdk7/hotspot-rt/hotspot: 6978641: Fix for 6929067 introduces
	additional overhead in thread creation/termination paths
Date: Fri, 27 Aug 2010 00:01:21 +0000
Size: 3299
Url: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20111019/67796564/AttachedMessage.nws