Request for review: 8044406: JVM crash with JDK8 (build 1.8.0-b132) with G1 GC

[Mikael Gerdin]

Hi Poonam,

On 2014-08-18 13:08, Poonam Bajaj wrote:
> Hello Mikael,
>
> Thanks for your review. Here's the updated webrev:
> http://cr.openjdk.java.net/~poonam/8044406/webrev.01/

This looks good, Reviewed.

/Mikael

>
> regards,
> Poonam
>
>
> On 8/18/2014 2:02 PM, Mikael Gerdin wrote:
>> Hi Poonam,
>>
>> On 2014-08-17 02:54, Poonam Bajaj wrote:
>>> Hi,
>>>
>>> Could I have reviews for the fix for a G1 GC crash:
>>>
>>> 8044406 <https://bugs.openjdk.java.net/browse/JDK-8044406>: JVM crash
>>> with JDK8 (build 1.8.0-b132) with G1 GC
>>> Webrev: http://cr.openjdk.java.net/~poonam/8044406/webrev.00/webrev/
>>
>> 7020     // Determine how far we are from the next card boundary. If
>> it is smaller than
>> 7021     // the minimum object size we can allocate into, expand into
>> the next card.
>> 7022     size_t top = cur->used();
>> 7023     size_t aligned_top = align_size_up_(top,
>> G1BlockOffsetSharedArray::N_bytes);
>>
>> It seems a bit odd to me to do this calculation on the sizes instead
>> of the addresses in the region. Also, the naming is confusing since
>> "top" almost always refers to the topmost allocated address in a
>> particular memory range.
>> Can you change the above to
>> HeapWord* top = cur->top();
>> HeapWord* aligned_top = align_ptr_up...
>>
>>
>> 7024
>> 7025     size_t to_allocate_words = (aligned_top - top) / HeapWordSize;
>>
>> The subtraction should be replaced by a call to pointer_delta if you
>> change the types of top and aligned_top to pointers.
>>
>> 7026     if ((cur->top() + to_allocate_words) != cur->end() && // Not
>> completely filled yet.
>> 7027         (to_allocate_words != 0) && // Not at card boundary.
>> 7028         (to_allocate_words <= CollectedHeap::min_fill_size())) {
>> 7029       // Cannot fit any object into the area to the next card
>> boundary. Simply create
>> 7030       // an object that fits a single object.
>> 7031       to_allocate_words += CollectedHeap::min_fill_size();
>> 7032     }
>>
>> Generally, if you feel the need to explain each condition in a
>> three-clause if statement then maybe it's a good idea to rephrase it
>> in some way. Maybe use a MAX2-expression to get above the minimum
>> object size and move the "to_allocate_words != 0" check to encompass
>> the entire filling logic?
>>
>> 7033
>> 7034     if (to_allocate_words != 0) {
>> 7035       HeapWord* dummy =
>> G1AllocRegion::attempt_allocation(to_allocate_words, true /*
>> bot_updates */);
>>
>> You don't need the G1AllocRegion scope prefix here, attempt_allocation
>> is in the current scope.
>>
>> 7036       CollectedHeap::fill_with_object(dummy, to_allocate_words);
>> 7037     }
>>
>> /Mikael
>>
>>>
>>> The crash happens with the following stack trace:
>>> Stack: [0x00007fd435a1f000,0x00007fd435b20000], sp=0x00007fd435b1bc30,
>>> free space=1011k
>>> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code,
>>> C=native code)
>>> V  [libjvm.so+0x541261]
>>> G1BlockOffsetArray::forward_to_block_containing_addr_slow(HeapWord*,
>>> HeapWord*, void const*)+0xf1
>>> V  [libjvm.so+0x959e54]
>>> DirtyCardToOopClosure::do_MemRegion(MemRegion)+0x64
>>> V  [libjvm.so+0x56d2a4] ScanRSClosure::doHeapRegion(HeapRegion*)+0x374
>>> V  [libjvm.so+0x542dd0]
>>> G1CollectedHeap::collection_set_iterate_from(HeapRegion*,
>>> HeapRegionClosure*)+0x60
>>> V  [libjvm.so+0x56c08c]  G1RemSet::scanRS(OopsInHeapRegionClosure*,
>>> CodeBlobToOopClosure*, int)+0xdc
>>> V  [libjvm.so+0x56c4d5]
>>> G1RemSet::oops_into_collection_set_do(OopsInHeapRegionClosure*,
>>> CodeBlobToOopClosure*, int)+0x145
>>> V  [libjvm.so+0x549ef4] G1CollectedHeap::g1_process_strong_roots(bool,
>>> SharedHeap::ScanningOption, OopClosure*, OopsInHeapRegionClosure*,
>>> G1KlassScanClosure*, int)+0x224
>>> V  [libjvm.so+0x558a88]  G1ParTask::work(unsigned int)+0xb88
>>> V  [libjvm.so+0xa4a9ff]  GangWorker::loop()+0xcf
>>> V  [libjvm.so+0x8a0058]  java_start(Thread*)+0x108
>>>
>>> Here, the GC thread crashes while scanning the RemSet (part of the
>>> non-CSet regions). And it happens to scan a region to which another
>>> thread in G1ParEvacuateFollowersClosure is copying contents to, and this
>>> region was found out to be an Old GC alloc region.
>>>
>>> This change makes sure that we fill up the last card of the Old GC alloc
>>> region that was being allocated into, with a dummy object so that it
>>> does not get scanned by the remset scanning GC threads.
>>>
>>> This change applies cleanly to 8u and 7u repos.
>>>
>>> Thanks to Thomas Schatzl for his help in investigating the crash and
>>> suggesting this solution.
>>>
>>> Testing:
>>> - JPRT
>>> - Testing by Coherence QA
>>>
>>> Thanks,
>>> Poonam
>>>