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

[Peter Xu <peterx@redhat.com>]

On Wed, Feb 04, 2026 at 02:07:02PM +0400, 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.

The split of sources v.s. manager makes sense to me.

I didn't follow the whole history of how the discard manager evolved, but
IIUC it was called "discard manager" only because initially we were pretty
much focused on the DONTNEED side of things so that when things got dropped
we need to notify.

However IIUC the current status quo of the discard manager covers both
sides (population and discards).  It maintains what ranges of memory is
available in general for consumption by listeners.

For that, I wonder if during this major refactoring we should think about
renaming this slightly misleading name.  Considering that the major
difference of such special MR (either managed by virtio-mem or ramattr) is
about its "sparseness": say, not all of the memory is used but only a
portion of it to be used in a somehow sparsed fashion, maybe
SparseRamSources / SparseRamManager?  Another option I thought about is
DynamicRam*.

No strong feelings on the namings.  But raising this up in case you also
think it might be a good idea.

[...]

> @@ -699,50 +682,60 @@ struct RamDiscardManagerClass {
>       * @replay_discarded:
>       *
>       * Call the #ReplayRamDiscardState callback for all discarded parts within
> -     * the #MemoryRegionSection via the #RamDiscardManager.
> +     * the #MemoryRegionSection via the #RamDiscardSource.
>       *
> -     * @rdm: the #RamDiscardManager
> +     * @rds: the #RamDiscardSource
>       * @section: the #MemoryRegionSection
>       * @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_discarded)(const RamDiscardManager *rdm,
> +    int (*replay_discarded)(const RamDiscardSource *rds,
>                              MemoryRegionSection *section,
>                              ReplayRamDiscardState replay_fn, void *opaque);

A major question I have here is how we should process replay_populated()
and replay_discarded() when split the class.

I had a gut feeling that this should not belong to the *Source object,
instead it might be more suitable for the manager?

I read into some later patches and I fully agree with your definition of
aggregations when there're multiple sources, which mentioned in the commit
messsages later of this part:

    The aggregation uses:
    - Populated: ALL sources populated
    - Discarded: ANY source discarded

IOW, a piece of mem that is "private" in terms of ram attr manager, but
"populated" in terms of virtio-mem, should be treated as discarded indeed.

However I also saw that in some follow up patches the series did a trick to
fetch the first Source then invoke the replay hook on the first source:

int ram_discard_manager_replay_populated(const RamDiscardManager *rdm,
                                         const MemoryRegionSection *section,
                                         ReplayRamDiscardState replay_fn,
                                         void *opaque)
{
    RamDiscardSourceEntry *first;
    ReplayCtx ctx;

    first = QLIST_FIRST(&rdm->source_list);
    if (!first) {
        return replay_fn(section, opaque);
    }

    ctx.rdm = rdm;
    ctx.replay_fn = replay_fn;
    ctx.user_opaque = opaque;

    return ram_discard_source_replay_populated(first->rds, section,
                                               aggregated_replay_populated_cb,
                                               &ctx);
}

It seems to me that aggregated_replay_populated_cb() will indeed still do
the proper aggregations, however if we could move the replay functions into
the manager (or do we need it to be a hook at all?  Perhaps it will just be
an API of the manager?), then we don't need this trick of fetching the 1st
source, instead the manager should do the math of "AND all the sources on
reported populated info" then invoke the listener handlers.  Maybe that'll
be cleaner from the high level.

What do you think?

Thanks,

-- 
Peter Xu