Re: [PATCH v4 03/13] memory: Unify the definiton of ReplayRamPopulate() and ReplayRamDiscard()

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 4/25/2025 8:42 PM, David Hildenbrand wrote:
> On 07.04.25 09:49, Chenyi Qiang wrote:
>> Update ReplayRamDiscard() function to return the result and unify the
>> ReplayRamPopulate() and ReplayRamDiscard() to ReplayStateChange() at
>> the same time due to their identical definitions. This unification
>> simplifies related structures, such as VirtIOMEMReplayData, which makes
>> it more cleaner and maintainable.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Changes in v4:
>>      - Modify the commit message. We won't use Replay() operation when
>>        doing the attribute change like v3.
>>
>> Changes in v3:
>>      - Newly added.
>> ---
> 
> [...]
> 
>>   -typedef int (*ReplayRamPopulate)(MemoryRegionSection *section, void
>> *opaque);
>> -typedef void (*ReplayRamDiscard)(MemoryRegionSection *section, void
>> *opaque);
>> +typedef int (*ReplayStateChange)(MemoryRegionSection *section, void
>> *opaque);
>>   
> 
> But it's not a state change.
> 
> ReplayRamState maybe?

OK. Will rename it to ReplayRamDiscardState as mentioned in another
thread. Thanks.

> 
> [...]
>>   /*
>> diff --git a/system/memory.c b/system/memory.c
>> index 62d6b410f0..b5ab729e13 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -2147,7 +2147,7 @@ bool ram_discard_manager_is_populated(const
>> RamDiscardManager *rdm,
>>     int ram_discard_manager_replay_populated(const RamDiscardManager
>> *rdm,
>>                                            MemoryRegionSection *section,
>> -                                         ReplayRamPopulate replay_fn,
>> +                                         ReplayStateChange replay_fn,
>>                                            void *opaque)
>>   {
>>       RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_GET_CLASS(rdm);
>> @@ -2156,15 +2156,15 @@ int ram_discard_manager_replay_populated(const
>> RamDiscardManager *rdm,
>>       return rdmc->replay_populated(rdm, section, replay_fn, opaque);
>>   }
>>   -void ram_discard_manager_replay_discarded(const RamDiscardManager
>> *rdm,
>> -                                          MemoryRegionSection *section,
>> -                                          ReplayRamDiscard replay_fn,
>> -                                          void *opaque)
>> +int ram_discard_manager_replay_discarded(const RamDiscardManager *rdm,
>> +                                         MemoryRegionSection *section,
>> +                                         ReplayStateChange replay_fn,
>> +                                         void *opaque)
>>   {
>>       RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_GET_CLASS(rdm);
>>         g_assert(rdmc->replay_discarded);
>> -    rdmc->replay_discarded(rdm, section, replay_fn, opaque);
>> +    return rdmc->replay_discarded(rdm, section, replay_fn, opaque);
>>   }
> 
> The idea was that ram_discard_manager_replay_discarded() would never be
> able to fail. But I don't think this really matters, because the
> function is provided by the caller, that can just always return 0 --
> like we do in dirty_bitmap_clear_section() now.
> 
> So yeah, this looks fine to me, given that we don't call it a "state
> change" when we are merely replaying a selected state.
>