[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [PATCH v6 01/12] memory: Introduce RamDiscardMgr for RAM memory regions
Date: Mon, 22 Feb 2021 15:18:05 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0

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:


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.


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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]