Re: [PATCH v5 01/12] system/memory: split RamDiscardManager into source and manager

On 4/6/26 15:43, Marc-André Lureau wrote:
> 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).
> 
> Reviewed-by: Peter Xu <[email protected]>
> Acked-by: David Hildenbrand <[email protected]>
> Signed-off-by: Marc-André Lureau <[email protected]>
> ---
>   include/hw/virtio/virtio-mem.h |   3 -
>   include/system/memory.h        | 197 +++++++++++++++++++++----------------
>   include/system/ramblock.h      |   2 -
>   hw/virtio/virtio-mem.c         | 163 ++++++------------------------
>   system/memory.c                | 218 ++++++++++++++++++++++++++++++++++++-----
>   system/ram-block-attributes.c  | 171 ++++++++++----------------------
>   6 files changed, 385 insertions(+), 369 deletions(-)


> +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) {

(Pre-existing, reduce scope?)

            RamDiscardListener *rdl2;

> +        /* 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;
> +}


>   void ram_discard_manager_register_listener(RamDiscardManager *rdm,
>                                              RamDiscardListener *rdl,
>                                              MemoryRegionSection *section)
>   {
> -    RamDiscardManagerClass *rdmc = RAM_DISCARD_MANAGER_GET_CLASS(rdm);
> +    int ret;
> +
> +    g_assert(section->mr == rdm->mr);
> +
> +    rdl->section = memory_region_section_new_copy(section);
> +    QLIST_INSERT_HEAD(&rdm->rdl_list, rdl, next);
>   
> -    g_assert(rdmc->register_listener);
> -    rdmc->register_listener(rdm, rdl, section);
> +    ret = ram_discard_source_replay_populated(rdm->rds, rdl->section,
> +                                              rdm_populate_cb, rdl);
> +    if (ret) {
> +        error_report("%s: Replaying populated ranges failed: %s", __func__,
> +                     strerror(-ret));

Pre-existing, should ram_discard_manager_register_listener propagate
Error*? For example the current caller provides an errp
(vfio_ram_discard_register_listener). Can be improved on top of course.

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>

> +    }
>   }