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

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

On 5/26/2025 5:35 PM, Philippe Mathieu-Daudé wrote:
> Hi Chenyi Qiang,
> 
> On 20/5/25 12:28, Chenyi Qiang wrote:
>> Update ReplayRamDiscard() function to return the result and unify the
>> ReplayRamPopulate() and ReplayRamDiscard() to ReplayRamDiscardState() at
>> the same time due to their identical definitions. This unification
>> simplifies related structures, such as VirtIOMEMReplayData, which makes
>> it cleaner.
>>
>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> ---
>> Changes in v5:
>>      - Rename ReplayRamStateChange to ReplayRamDiscardState (David)
>>      - return data->fn(s, data->opaque) instead of 0 in
>>        virtio_mem_rdm_replay_discarded_cb(). (Alexey)
>>
>> 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.
>> ---
>>   hw/virtio/virtio-mem.c  | 21 ++++++++++-----------
>>   include/system/memory.h | 36 +++++++++++++++++++-----------------
>>   migration/ram.c         |  5 +++--
>>   system/memory.c         | 12 ++++++------
>>   4 files changed, 38 insertions(+), 36 deletions(-)
> 
> 
>> diff --git a/include/system/memory.h b/include/system/memory.h
>> index 896948deb1..83b28551c4 100644
>> --- a/include/system/memory.h
>> +++ b/include/system/memory.h
>> @@ -575,8 +575,8 @@ static inline void
>> ram_discard_listener_init(RamDiscardListener *rdl,
>>       rdl->double_discard_supported = double_discard_supported;
>>   }
>>   -typedef int (*ReplayRamPopulate)(MemoryRegionSection *section, void
>> *opaque);
>> -typedef void (*ReplayRamDiscard)(MemoryRegionSection *section, void
>> *opaque);
>> +typedef int (*ReplayRamDiscardState)(MemoryRegionSection *section,
>> +                                     void *opaque);
> 
> While changing this prototype, please add a documentation comment.

[...]

> 
>>   /*
>>    * RamDiscardManagerClass:
>> @@ -650,36 +650,38 @@ struct RamDiscardManagerClass {
>>       /**
>>        * @replay_populated:
>>        *
>> -     * Call the #ReplayRamPopulate callback for all populated parts
>> within the
>> -     * #MemoryRegionSection via the #RamDiscardManager.
>> +     * Call the #ReplayRamDiscardState callback for all populated
>> parts within
>> +     * the #MemoryRegionSection via the #RamDiscardManager.
>>        *
>>        * In case any call fails, no further calls are made.
>>        *
>>        * @rdm: the #RamDiscardManager
>>        * @section: the #MemoryRegionSection
>> -     * @replay_fn: the #ReplayRamPopulate callback
>> +     * @replay_fn: the #ReplayRamDiscardState callback
>>        * @opaque: pointer to forward to the callback
>>        *
>>        * Returns 0 on success, or a negative error if any notification
>> failed.
>>        */
>>       int (*replay_populated)(const RamDiscardManager *rdm,
>>                               MemoryRegionSection *section,
>> -                            ReplayRamPopulate replay_fn, void *opaque);
>> +                            ReplayRamDiscardState replay_fn, void
>> *opaque);
>>         /**
>>        * @replay_discarded:
>>        *
>> -     * Call the #ReplayRamDiscard callback for all discarded parts
>> within the
>> -     * #MemoryRegionSection via the #RamDiscardManager.
>> +     * Call the #ReplayRamDiscardState callback for all discarded
>> parts within
>> +     * the #MemoryRegionSection via the #RamDiscardManager.
>>        *
>>        * @rdm: the #RamDiscardManager
>>        * @section: the #MemoryRegionSection
>> -     * @replay_fn: the #ReplayRamDiscard callback
>> +     * @replay_fn: the #ReplayRamDiscardState callback
>>        * @opaque: pointer to forward to the callback
>> +     *
>> +     * Returns 0 on success, or a negative error if any notification
>> failed.
>>        */
>> -    void (*replay_discarded)(const RamDiscardManager *rdm,
>> -                             MemoryRegionSection *section,
>> -                             ReplayRamDiscard replay_fn, void *opaque);
>> +    int (*replay_discarded)(const RamDiscardManager *rdm,
>> +                            MemoryRegionSection *section,
>> +                            ReplayRamDiscardState replay_fn, void
>> *opaque);
>>         /**
>>        * @register_listener:
>> @@ -722,13 +724,13 @@ bool ram_discard_manager_is_populated(const
>> RamDiscardManager *rdm,
>>     int ram_discard_manager_replay_populated(const RamDiscardManager
>> *rdm,
>>                                            MemoryRegionSection *section,
>> -                                         ReplayRamPopulate replay_fn,
>> +                                         ReplayRamDiscardState
>> replay_fn,
>>                                            void *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,
>> +                                         ReplayRamDiscardState
>> replay_fn,
>> +                                         void *opaque);
> 
> Similar for ram_discard_manager_replay_populated() and
> ram_discard_manager_replay_discarded(), since you understood
> what they do :)

Sure, will do. Thanks!

> 
> Thanks!
> 
> Phil.
>