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

[David Hildenbrand <david@redhat.com>]

On 22.02.21 15:18, Paolo Bonzini wrote:
> 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.

It's similarly concerned with rather small, lightweight updates I would say.

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

I see roughly how it could work, however, I am not sure yet if this is 
the right approach.

I think instead of region_populate/region_discard we would want 
individual region_add/region_del when populating/discarding for all 
MemoryListeners that opt-in somehow (e.g., VFIO, dump-guest-memory, 
...). Similarly, we would want to call log_sync()/log_clear() then only 
for these parts.

But what happens when I populate/discard some memory? I don't want to 
trigger an address space transaction (begin()...region_nop()...commit()) 
- whenever I populate/discard memory (e.g., in 2 MB granularity). 
Especially not, if nothing might have changed for most other 
MemoryListeners.

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

How would we handle vfio_listerner_log_sync()vfio_sync_dirty_bitmap()?

-- 
Thanks,

David / dhildenb