8264640: CMS ParScanClosure misses a barrier #165
Hi, please review an original fix for a GC crash. The jdk13u is the latest supported version that still has buggy code, it was deleted in jdk14 as a part of JEP 363: Remove the Concurrent Mark Sweep (CMS) Garbage Collector. So I'm proposing it here.
The fix is low-risk, on x86-64 it just introduces a compiler barrier to prevent two reads to be reordered as intended by surrounding comments. On CPUs with weaker memory models it introduces CPU barriers as well.
This looks good to me. I think I?m still a reviewer for the jdk-updates project.
For the benefit of everyone else...
We were seeing this as a crash when obtaining the size of an object to be copied. The klass was observed to be transiently NULL. We found that the object, reached through another reference path, had already been copied and the from-space oop placed on the task queue for subsequent reference field scanning. The task queue, however, had overflowed and the from-space oop was placed on the shared overflow queue where objects are chained together through their klass field. If the reads are ordered as they are in the code then everything is OK as per the comment at line 105 (in ParScanClosure::do_oop_work) but we found that gcc had reordered the reads in the non-compressed oops case. So the mark word is read and the object is observed to not forwarded (yet). Then, via another reference path, the object is copied, forwarded, and placed on the overflow task queue ? over writing the from-space object?s klass. Then in the original path the klass is read and observed to be NULL or the next overflow entry ? leading to the crash. When the from-space oop is dequeued, its klass is restored ? which is what was observed in the core file.
Using worker thread local queues, -XX:+ParGCUseLocalOverflow, seems to workaround the problem.
John, thank you for review and the comment!
On 4/8/21 9:33 PM, John Cuthbertson wrote:
@AntonKozlov This change now passes all automated pre-integration checks.
After integration, the commit message for the final commit will be:
At the time when this comment was updated there had been 12 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@yan-too) but any other Committer may sponsor as well.
Your commit was automatically rebased without conflicts.
Pushed as commit efc81a3.