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

[David Hildenbrand <david@redhat.com>]

>> +    /**
>> +     * @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.

> 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.

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.


-- 
Thanks,

David / dhildenb