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
--- a/hotspot/src/os/bsd/vm/os_bsd.cpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/os/bsd/vm/os_bsd.cpp Tue Jan 22 05:56:42 2013 -0800
@@ -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 @@
}
-- _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 @@
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 @@
}
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 @@
_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 @@
_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() {
--- a/hotspot/src/os/linux/vm/os_linux.cpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/os/linux/vm/os_linux.cpp Tue Jan 22 05:56:42 2013 -0800
@@ -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 @@
}
-- _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 @@
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 ;
- }
- 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");
- }
+ // 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);
+ }
+ 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 @@
}
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 @@
_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 @@
_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() {
--- a/hotspot/src/os/solaris/vm/os_solaris.cpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/os/solaris/vm/os_solaris.cpp Tue Jan 22 05:56:42 2013 -0800
@@ -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 @@
_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 @@
_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;
-
- // 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 ;
- }
+ // 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;
// 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 @@
}
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 @@
_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 @@
_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() {
--- a/hotspot/src/os/windows/vm/os_windows.cpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/os/windows/vm/os_windows.cpp Tue Jan 22 05:56:42 2013 -0800
@@ -4565,6 +4565,7 @@
}
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::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);
}
--- a/hotspot/src/share/vm/classfile/javaClasses.cpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/share/vm/classfile/javaClasses.cpp Tue Jan 22 05:56:42 2013 -0800
@@ -911,7 +911,6 @@
// 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);
--- a/hotspot/src/share/vm/runtime/objectMonitor.cpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/share/vm/runtime/objectMonitor.cpp Tue Jan 22 05:56:42 2013 -0800
@@ -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 @@
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 @@
//
// 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().
--- a/hotspot/src/share/vm/runtime/objectMonitor.inline.hpp Tue Jan 22 05:55:04 2013 -0800
+++ b/hotspot/src/share/vm/runtime/objectMonitor.inline.hpp Tue Jan 22 05:56:42 2013 -0800
@@ -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 @@
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;
}