Hi
On Thu, Feb 19, 2026 at 8:40 PM Peter Xu <[email protected]> wrote:
>
> On Wed, Feb 04, 2026 at 02:07:02PM +0400, [email protected] wrote:
> > From: Marc-André Lureau <[email protected]>
> >
> > 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.
That makes sense, it can be done later though, it's not blocking imho
>
> [...]
>
> > @@ -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?
Do you mean that we could drop replay_populated/replay_discarded from
the source? I think we could. replay_by_populated_state() already
handles the aggregation logic by using is_populated() alone. There
isn't much gain in the above implementation narrowing the iteration
from the first source replay.
thanks for the review
