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

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

On 4/9/2025 1:43 PM, Alexey Kardashevskiy wrote:
> 
> 
> On 7/4/25 17: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.
>> ---
>>   hw/virtio/virtio-mem.c | 20 ++++++++++----------
>>   include/exec/memory.h  | 31 ++++++++++++++++---------------
>>   migration/ram.c        |  5 +++--
>>   system/memory.c        | 12 ++++++------
>>   4 files changed, 35 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
>> index d0d3a0240f..1a88d649cb 100644
>> --- a/hw/virtio/virtio-mem.c
>> +++ b/hw/virtio/virtio-mem.c
>> @@ -1733,7 +1733,7 @@ static bool virtio_mem_rdm_is_populated(const
>> RamDiscardManager *rdm,
>>   }
>>     struct VirtIOMEMReplayData {
>> -    void *fn;
>> +    ReplayStateChange fn;
> 
> 
> s/ReplayStateChange/ReplayRamStateChange/
> 
> Just "State" is way too generic imho.

LGTM.

> 
> 
>>       void *opaque;
>>   };
>>   @@ -1741,12 +1741,12 @@ static int
>> virtio_mem_rdm_replay_populated_cb(MemoryRegionSection *s, void *arg)
>>   {
>>       struct VirtIOMEMReplayData *data = arg;
>>   -    return ((ReplayRamPopulate)data->fn)(s, data->opaque);
>> +    return data->fn(s, data->opaque);
>>   }
>>     static int virtio_mem_rdm_replay_populated(const RamDiscardManager
>> *rdm,
>>                                              MemoryRegionSection *s,
>> -                                           ReplayRamPopulate replay_fn,
>> +                                           ReplayStateChange replay_fn,
>>                                              void *opaque)
>>   {
>>       const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
>> @@ -1765,14 +1765,14 @@ static int
>> virtio_mem_rdm_replay_discarded_cb(MemoryRegionSection *s,
>>   {
>>       struct VirtIOMEMReplayData *data = arg;
>>   -    ((ReplayRamDiscard)data->fn)(s, data->opaque);
>> +    data->fn(s, data->opaque);
>>       return 0;
> 
> return data->fn(s, data->opaque); ?
> 
> Or a comment why we ignore the return result? Thanks,

You are right, since we have made ReplayRamDiscard() return the results,
it is doable to use "return data->fn(s, data->opaque)" directly. Thanks.

> 
>>   }
>>   -static void virtio_mem_rdm_replay_discarded(const RamDiscardManager
>> *rdm,
>> -                                            MemoryRegionSection *s,
>> -                                            ReplayRamDiscard replay_fn,
>> -                                            void *opaque)
>> +static int virtio_mem_rdm_replay_discarded(const RamDiscardManager *rdm,
>> +                                           MemoryRegionSection *s,
>> +                                           ReplayStateChange replay_fn,
>> +                                           void *opaque)
>>   {
>>       const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
>>       struct VirtIOMEMReplayData data = {
>> @@ -1781,8 +1781,8 @@ static void
>> virtio_mem_rdm_replay_discarded(const RamDiscardManager *rdm,
>>       };
>>         g_assert(s->mr == &vmem->memdev->mr);
>> -    virtio_mem_for_each_unplugged_section(vmem, s, &data,
>> -                                         
>> virtio_mem_rdm_replay_discarded_cb);
>> +    return virtio_mem_for_each_unplugged_section(vmem, s, &data,
>> +                                                
>> virtio_mem_rdm_replay_discarded_cb);
> 
> 
> a nit: "WARNING: line over 80 characters" - I have no idea what is the
> best thing to do here though.

It is not easy to adjust the line. Then I think we can keep it.

> 
> 
>