RFR: 8352185: Shenandoah: Invalid logic for remembered set verification [v11]

[William Kemper]

On Thu, 20 Mar 2025 20:06:58 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`
>
> Xiaolong Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix wrong comments

Couple of nits.

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

> 709:   }
> 710: 
> 711:   if (ShenandoahVerify && heap->mode()->is_generational()) {

Are we calling `verify_before_concmark` twice in generational mode? Should we delete this second call here?

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

> 1375:   log_debug(gc)("Verifying remembered set at %s mark", old_generation->is_doing_mixed_evacuations() ? "mixed" : "young");
> 1376: 
> 1377:   ShenandoahWriteTableScanner scanner(ShenandoahGenerationalHeap::heap()->old_generation()->card_scan());

Can use existing local variable: `old_generation`?

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

Changes requested by wkemper (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24092#pullrequestreview-2703995429
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r2006389044
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r2006383550