From 01c35675d2a9b11d5c9c27533525bdc07ebd5743 Mon Sep 17 00:00:00 2001 From: dcubed Date: Tue, 22 Jan 2013 05:56:42 -0800 Subject: [PATCH] 8004902: correctness fixes motivated by contended locking work (6607129) Summary: misc correctness fixes Reviewed-by: acorn, dholmes, dice, sspitsyn Contributed-by: dave.dice@oracle.com --- src/os/bsd/vm/os_bsd.cpp | 88 ++++++++++-------- src/os/linux/vm/os_linux.cpp | 88 ++++++++++-------- src/os/solaris/vm/os_solaris.cpp | 89 +++++++++---------- src/os/windows/vm/os_windows.cpp | 39 ++++---- src/share/vm/classfile/javaClasses.cpp | 1 - src/share/vm/runtime/objectMonitor.cpp | 9 +- src/share/vm/runtime/objectMonitor.inline.hpp | 6 +- 7 files changed, 168 insertions(+), 152 deletions(-) diff --git a/src/os/bsd/vm/os_bsd.cpp b/src/os/bsd/vm/os_bsd.cpp index f04827862..477b4923e 100644 --- a/src/os/bsd/vm/os_bsd.cpp +++ b/src/os/bsd/vm/os_bsd.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2012, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2013, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -4094,11 +4094,12 @@ void os::PlatformEvent::park() { // AKA "down()" } -- _nParked ; - // In theory we could move the ST of 0 into _Event past the unlock(), - // but then we'd need a MEMBAR after the ST. _Event = 0 ; status = pthread_mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock"); + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other. + OrderAccess::fence(); } guarantee (_Event >= 0, "invariant") ; } @@ -4161,40 +4162,44 @@ int os::PlatformEvent::park(jlong millis) { status = pthread_mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock"); assert (_nParked == 0, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other. + OrderAccess::fence(); return ret; } void os::PlatformEvent::unpark() { - int v, AnyWaiters ; - for (;;) { - v = _Event ; - if (v > 0) { - // The LD of _Event could have reordered or be satisfied - // by a read-aside from this processor's write buffer. - // To avoid problems execute a barrier and then - // ratify the value. - OrderAccess::fence() ; - if (_Event == v) return ; - continue ; - } - if (Atomic::cmpxchg (v+1, &_Event, v) == v) break ; + // Transitions for _Event: + // 0 :=> 1 + // 1 :=> 1 + // -1 :=> either 0 or 1; must signal target thread + // That is, we can safely transition _Event from -1 to either + // 0 or 1. Forcing 1 is slightly more efficient for back-to-back + // unpark() calls. + // See also: "Semaphores in Plan 9" by Mullender & Cox + // + // Note: Forcing a transition from "-1" to "1" on an unpark() means + // that it will take two back-to-back park() calls for the owning + // thread to block. This has the benefit of forcing a spurious return + // from the first park() call after an unpark() call which will help + // shake out uses of park() and unpark() without condition variables. + + if (Atomic::xchg(1, &_Event) >= 0) return; + + // Wait for the thread associated with the event to vacate + int status = pthread_mutex_lock(_mutex); + assert_status(status == 0, status, "mutex_lock"); + int AnyWaiters = _nParked; + assert(AnyWaiters == 0 || AnyWaiters == 1, "invariant"); + if (AnyWaiters != 0 && WorkAroundNPTLTimedWaitHang) { + AnyWaiters = 0; + pthread_cond_signal(_cond); } - if (v < 0) { - // Wait for the thread associated with the event to vacate - int status = pthread_mutex_lock(_mutex); - assert_status(status == 0, status, "mutex_lock"); - AnyWaiters = _nParked ; - assert (AnyWaiters == 0 || AnyWaiters == 1, "invariant") ; - if (AnyWaiters != 0 && WorkAroundNPTLTimedWaitHang) { - AnyWaiters = 0 ; - pthread_cond_signal (_cond); - } - status = pthread_mutex_unlock(_mutex); - assert_status(status == 0, status, "mutex_unlock"); - if (AnyWaiters != 0) { - status = pthread_cond_signal(_cond); - assert_status(status == 0, status, "cond_signal"); - } + status = pthread_mutex_unlock(_mutex); + assert_status(status == 0, status, "mutex_unlock"); + if (AnyWaiters != 0) { + status = pthread_cond_signal(_cond); + assert_status(status == 0, status, "cond_signal"); } // Note that we signal() _after dropping the lock for "immortal" Events. @@ -4280,13 +4285,14 @@ static void unpackTime(struct timespec* absTime, bool isAbsolute, jlong time) { } void Parker::park(bool isAbsolute, jlong time) { + // Ideally we'd do something useful while spinning, such + // as calling unpackTime(). + // Optional fast-path check: // Return immediately if a permit is available. - if (_counter > 0) { - _counter = 0 ; - OrderAccess::fence(); - return ; - } + // We depend on Atomic::xchg() having full barrier semantics + // since we are doing a lock-free update to _counter. + if (Atomic::xchg(0, &_counter) > 0) return; Thread* thread = Thread::current(); assert(thread->is_Java_thread(), "Must be JavaThread"); @@ -4327,6 +4333,8 @@ void Parker::park(bool isAbsolute, jlong time) { _counter = 0; status = pthread_mutex_unlock(_mutex); assert (status == 0, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other and Java-level accesses. OrderAccess::fence(); return; } @@ -4363,12 +4371,14 @@ void Parker::park(bool isAbsolute, jlong time) { _counter = 0 ; status = pthread_mutex_unlock(_mutex) ; assert_status(status == 0, status, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other and Java-level accesses. + OrderAccess::fence(); + // If externally suspended while waiting, re-suspend if (jt->handle_special_suspend_equivalent_condition()) { jt->java_suspend_self(); } - - OrderAccess::fence(); } void Parker::unpark() { diff --git a/src/os/linux/vm/os_linux.cpp b/src/os/linux/vm/os_linux.cpp index 8377294d2..8ff3c786b 100644 --- a/src/os/linux/vm/os_linux.cpp +++ b/src/os/linux/vm/os_linux.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2012, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2013, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -5001,11 +5001,12 @@ void os::PlatformEvent::park() { // AKA "down()" } -- _nParked ; - // In theory we could move the ST of 0 into _Event past the unlock(), - // but then we'd need a MEMBAR after the ST. _Event = 0 ; status = pthread_mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock"); + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other. + OrderAccess::fence(); } guarantee (_Event >= 0, "invariant") ; } @@ -5068,40 +5069,44 @@ int os::PlatformEvent::park(jlong millis) { status = pthread_mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock"); assert (_nParked == 0, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other. + OrderAccess::fence(); return ret; } void os::PlatformEvent::unpark() { - int v, AnyWaiters ; - for (;;) { - v = _Event ; - if (v > 0) { - // The LD of _Event could have reordered or be satisfied - // by a read-aside from this processor's write buffer. - // To avoid problems execute a barrier and then - // ratify the value. - OrderAccess::fence() ; - if (_Event == v) return ; - continue ; - } - if (Atomic::cmpxchg (v+1, &_Event, v) == v) break ; + // Transitions for _Event: + // 0 :=> 1 + // 1 :=> 1 + // -1 :=> either 0 or 1; must signal target thread + // That is, we can safely transition _Event from -1 to either + // 0 or 1. Forcing 1 is slightly more efficient for back-to-back + // unpark() calls. + // See also: "Semaphores in Plan 9" by Mullender & Cox + // + // Note: Forcing a transition from "-1" to "1" on an unpark() means + // that it will take two back-to-back park() calls for the owning + // thread to block. This has the benefit of forcing a spurious return + // from the first park() call after an unpark() call which will help + // shake out uses of park() and unpark() without condition variables. + + if (Atomic::xchg(1, &_Event) >= 0) return; + + // Wait for the thread associated with the event to vacate + int status = pthread_mutex_lock(_mutex); + assert_status(status == 0, status, "mutex_lock"); + int AnyWaiters = _nParked; + assert(AnyWaiters == 0 || AnyWaiters == 1, "invariant"); + if (AnyWaiters != 0 && WorkAroundNPTLTimedWaitHang) { + AnyWaiters = 0; + pthread_cond_signal(_cond); } - if (v < 0) { - // Wait for the thread associated with the event to vacate - int status = pthread_mutex_lock(_mutex); - assert_status(status == 0, status, "mutex_lock"); - AnyWaiters = _nParked ; - assert (AnyWaiters == 0 || AnyWaiters == 1, "invariant") ; - if (AnyWaiters != 0 && WorkAroundNPTLTimedWaitHang) { - AnyWaiters = 0 ; - pthread_cond_signal (_cond); - } - status = pthread_mutex_unlock(_mutex); - assert_status(status == 0, status, "mutex_unlock"); - if (AnyWaiters != 0) { - status = pthread_cond_signal(_cond); - assert_status(status == 0, status, "cond_signal"); - } + status = pthread_mutex_unlock(_mutex); + assert_status(status == 0, status, "mutex_unlock"); + if (AnyWaiters != 0) { + status = pthread_cond_signal(_cond); + assert_status(status == 0, status, "cond_signal"); } // Note that we signal() _after dropping the lock for "immortal" Events. @@ -5187,13 +5192,14 @@ static void unpackTime(timespec* absTime, bool isAbsolute, jlong time) { } void Parker::park(bool isAbsolute, jlong time) { + // Ideally we'd do something useful while spinning, such + // as calling unpackTime(). + // Optional fast-path check: // Return immediately if a permit is available. - if (_counter > 0) { - _counter = 0 ; - OrderAccess::fence(); - return ; - } + // We depend on Atomic::xchg() having full barrier semantics + // since we are doing a lock-free update to _counter. + if (Atomic::xchg(0, &_counter) > 0) return; Thread* thread = Thread::current(); assert(thread->is_Java_thread(), "Must be JavaThread"); @@ -5234,6 +5240,8 @@ void Parker::park(bool isAbsolute, jlong time) { _counter = 0; status = pthread_mutex_unlock(_mutex); assert (status == 0, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other and Java-level accesses. OrderAccess::fence(); return; } @@ -5270,12 +5278,14 @@ void Parker::park(bool isAbsolute, jlong time) { _counter = 0 ; status = pthread_mutex_unlock(_mutex) ; assert_status(status == 0, status, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other and Java-level accesses. + OrderAccess::fence(); + // If externally suspended while waiting, re-suspend if (jt->handle_special_suspend_equivalent_condition()) { jt->java_suspend_self(); } - - OrderAccess::fence(); } void Parker::unpark() { diff --git a/src/os/solaris/vm/os_solaris.cpp b/src/os/solaris/vm/os_solaris.cpp index 41143af2d..389ed9aa2 100644 --- a/src/os/solaris/vm/os_solaris.cpp +++ b/src/os/solaris/vm/os_solaris.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2013, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -6014,6 +6014,9 @@ void os::PlatformEvent::park() { // AKA: down() _Event = 0 ; status = os::Solaris::mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock"); + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other. + OrderAccess::fence(); } } @@ -6055,51 +6058,43 @@ int os::PlatformEvent::park(jlong millis) { _Event = 0 ; status = os::Solaris::mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock"); + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other. + OrderAccess::fence(); return ret; } void os::PlatformEvent::unpark() { - int v, AnyWaiters; + // Transitions for _Event: + // 0 :=> 1 + // 1 :=> 1 + // -1 :=> either 0 or 1; must signal target thread + // That is, we can safely transition _Event from -1 to either + // 0 or 1. Forcing 1 is slightly more efficient for back-to-back + // unpark() calls. + // See also: "Semaphores in Plan 9" by Mullender & Cox + // + // Note: Forcing a transition from "-1" to "1" on an unpark() means + // that it will take two back-to-back park() calls for the owning + // thread to block. This has the benefit of forcing a spurious return + // from the first park() call after an unpark() call which will help + // shake out uses of park() and unpark() without condition variables. - // Increment _Event. - // Another acceptable implementation would be to simply swap 1 - // into _Event: - // if (Swap (&_Event, 1) < 0) { - // mutex_lock (_mutex) ; AnyWaiters = nParked; mutex_unlock (_mutex) ; - // if (AnyWaiters) cond_signal (_cond) ; - // } - - for (;;) { - v = _Event ; - if (v > 0) { - // The LD of _Event could have reordered or be satisfied - // by a read-aside from this processor's write buffer. - // To avoid problems execute a barrier and then - // ratify the value. A degenerate CAS() would also work. - // Viz., CAS (v+0, &_Event, v) == v). - OrderAccess::fence() ; - if (_Event == v) return ; - continue ; - } - if (Atomic::cmpxchg (v+1, &_Event, v) == v) break ; - } + if (Atomic::xchg(1, &_Event) >= 0) return; // If the thread associated with the event was parked, wake it. - if (v < 0) { - int status ; - // Wait for the thread assoc with the PlatformEvent to vacate. - status = os::Solaris::mutex_lock(_mutex); - assert_status(status == 0, status, "mutex_lock"); - AnyWaiters = _nParked ; - status = os::Solaris::mutex_unlock(_mutex); - assert_status(status == 0, status, "mutex_unlock"); - guarantee (AnyWaiters == 0 || AnyWaiters == 1, "invariant") ; - if (AnyWaiters != 0) { - // We intentional signal *after* dropping the lock - // to avoid a common class of futile wakeups. - status = os::Solaris::cond_signal(_cond); - assert_status(status == 0, status, "cond_signal"); - } + // Wait for the thread assoc with the PlatformEvent to vacate. + int status = os::Solaris::mutex_lock(_mutex); + assert_status(status == 0, status, "mutex_lock"); + int AnyWaiters = _nParked; + status = os::Solaris::mutex_unlock(_mutex); + assert_status(status == 0, status, "mutex_unlock"); + guarantee(AnyWaiters == 0 || AnyWaiters == 1, "invariant"); + if (AnyWaiters != 0) { + // We intentional signal *after* dropping the lock + // to avoid a common class of futile wakeups. + status = os::Solaris::cond_signal(_cond); + assert_status(status == 0, status, "cond_signal"); } } @@ -6177,14 +6172,14 @@ static void unpackTime(timespec* absTime, bool isAbsolute, jlong time) { } void Parker::park(bool isAbsolute, jlong time) { + // Ideally we'd do something useful while spinning, such + // as calling unpackTime(). // Optional fast-path check: // Return immediately if a permit is available. - if (_counter > 0) { - _counter = 0 ; - OrderAccess::fence(); - return ; - } + // We depend on Atomic::xchg() having full barrier semantics + // since we are doing a lock-free update to _counter. + if (Atomic::xchg(0, &_counter) > 0) return; // Optional fast-exit: Check interrupt before trying to wait Thread* thread = Thread::current(); @@ -6226,6 +6221,8 @@ void Parker::park(bool isAbsolute, jlong time) { _counter = 0; status = os::Solaris::mutex_unlock(_mutex); assert (status == 0, "invariant") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other and Java-level accesses. OrderAccess::fence(); return; } @@ -6267,12 +6264,14 @@ void Parker::park(bool isAbsolute, jlong time) { _counter = 0 ; status = os::Solaris::mutex_unlock(_mutex); assert_status(status == 0, status, "mutex_unlock") ; + // Paranoia to ensure our locked and lock-free paths interact + // correctly with each other and Java-level accesses. + OrderAccess::fence(); // If externally suspended while waiting, re-suspend if (jt->handle_special_suspend_equivalent_condition()) { jt->java_suspend_self(); } - OrderAccess::fence(); } void Parker::unpark() { diff --git a/src/os/windows/vm/os_windows.cpp b/src/os/windows/vm/os_windows.cpp index a10b83f89..74f9e6a9c 100644 --- a/src/os/windows/vm/os_windows.cpp +++ b/src/os/windows/vm/os_windows.cpp @@ -4565,6 +4565,7 @@ int os::PlatformEvent::park (jlong Millis) { } v = _Event ; _Event = 0 ; + // see comment at end of os::PlatformEvent::park() below: OrderAccess::fence() ; // If we encounter a nearly simultanous timeout expiry and unpark() // we return OS_OK indicating we awoke via unpark(). @@ -4602,25 +4603,25 @@ void os::PlatformEvent::park () { void os::PlatformEvent::unpark() { guarantee (_ParkHandle != NULL, "Invariant") ; - int v ; - for (;;) { - v = _Event ; // Increment _Event if it's < 1. - if (v > 0) { - // If it's already signaled just return. - // The LD of _Event could have reordered or be satisfied - // by a read-aside from this processor's write buffer. - // To avoid problems execute a barrier and then - // ratify the value. A degenerate CAS() would also work. - // Viz., CAS (v+0, &_Event, v) == v). - OrderAccess::fence() ; - if (_Event == v) return ; - continue ; - } - if (Atomic::cmpxchg (v+1, &_Event, v) == v) break ; - } - if (v < 0) { - ::SetEvent (_ParkHandle) ; - } + + // Transitions for _Event: + // 0 :=> 1 + // 1 :=> 1 + // -1 :=> either 0 or 1; must signal target thread + // That is, we can safely transition _Event from -1 to either + // 0 or 1. Forcing 1 is slightly more efficient for back-to-back + // unpark() calls. + // See also: "Semaphores in Plan 9" by Mullender & Cox + // + // Note: Forcing a transition from "-1" to "1" on an unpark() means + // that it will take two back-to-back park() calls for the owning + // thread to block. This has the benefit of forcing a spurious return + // from the first park() call after an unpark() call which will help + // shake out uses of park() and unpark() without condition variables. + + if (Atomic::xchg(1, &_Event) >= 0) return; + + ::SetEvent(_ParkHandle); } diff --git a/src/share/vm/classfile/javaClasses.cpp b/src/share/vm/classfile/javaClasses.cpp index cdca3d104..fe03d3fb1 100644 --- a/src/share/vm/classfile/javaClasses.cpp +++ b/src/share/vm/classfile/javaClasses.cpp @@ -911,7 +911,6 @@ jlong java_lang_Thread::stackSize(oop java_thread) { // Write the thread status value to threadStatus field in java.lang.Thread java class. void java_lang_Thread::set_thread_status(oop java_thread, java_lang_Thread::ThreadStatus status) { - assert(JavaThread::current()->thread_state() == _thread_in_vm, "Java Thread is not running in vm"); // The threadStatus is only present starting in 1.5 if (_thread_status_offset > 0) { java_thread->int_field_put(_thread_status_offset, status); diff --git a/src/share/vm/runtime/objectMonitor.cpp b/src/share/vm/runtime/objectMonitor.cpp index 2eefe2911..d6d2f1c31 100644 --- a/src/share/vm/runtime/objectMonitor.cpp +++ b/src/share/vm/runtime/objectMonitor.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2012, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2013, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -653,8 +653,7 @@ void ATTR ObjectMonitor::EnterI (TRAPS) { assert (_succ != Self, "invariant") ; if (_Responsible == Self) { _Responsible = NULL ; - // Dekker pivot-point. - // Consider OrderAccess::storeload() here + OrderAccess::fence(); // Dekker pivot-point // We may leave threads on cxq|EntryList without a designated // "Responsible" thread. This is benign. When this thread subsequently @@ -674,10 +673,6 @@ void ATTR ObjectMonitor::EnterI (TRAPS) { // // The MEMBAR, above, prevents the LD of cxq|EntryList in the subsequent // exit operation from floating above the ST Responsible=null. - // - // In *practice* however, EnterI() is always followed by some atomic - // operation such as the decrement of _count in ::enter(). Those atomics - // obviate the need for the explicit MEMBAR, above. } // We've acquired ownership with CAS(). diff --git a/src/share/vm/runtime/objectMonitor.inline.hpp b/src/share/vm/runtime/objectMonitor.inline.hpp index 2107523a3..ebe2b7dcb 100644 --- a/src/share/vm/runtime/objectMonitor.inline.hpp +++ b/src/share/vm/runtime/objectMonitor.inline.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2010, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2013, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -101,10 +101,12 @@ inline intptr_t ObjectMonitor::contentions() const { return _count; } +// Do NOT set _count = 0. There is a race such that _count could +// be set while inflating prior to setting _owner +// Just use Atomic::inc/dec and assert 0 when monitor put on free list inline void ObjectMonitor::set_owner(void* owner) { _owner = owner; _recursions = 0; - _count = 0; } -- GitLab