--- old/src/hotspot/share/runtime/objectMonitor.hpp 2020-07-14 21:38:23.000000000 -0400 +++ new/src/hotspot/share/runtime/objectMonitor.hpp 2020-07-14 21:38:23.000000000 -0400 @@ -277,9 +277,14 @@ // _owner field. Returns the prior value of the _owner field. void* try_set_owner_from(void* old_value, void* new_value); + // Simply get _next_om field. ObjectMonitor* next_om() const; + // Get _next_om field with acquire semantics. + ObjectMonitor* next_om_acquire() const; // Simply set _next_om field to new_value. void set_next_om(ObjectMonitor* new_value); + // Set _next_om field to new_value with release semantics. + void release_set_next_om(ObjectMonitor* new_value); // Try to set _next_om field to new_value if the current value matches // old_value, using Atomic::cmpxchg(). Otherwise, does not change the // _next_om field. Returns the prior value of the _next_om field. --- old/src/hotspot/share/runtime/objectMonitor.inline.hpp 2020-07-14 21:38:24.000000000 -0400 +++ new/src/hotspot/share/runtime/objectMonitor.inline.hpp 2020-07-14 21:38:24.000000000 -0400 @@ -223,15 +223,26 @@ // use Atomic operations to disable compiler optimizations that // might try to elide loading and/or storing this field. +// Simply get _next_om field. inline ObjectMonitor* ObjectMonitor::next_om() const { return Atomic::load(&_next_om); } +// Get _next_om field with acquire semantics. +inline ObjectMonitor* ObjectMonitor::next_om_acquire() const { + return Atomic::load_acquire(&_next_om); +} + // Simply set _next_om field to new_value. inline void ObjectMonitor::set_next_om(ObjectMonitor* new_value) { Atomic::store(&_next_om, new_value); } +// Set _next_om field to new_value with release semantics. +inline void ObjectMonitor::release_set_next_om(ObjectMonitor* new_value) { + Atomic::release_store(&_next_om, new_value); +} + // Try to set _next_om field to new_value if the current value matches // old_value. Otherwise, does not change the _next_om field. Returns // the prior value of the _next_om field. --- old/src/hotspot/share/runtime/synchronizer.cpp 2020-07-14 21:38:25.000000000 -0400 +++ new/src/hotspot/share/runtime/synchronizer.cpp 2020-07-14 21:38:25.000000000 -0400 @@ -174,7 +174,7 @@ // Return true if the ObjectMonitor is locked. // Otherwise returns false. static bool is_locked(ObjectMonitor* om) { - return ((intptr_t)om->next_om() & OM_LOCK_BIT) == OM_LOCK_BIT; + return ((intptr_t)om->next_om_acquire() & OM_LOCK_BIT) == OM_LOCK_BIT; } // Mark an ObjectMonitor* with OM_LOCK_BIT and return it. @@ -215,18 +215,23 @@ " must have OM_LOCK_BIT=%x set.", p2i(next), OM_LOCK_BIT); next = (ObjectMonitor*)((intptr_t)next & ~OM_LOCK_BIT); // Clear OM_LOCK_BIT. - om->set_next_om(next); + om->release_set_next_om(next); } // Get the list head after locking it. Returns the list head or NULL // if the list is empty. static ObjectMonitor* get_list_head_locked(ObjectMonitor** list_p) { while (true) { + // Acquire semantics not needed on this list load since we're + // checking for NULL here or following up with a cmpxchg() via + // try_om_lock() below and we retry on cmpxchg() failure. ObjectMonitor* mid = Atomic::load(list_p); if (mid == NULL) { return NULL; // The list is empty. } if (try_om_lock(mid)) { + // Acquire semantics not needed on this list load since memory is + // already consistent due to the cmpxchg() via try_om_lock() above. if (Atomic::load(list_p) != mid) { // The list head changed before we could lock it so we have to retry. om_unlock(mid); @@ -249,12 +254,17 @@ int count, ObjectMonitor** list_p, int* count_p) { while (true) { + // Acquire semantics not needed on this list load since we're + // following up with a cmpxchg() via try_om_lock() below and we + // retry on cmpxchg() failure. ObjectMonitor* cur = Atomic::load(list_p); // Prepend list to *list_p. if (!try_om_lock(tail)) { // Failed to lock tail due to a list walker so try it all again. continue; } + // Release semantics not needed on this "unlock" since memory is + // already consistent due to the cmpxchg() via try_om_lock() above. tail->set_next_om(cur); // tail now points to cur (and unlocks tail) if (cur == NULL) { // No potential race with takers or other prependers since @@ -342,14 +352,19 @@ // Lock the list head to guard against races with a list walker // or async deflater thread (which only races in om_in_use_list): if ((cur = get_list_head_locked(list_p)) != NULL) { - // List head is now locked so we can safely switch it. + // List head is now locked so we can safely switch it. Release + // semantics not needed on this "unlock" since memory is already + // consistent due to the cmpxchg() via get_list_head_locked() above. m->set_next_om(cur); // m now points to cur (and unlocks m) + OrderAccess::storestore(); // Make sure set_next_om() is seen first. Atomic::store(list_p, m); // Switch list head to unlocked m. om_unlock(cur); break; } // The list is empty so try to set the list head. assert(cur == NULL, "cur must be NULL: cur=" INTPTR_FORMAT, p2i(cur)); + // Release semantics not needed on this "unlock" since memory + // is already consistent. m->set_next_om(cur); // m now points to NULL (and unlocks m) if (Atomic::cmpxchg(list_p, cur, m) == cur) { // List head is now unlocked m. @@ -384,7 +399,9 @@ } ObjectMonitor* next = unmarked_next(take); // Switch locked list head to next (which unlocks the list head, but - // leaves take locked): + // leaves take locked). Release semantics not needed on this "unlock" + // since memory is already consistent due to the cmpxchg() via + // get_list_head_locked() above. Atomic::store(list_p, next); Atomic::dec(count_p); // Unlock take, but leave the next value for any lagging list @@ -1343,6 +1360,7 @@ void ObjectSynchronizer::global_used_oops_do(OopClosure* f) { assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint"); + // Acquire semantics not needed since we're at a safepoint. list_oops_do(Atomic::load(&om_list_globals._in_use_list), f); } @@ -1408,6 +1426,9 @@ // 2: try to allocate from the global om_list_globals._free_list // If we're using thread-local free lists then try // to reprovision the caller's free list. + // Acquire semantics not needed on this list load since memory + // is already consistent due to the cmpxchg() via + // take_from_start_of_om_free_list() above. if (Atomic::load(&om_list_globals._free_list) != NULL) { // Reprovision the thread's om_free_list. // Use bulk transfers to reduce the allocation rate and heat @@ -1536,7 +1557,9 @@ // First special case: // 'm' matches mid, is the list head and is locked. Switch the list // head to next which unlocks the list head, but leaves the extracted - // mid locked: + // mid locked. Release semantics not needed on this "unlock" since + // memory is already consistent due to the get_list_head_locked() + // above. Atomic::store(&self->om_in_use_list, next); } else if (m == next) { // Second special case: @@ -1550,7 +1573,9 @@ // Update next to what follows mid (if anything): next = unmarked_next(mid); // Switch next after the list head to new next which unlocks the - // list head, but leaves the extracted mid locked: + // list head, but leaves the extracted mid locked. Release semantics + // not needed on this "unlock" since memory is already consistent + // due to the get_list_head_locked() above. self->om_in_use_list->set_next_om(next); } else { // We have to search the list to find 'm'. @@ -1570,7 +1595,10 @@ // Update next to what follows mid (if anything): next = unmarked_next(mid); // Switch next after the anchor to new next which unlocks the - // anchor, but leaves the extracted mid locked: + // anchor, but leaves the extracted mid locked. Release semantics + // not needed on this "unlock" since memory is already consistent + // due to the om_unlock() above before entering the loop or the + // om_unlock() below before looping again. anchor->set_next_om(next); break; } else { @@ -1676,6 +1704,7 @@ ADIM_guarantee(l_om_in_use_count == in_use_count, "in-use counts don't match: " "l_om_in_use_count=%d, in_use_count=%d", l_om_in_use_count, in_use_count); Atomic::store(&self->om_in_use_count, 0); + OrderAccess::storestore(); // Make sure counter update is seen first. // Clear the in-use list head (which also unlocks it): Atomic::store(&self->om_in_use_list, (ObjectMonitor*)NULL); om_unlock(in_use_list); @@ -1719,6 +1748,7 @@ ADIM_guarantee(l_om_free_count == free_count, "free counts don't match: " "l_om_free_count=%d, free_count=%d", l_om_free_count, free_count); Atomic::store(&self->om_free_count, 0); + OrderAccess::storestore(); // Make sure counter update is seen first. Atomic::store(&self->om_free_list, (ObjectMonitor*)NULL); om_unlock(free_list); } @@ -1903,6 +1933,7 @@ // Must preserve store ordering. The monitor state must // be stable at the time of publishing the monitor address. guarantee(object->mark() == markWord::INFLATING(), "invariant"); + // Release semantics so that above set_object() is seen first. object->release_set_mark(markWord::encode(m)); // Once ObjectMonitor is configured and the object is associated @@ -2076,6 +2107,7 @@ } // Restore the header back to obj + // XXX - I have no rationale for this "release", but it's been here forever. obj->release_set_mark(dmw); if (AsyncDeflateIdleMonitors) { // clear() expects the owner field to be NULL. @@ -2278,7 +2310,8 @@ // At this point mid is disconnected from the in-use list. deflated_count++; Atomic::dec(count_p); - // mid is current tail in the free_head_p list so NULL terminate it: + // mid is current tail in the free_head_p list so NULL terminate it. + // No release semantics needed since Atomic::dec() already provides it. mid->set_next_om(NULL); } else { cur_mid_in_use = mid; @@ -2359,20 +2392,24 @@ if (cur_mid_in_use == NULL) { // mid is the list head and it is locked. Switch the list head // to next which is also locked (if not NULL) and also leave - // mid locked: - Atomic::store(list_p, next); + // mid locked. Release semantics needed since not all code paths + // in deflate_monitor_using_JT() ensure memory consistency. + Atomic::release_store(list_p, next); } else { ObjectMonitor* locked_next = mark_om_ptr(next); // mid and cur_mid_in_use are locked. Switch cur_mid_in_use's - // next field to locked_next and also leave mid locked: - cur_mid_in_use->set_next_om(locked_next); + // next field to locked_next and also leave mid locked. + // Release semantics needed since not all code paths in + // deflate_monitor_using_JT() ensure memory consistency. + cur_mid_in_use->release_set_next_om(locked_next); } // At this point mid is disconnected from the in-use list so // its lock longer has any effects on in-use list. deflated_count++; Atomic::dec(count_p); - // mid is current tail in the free_head_p list so NULL terminate it - // (which also unlocks it): + // mid is current tail in the free_head_p list so NULL terminate + // it (which also unlocks it). No release semantics needed since + // Atomic::dec() already provides it. mid->set_next_om(NULL); // All the list management is done so move on to the next one: @@ -2400,6 +2437,9 @@ next = next_next; if (SafepointMechanism::should_block(self) && + // Acquire semantics are not needed on this list load since + // it is not dependent on the following load which does have + // acquire semantics. cur_mid_in_use != Atomic::load(list_p) && cur_mid_in_use->is_old()) { // If a safepoint has started and cur_mid_in_use is not the list // head and is old, then it is safe to use as saved state. Return @@ -2462,6 +2502,7 @@ // For moribund threads, scan om_list_globals._in_use_list int deflated_count = 0; + // Acquire semantics not needed since we are at a safepoint. if (Atomic::load(&om_list_globals._in_use_list) != NULL) { // Update n_in_circulation before om_list_globals._in_use_count is // updated by deflation. @@ -2547,6 +2588,7 @@ ADIM_guarantee(list != NULL, "om_list_globals._wait_list must not be NULL"); int count = Atomic::load(&om_list_globals._wait_count); Atomic::store(&om_list_globals._wait_count, 0); + OrderAccess::storestore(); // Make sure counter update is seen first. Atomic::store(&om_list_globals._wait_list, (ObjectMonitor*)NULL); // Find the tail for prepend_list_to_common(). No need to mark