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

[Paolo Bonzini]

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.



Paolo