United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-6940310 G1: MT-unsafe calls to CM::region_stack_push() / CM::region_stack_pop()
JDK-6940310 : G1: MT-unsafe calls to CM::region_stack_push() / CM::region_stack_pop()

Details
Type:
Bug
Submit Date:
2010-04-01
Status:
Resolved
Updated Date:
2010-09-24
Project Name:
JDK
Resolved Date:
2010-04-21
Component:
hotspot
OS:
windows_2003,generic
Sub-Component:
gc
CPU:
x86,generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
hs16,hs18
Fixed Versions:
hs18 (b03)

Related Reports
Backport:
Backport:
Backport:
Backport:
Backport:
Backport:
Duplicate:
Relates:
Relates:

Sub Tasks

Description
We add / remove entries to / from the region stack in the ConcurrentMark class using the region_stack_push() / region_stack_pop() methods. The assumption for those methods is that region_stack_push() will only be called during evacuation pauses and the region_stack_pop() will only be called during concurrent marking. So, their implementation which is based on CAS is safe as long as we only call one of those methods concurrently by multiple threads but not both.

Unfortunately, there's a case where both can be called concurrently: during marking we mostly call region_stack_pop() to process entries from the region stack. But if we get a safepoint while we're processing one of the entries we will use region_stack_push() to push the remainder of the region we're processing so that we can carry on processing it in the future. And if we call region_stack_push() / region_stack_pop() concurrently, the results are undefined (i.e., the pop method can decrement the index and read the contents from the array before the push method has actaully installed them).

                                    

Comments
EVALUATION

See Description.
                                     
2010-04-01
SUGGESTED FIX

There are a couple of solutions to this problem. First: manipulate the region stack while holding a lock instead of using a CAS. Second: if a CM task wants to retain a region for later processing, it can save it on a local MemRegion field and make sure it processes it next time around, instead of pushing it on the region stack. The latter solution will be a bit more efficient. However, it will complicate a few things in the concurrent marking code given that there are a couple of places when we need to know whether the region stack is empty or not and having half-scanned regions saved for later processing will be the same as having a non-empty region stack.
                                     
2010-04-01
SUGGESTED FIX

For this fix we'll take a conservative approach. We'll carry on using the CAS-based region_stack_push() method during evacuation pauses. However, we'll introduce region_stack_push_with_lock() / region_stack_pop_with_lock() to be used during the marking phase (so that they can be safely called concurrently with each other). The alternative fix (each CMTask saves a region locally to be able to get back to it later) is a bit more fiddly.
                                     
2010-04-02
EVALUATION

http://hg.openjdk.java.net/jdk7/hotspot-gc/hotspot/rev/72f725c5a7be
                                     
2010-04-07
EVALUATION

http://hg.openjdk.java.net/hsx/hsx17/baseline/rev/e9ae45b9ec48
                                     
2010-04-08
EVALUATION

http://hg.openjdk.java.net/hsx/hsx17/master/rev/e9ae45b9ec48
                                     
2010-04-09



Hardware and Software, Engineered to Work Together