JDK-8166811 : Missing memory fences between memory allocation and refinement
  • Type: Bug
  • Component: hotspot
  • Sub-Component: gc
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2016-09-27
  • Updated: 2021-01-13
  • Resolved: 2016-11-23
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9 Other
9 b150Fixed openjdk8uUnresolved
Related Reports
Relates :  
JDK-8160369 - Memory fences needed around setting and reading object lengths
Description
HeapRegion::oops_on_card_seq_iterate_careful begins with a block of code that intersects the allocated part of the region (based on top or scan_top) with the card region, and if empty exits early.  It then tests the region for being young, exiting early if so.  The is_young test is noted as needing to follow the check for allocation in the region, and notes that a newly allocated young region has its type set before top is set.  However, there don't appear to be any memory barriers to enforce either ordering.
Comments
Plain old regions are not problematic as they are only allocated and their type and top pointer only during GC. For humongous allocations we have the following ordering of operations: [if freshly allocated: - set type to free - set bottom and top to the same value - storestore - publish the HeapRegion in the region table ] (in humongous_obj_allocate_initialize_regions()) - set types of HeapRegion*'s to humongous[_continues] - storestore - set new top The problem as suggested in the CR is that there is no ordering on the reader side.
08-11-2016
Just a suggestion, what about using scan_top for all regions, setting it every GC? If it is set to bottom() for all regions during initialization, and always return its actual value in HeapRegion::scan_top(). During GC we update it for the old gen regions we allocate into. At mutator time, only humongous object allocation sets it as needed. So this issue may not occur. And we can remove the region type checks. And the special casing for is_gc_active(). (This is a half-baked idea I got when reading this, not sure if it is actually desirable yet :)).
28-09-2016
The above suggests there should be a storestore/release before setting top in G1AllocRegion::allocate, and a loadload/acquire when reading top in oops_on_card_xxx. However, I wonder if there might be a way to restructure oops_on_card_xxx to avoid all this, by testing for is_old | is_humongous to proceed, rather than testing for is_young to not proceed.
27-09-2016
This change: 6956639: G1: assert(cached_ptr != card_ptr) failed: shouldn't be, concurrentG1Refine.cpp:307 changed the allocation code in g1CollectedHeap.cpp to have a storestore "to ensure that the store to top does not float above the setting of the young type." However, it looks like refactoring has changed things a lot, and while I can find what I think is the corresponding code (see previous comment for young), there's no sign of a storestore/release barrier to ensure ordering. Note that the other relevant concurrent allocation path, humongous_obj_allocate_initialize_regions, does have a storestore for this purpose. And for GC allocation, because oops_on_card_xxx uses scan_top rather than top as the limit during GC, there's no need for barriers on either side in the during-GC case. 6956639 also added the code in oops_on_card_xxx that has the ordering constraint, along with the comment describing that constraint, but did not add any loadload/acquire barriers there.
27-09-2016
G1AllocRegion::new_alloc_region_and_allocate calls (1) allocate_new_region, which for mutator allocations calls set_region_shoft_lived_locked, adding the region to eden and setting it young. (2) allocate, which calls set_top. There is a storestore following the allocate, to ensure the "alloc region cannot be empty" invariant, but that invariant doesn't help in the context of refinement.
27-09-2016