[15] RFR(S): 8246676: monitor list lock operations need more fencing and 8247280

[David Holmes]

Hi Dan,

On 15/07/2020 12:30 pm, Daniel D. Daugherty wrote:
> Greetings,
> 
> These fixes are targeted for JDK15 and I would like to push both of these
> fixes before the RDP2 cutoff on Thursday.
> 
> I have a JDK15 fix ready for a couple of related ObjectMonitor bug fixes:
> 
>      JDK-8246676 monitor list lock operations need more fencing
>      https://bugs.openjdk.java.net/browse/JDK-8246676
> 
>      JDK-8247280 more fencing needed in async deflation for non-TSO 
> machines
>      https://bugs.openjdk.java.net/browse/JDK-8247280
> 
> The fix for JDK-8246676 has been through three rounds of preliminary
> code review with David H.; Erik O. and Robbin participated in the first
> preliminary code review round. Mostly comment changes or backouts of
> code changes to return to the baseline code were done in the second and
> third preliminary rounds.
> 
> The fix for JDK-8247280 has been through two rounds of preliminary
> code review with David H. The bug fix itself was suggested by Erik O. so
> he's likely on-board with my implementation of the fix. :-)
> 
> Many thanks to David H., Erik O. and Robbin for their many emails on
> these topics and for reviewing the preliminary webrevs.
> 
> Here are the two webrevs:
> 
> http://cr.openjdk.java.net/~dcubed/8247280-webrev/0-for-jdk15/

My only follow up here is the proper fix for using Atomics with enums. 
The patch is below and I've tested it with tiers 1-3 (link sent separately).

> http://cr.openjdk.java.net/~dcubed/8246676-webrev/0-for-jdk15/

Also looks good.

> The project is currently baselined on jdk-15+30 and has gone through
> Mach5 Tier[1-3],4,5,6,7,8 testing with no regressions. I've also run
> my inflation stress kit on Linux-X64 and macOSX without any regressions.
> 
> Thanks, in advance, for any comments, questions or suggestions.
> 
> Dan

Thanks for working through all the details on this.

David
-----

diff -r 3bef86e53c51 src/hotspot/share/runtime/objectMonitor.hpp
--- a/src/hotspot/share/runtime/objectMonitor.hpp
+++ b/src/hotspot/share/runtime/objectMonitor.hpp
@@ -27,6 +27,7 @@

  #include "memory/allocation.hpp"
  #include "memory/padded.hpp"
+#include "metaprogramming/isRegisteredEnum.hpp"
  #include "oops/markWord.hpp"
  #include "runtime/os.hpp"
  #include "runtime/park.hpp"
@@ -372,4 +373,7 @@
    void      install_displaced_markword_in_object(const oop obj);
  };

+// Register for atomic operations.
+template<> struct IsRegisteredEnum<ObjectMonitor::AllocationState> : 
public TrueType {};
+
  #endif // SHARE_RUNTIME_OBJECTMONITOR_HPP
diff -r 3bef86e53c51 src/hotspot/share/runtime/objectMonitor.inline.hpp
--- a/src/hotspot/share/runtime/objectMonitor.inline.hpp
+++ b/src/hotspot/share/runtime/objectMonitor.inline.hpp
@@ -196,7 +196,7 @@
  }

  inline void 
ObjectMonitor::release_set_allocation_state(ObjectMonitor::AllocationState 
s) {
-  Atomic::release_store((int*)&_allocation_state, (int)s);
+  Atomic::release_store(&_allocation_state, s);
  }

  inline void 
ObjectMonitor::set_allocation_state(ObjectMonitor::AllocationState s) {
@@ -208,7 +208,7 @@
  }

  inline ObjectMonitor::AllocationState 
ObjectMonitor::allocation_state_acquire() const {
-  return (AllocationState)Atomic::load_acquire((int*)&_allocation_state);
+  return Atomic::load_acquire(&_allocation_state);
  }

  inline bool ObjectMonitor::is_free() const {