Re: [PATCH v3 06/15] system/memory: split RamDiscardManager into source and manager

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

On 2/26/2026 9:59 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Refactor the RamDiscardManager interface into two distinct components:
> - RamDiscardSource: An interface that state providers (virtio-mem,
>   RamBlockAttributes) implement to provide discard state information
>   (granularity, populated/discarded ranges, replay callbacks).
> - RamDiscardManager: A concrete QOM object that wraps a source, owns
>   the listener list, and handles listener registration/unregistration
>   and notifications.
> 
> This separation moves the listener management logic from individual
> source implementations into the central RamDiscardManager, reducing
> code duplication between virtio-mem and RamBlockAttributes.
> 
> The change prepares for future work where a RamDiscardManager could
> aggregate multiple sources.
> 
> Note, the original virtio-mem code had conditions before discard:
>   if (vmem->size) {
>       rdl->notify_discard(rdl, rdl->section);
>   }
> however, the new code calls discard unconditionally. This is considered
> safe, since the populate/discard of sections are already asymmetrical
> (unplug & unregister all listener section unconditionally).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/hw/virtio/virtio-mem.h |   3 -
>  include/system/memory.h        | 197 ++++++++++++++++-------------
>  include/system/ramblock.h      |   3 +-
>  hw/virtio/virtio-mem.c         | 163 +++++-------------------
>  system/memory.c                | 218 +++++++++++++++++++++++++++++----
>  system/ram-block-attributes.c  | 171 ++++++++------------------
>  6 files changed, 386 insertions(+), 369 deletions(-)
> 

[...]

> diff --git a/system/memory.c b/system/memory.c
> index c51d0798a84..3e7fd759692 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2105,34 +2105,88 @@ RamDiscardManager *memory_region_get_ram_discard_manager(MemoryRegion *mr)
>      return mr->rdm;
>  }
>  
> -int memory_region_set_ram_discard_manager(MemoryRegion *mr,
> -                                          RamDiscardManager *rdm)
> +static RamDiscardManager *ram_discard_manager_new(MemoryRegion *mr,
> +                                                  RamDiscardSource *rds)
> +{
> +    RamDiscardManager *rdm = RAM_DISCARD_MANAGER(object_new(TYPE_RAM_DISCARD_MANAGER));
> +
> +    rdm->rds = rds;
> +    rdm->mr = mr;
> +    QLIST_INIT(&rdm->rdl_list);

Is this QLIST_INIT() redundant since it is already called in ram_discard_manager_initfn()?

> +    return rdm;
> +}
> +

[...]

> +}
> +
> +static void ram_discard_manager_initfn(Object *obj)
> +{
> +    RamDiscardManager *rdm = RAM_DISCARD_MANAGER(obj);
> +
> +    QLIST_INIT(&rdm->rdl_list);
> +}
> +
> +static void ram_discard_manager_finalize(Object *obj)
> +{
> +    RamDiscardManager *rdm = RAM_DISCARD_MANAGER(obj);
>  
> -    g_assert(rdmc->replay_discarded);
> -    return rdmc->replay_discarded(rdm, section, replay_fn, opaque);
> +    g_assert(QLIST_EMPTY(&rdm->rdl_list));
> +}
> +
> +int ram_discard_manager_notify_populate(RamDiscardManager *rdm,
> +                                        uint64_t offset, uint64_t size)
> +{
> +    RamDiscardListener *rdl, *rdl2;
> +    int ret = 0;
> +
> +    QLIST_FOREACH(rdl, &rdm->rdl_list, next) {
> +        MemoryRegionSection tmp = *rdl->section;
> +
> +        if (!memory_region_section_intersect_range(&tmp, offset, size)) {
> +            continue;
> +        }
> +        ret = rdl->notify_populate(rdl, &tmp);
> +        if (ret) {
> +            break;
> +        }
> +    }
> +
> +    if (ret) {
> +        /* Notify all already-notified listeners about discard. */
> +        QLIST_FOREACH(rdl2, &rdm->rdl_list, next) {
> +            MemoryRegionSection tmp = *rdl2->section;
> +
> +            if (rdl2 == rdl) {
> +                break;
> +            }
> +            if (!memory_region_section_intersect_range(&tmp, offset, size)) {
> +                continue;
> +            }
> +            rdl2->notify_discard(rdl2, &tmp);
> +        }
> +    }
> +    return ret;
> +}
> +

[...]

>  
>  static int
>  ram_block_attributes_notify_populate(RamBlockAttributes *attr,
>                                       uint64_t offset, uint64_t size)
>  {
> -    RamDiscardListener *rdl;
> -    int ret = 0;
> -
> -    QLIST_FOREACH(rdl, &attr->rdl_list, next) {
> -        MemoryRegionSection tmp = *rdl->section;
> -
> -        if (!memory_region_section_intersect_range(&tmp, offset, size)) {
> -            continue;
> -        }
> -        ret = rdl->notify_populate(rdl, &tmp);
> -        if (ret) {
> -            break;
> -        }
> -    }
> +    RamDiscardManager *rdm = memory_region_get_ram_discard_manager(attr->ram_block->mr);
>  
> -    return ret;
> +    return ram_discard_manager_notify_populate(rdm, offset, size);
>  }

This change introduces a slight difference, as it will perform a rollback if ->notify_populate() fails.
I believe this is acceptable since, currently, memory conversion failures result in QEMU terminating
rather than resuming the guest or retrying the operation. And this rollback is necessary if we plan to support
retry operations in the future. Therefore, we can retain this modification.