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

[David Hildenbrand <david@redhat.com>]

On 22.02.21 18:37, Paolo Bonzini wrote:
> On 22/02/21 15:53, David Hildenbrand wrote:
>>> 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.
> 
> Why does that matter?  I think if it's concerned with the MemoryRegion
> address space it should use MemoryListener and MemoryRegionSection.
> 
>>>> 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 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.
> 
> Right, that was the reason why I was suggesting different callbacks.
> For the VFIO listener, which doesn't have begin or commit callbacks, I
> think you could just rename region_add to region_populate, and point
> both region_del and region_discard to the existing region_del commit.
> 
> Calling log_sync/log_clear only for populated parts also makes sense.
> log_sync and log_clear do not have to be within begin/commit, so you can
> change the semantics to call them more than once.

I'll prototype to see how it looks/feels. As long as it's moving logic 
out of the VFIO code into the address space update code it could be 
quite alright.

-- 
Thanks,

David / dhildenb