Request for approval: 6929067: Stack guard pages should be removed when thread is detached

[David Holmes]

Here's the prototype including the safety-check Coleen wanted.

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);
    }
@@ -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;
    }

We skip get_stack_bounds for all but the main thread in product mode; 
then in debug mode we always call get_stack_bounds but trigger an assert 
of we seem to find a growable stack that does not belong to the main thread.

This fixes our performance issue, but otherwise has not been fully tested.

David
-----


David Holmes said the following on 08/19/10 09:41:
> Andrew Haley said the following on 08/17/10 18:14:
>> On 08/16/2010 11:57 PM, David Holmes wrote:
>>> Andrew Haley said the following on 08/16/10 18:42:
>>>> On 08/16/2010 03:46 AM, David Holmes wrote:
>>>>> Looking further into this, isn't the only thread that can be 
>>>>> affected by
>>>>> this the main thread? So we could perform this only if
>>>>> os::is_initial_thread() returns true?
>>>> I suppose we could, yes.  I wonder if it'd be a latent bug to assume
>>>> that Java threads could never use growable mappings, though.  Doing
>>>> this makes the system a bit less robust.
>>> As I understand it neither LinuxThreads nor NPTL have used growable
>>> mappings for a very long time. Even if there were a reason to go back to
>>> this it would have to be done in a compatible way and so there would be
>>> time to "enhance" the VM to accommodate it.
>>
>> Oh, I'm sure there would.  The problem is that this bug is very
>> subtle, and if it came back again it would be just as hard to
>> diagnose.  If the change to use growable mappings were made upstream
>> on some platform, would we notice?
> 
> I would hope (and I know hopes can be dashed) that the upstream 
> maintainers would not make such a change silently.
> 
> Was there a regression test created for this bug? That would catch any 
> change.
> 
> David
> -----
> 
>>>> Do you really think that the cost of get_stack_bounds() is 
>>>> significant in
>>>> the context of terminating a thread?
>>> It can be. This has introduced a new bottleneck on the thread
>>> termination path and we're seeing that affect on certain systems (the
>>> details of which I can't go in to).
>>
>> Aha, so this is not just a theoretical concern.  Fair enough.  It
>> seems like I may have falsely assumed the work in the kernel to
>> terminate a thread would be greater than this.
>>
>> Andrew.