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

[Paolo Bonzini <pbonzini@redhat.com>]

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