RFR: 8352185: Shenandoah: Invalid logic for remembered set verification

[Kelvin Nilsen]

On Tue, 18 Mar 2025 00:19:35 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

> There are some scenarios in which GenShen may have improper remembered set verification logic:
> 
> 1. Concurrent young cycles following a Full GC:
> 
> In the end of ShenandoahFullGC, it resets bitmaps for the entire heap w/o resetting marking context to be incomplete, but ShenandoahVerifier has code like below to get a complete old marking context for remembered set verification
> 
> 
> ShenandoahVerifier  
> ShenandoahMarkingContext* ShenandoahVerifier::get_marking_context_for_old() {
>   shenandoah_assert_generations_reconciled();
>   if (_heap->old_generation()->is_mark_complete() || _heap->gc_generation()->is_global()) {
>     return _heap->complete_marking_context();
>   }
>   return nullptr;
> }
> 
> 
> For the concurrent young GC cycles after a full GC, the old marking context used for remembered set verification is stale, and may cause unexpected result. 
> 
> 2. For the impl of `ShenandoahVerifier::get_marking_context_for_old` mentioned above, it always return a marking context for global GC, but marking bitmaps is already reset before before init-mark, `ShenandoahVerifier::help_verify_region_rem_set` always skip verification in this case. 
> 
> 3. ShenandoahConcurrentGC always clean remembered set read table, but only swap read/write table when gc generation is young, this issue causes remembered set verification before init-mark to use a completely clean remembered set, but it is covered by issue 2. 
> 
> 
> ### Test
> - [x] `make test TEST=hotspot_gc_shenandoah`

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 648:

> 646:   assert(!heap->has_forwarded_objects(), "No forwarded objects on this path");
> 647: 
> 648:   if (heap->mode()->is_generational()) {

I think we do not want to change this code.  We only swap remembered set for young-gen because gen will scan the remset and reconstruct it with more updated information.  For  a global GC, we do not scan the remset and do not reconstruct it.  If we swap here, we will lose the information that is currently within the remset.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 953:

> 951:     // pinned regions.
> 952:     if (!r->is_pinned()) {
> 953:       _heap->marking_context()->reset_top_at_mark_start(r);

Here, and below, I think we want to keep complete_marking_context() rather than changing to marking_context()

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 1281:

> 1279: ShenandoahMarkingContext* ShenandoahVerifier::get_marking_context_for_old() {
> 1280:   shenandoah_assert_generations_reconciled();
> 1281:   if (_heap->old_generation()->is_mark_complete()) {

In the case that this is an global GC, we know that old-generation()->is_mark_complete() by virtue of the current program counter, I assume.  (We would only ask for the old marking context if global marking were already finished.)  In the case that we are doing a global GC cycle, I'm guessing that we do not set is-mark-complete for the old generation.  So that's why I believe you need to keep the condition as originally written.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r1999966128
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r1999982949
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r1999975507