Re: [PATCH v6 01/12] memory: Introduce RamDiscardMgr for RAM memory regions

[Paolo Bonzini]

On 22/02/21 15:03, David Hildenbrand wrote:
> > > +    /**
> > > +     * @replay_populated:
> > > +     *
> > > +     * Notify the #RamDiscardListener about all populated parts 
> > > within the
> > > +     * #MemoryRegion via the #RamDiscardMgr.
> > > +     *
> > > +     * In case any notification fails, no further notifications are 
> > > triggered.
> > > +     * The listener is not required to be registered.
> > > +     *
> > > +     * @rdm: the #RamDiscardMgr
> > > +     * @mr: the #MemoryRegion
> > > +     * @rdl: the #RamDiscardListener
> > > +     *
> > > +     * Returns 0 on success, or a negative error if any notification 
> > > failed.
> > > +     */
> > > +    int (*replay_populated)(const RamDiscardMgr *rdm, const 
> > > MemoryRegion *mr,
> > > +                            RamDiscardListener *rdl);
> >
> > If this function is only going to use a single callback, just pass it
> > (together with a void *opaque) as the argument.
> > > +};
> > > +
> > >   typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > >   typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > > @@ -487,6 +683,7 @@ struct MemoryRegion {
> > >       const char *name;
> > >       unsigned ioeventfd_nb;
> > >       MemoryRegionIoeventfd *ioeventfds;
> > > +    RamDiscardMgr *rdm; /* Only for RAM */
> > >   };
> >
> >
> > The idea of sending discard notifications is obviously good.  I have a
> > couple of questions on the design that you used for the interface; I'm
> > not saying that it should be done differently, I would only like to
> > understand the trade offs that you chose:
>
> Sure!
>
> > 1) can the RamDiscardManager (no abbreviations :)) be just the owner of
>
> I used to call it "SparseRamManager", but wanted to stress the semantics 
> - and can use RamDiscardManager ;) . Suggestions welcome.
>
> > the memory region (obj->parent)?  Alternatively, if you want to make it
> > separate from the owner, does it make sense for it to be a separate
> > reusable object, sitting between virtio-mem and the MemoryRegion, so
> > that the implementation can be reused?
>
> virtio-mem consumes a memory backend (e.g., memory-backend-ram). That 
> one logically "ownes" the memory region (and thereby the RAMBlock).
>
> The memory backend gets assigned to virtio-mem. At that point, 
> virtio-mem "owns" the memory backend. It will set itself as the 
> RamDiscardsManager before mapping the memory region into system address 
> space (whereby it will get exposed to the system).
>
> This flow made sense to me. Regarding "reusable object" - I think the 
> only stuff we could fit in there would be e.g., maintaining the lists of 
> notifiers. I'd rather wait until we actually have a second user to see 
> what we can factor out.
>
> If you have any suggestion/preference, please let me know.
>
> > 2) why have the new RamDiscardListener instead of just extending
> > MemoryListener? Conveniently, vfio already has a MemoryListener that can
>
> It behaves more like the IOMMU notifier in that regard.

Yes, but does it behave more like the IOMMU notifier in other regards? 
:)  The IOMMU notifier is concerned with an iova concept that doesn't 
exist at the MemoryRegion level, while RamDiscardListener works at the 
(MemoryRegion, offset) level that can easily be represented by a 
MemoryRegionSection.  Using MemoryRegionSection might even simplify the 
listener code.

> > be extended, and you wouldn't need the list of RamDiscardListeners.
> > There is already a precedent of replaying the current state when a
> > listener is added (see listener_add_address_space), so this is not
> > something different between ML and RDL.
>
> The main motivation is to let listener decide how it wants to handle the 
> memory region. For example, for vhost, vdpa, kvm, ... I only want a 
> single region, not separate ones for each and every populated range, 
> punching out discarded ranges. Note that there are cases (i.e., 
> anonymous memory), where it's even valid for the guest to read discarded 
> memory.

Yes, I agree with that.  You would still have the same 
region-add/region_nop/region_del callbacks for KVM and friends; on top 
of that you would have region_populate/region_discard callbacks for VFIO.

Populated regions would be replayed after region_add, while I don't 
think it makes sense to have a region_discard_all callback before 
region_discard.

Paolo

> Special cases are only required in corner cases, namely whenever we 
> unconditionally:
>
> a) Read memory inside a memory region. (e.g., guest-memory-dump)
> b) Write memory inside a memory region. (e.g., TPM, migration)
> c) Populate memory inside a memory region. (e.g., vfio)
>
> > Also, if you add a new interface, you should have "method call" wrappers
> > similar to user_creatable_complete or user_creatable_can_be_deleted.
>
> I think I had those at some point but decided to drop them. Can readd them.