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

[David Hildenbrand <david@redhat.com>]

On 22.02.21 20:43, David Hildenbrand wrote:
>>>>> 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.
> 
> So I looked at the simplest of all cases (discard) and I am not convinced yet
> that this is the right approach. I can understand why it looks like this fits
> into the MemoryListener, but I am not sure if gives us any real benefits or
> makes the code any clearer (I'd even say it's the contrary).
> 
> 
> +void memory_region_notify_discard(MemoryRegion *mr, hwaddr offset,
> +                                  hwaddr size)
> +{
> +    hwaddr mr_start, mr_end;
> +    MemoryRegionSection mrs;
> +    MemoryListener *listener;
> +    AddressSpace *as;
> +    FlatView *view;
> +    FlatRange *fr;
> +
> +    QTAILQ_FOREACH(listener, &memory_listeners, link) {
> +        if (!listener->region_discard) {
> +            continue;
> +        }
> +        as = listener->address_space;
> +        view = address_space_get_flatview(as);
> +        FOR_EACH_FLAT_RANGE(fr, view) {
> +            if (fr->mr != mr) {
> +                continue;
> +            }
> +
> +            mrs = section_from_flat_range(fr, view);
> +
> +            mr_start = MAX(mrs.offset_within_region, offset);
> +            mr_end = MIN(offset + size,
> +                          mrs.offset_within_region + int128_get64(mrs.size));
> +            mr_end = MIN(mr_end, offset + size);
> +
> +            if (mr_start >= mr_end) {
> +                continue;
> +            }
> +
> +            mrs.offset_within_address_space += mr_start -
> +                                               mrs.offset_within_region;
> +            mrs.offset_within_region = mr_start;
> +            mrs.size = int128_make64(mr_end - mr_start);
> +            listener->region_discard(listener, &mrs);
> +        }
> +        flatview_unref(view);
> +    }
> +}
> 
> Maybe I am missing something important. This looks highly inefficient.
> 
> 1. Although we know the memory region we have to walk over the whole address
>      space ... over and over again for each potential listener.
> 
> 2. Even without any applicable listeners (=> ! VFIO) we loop over all listeners.
>      There are ways around that but it doesn't make the code nicer IMHO.
> 
> 3. In the future I am planning on sending populate/discard events
>      without the BQL (in my approach, synchronizing internally against
>      register/unregister/populate/discard ...). I don't see an easy way
>      to achieve that here. I think we are required to hold the BQL on any
>      updates.
> 
> memory_region_notify_populate() gets quite ugly when we realize halfway that
> we have to revert what we already did by notifying about already populated
> pieces ...
> 

So, the more I look into it the more I doubt this should go into the 
MemoryListener.

The RamDiscardManager is specific to RAM memory regions - similarly the 
IOMMU notifier is specific to IOMMU regions.

In the near future we will have two "clients" (vfio, 
tpm/dump-guest-memory), whereby only vfio will actually has to register 
for updates at runtime.

I really want to have a dedicated registration/notifier mechanism, for 
reasons already mentioned in my last mail, but also to later reuse that 
mechanism in other context as noted in the cover letter:

"Note: At some point, we might want to let RAMBlock users (esp. vfio 
used for nvme://) consume this interface as well. We'll need RAMBlock 
notifier calls when a RAMBlock is getting mapped/unmapped (via the 
corresponding memory region), so we can properly register a listener 
there as well."


However, I do agree that the notifier should work with 
MemoryRegionSection - this will make the "client" code much easier.

The register()/replay_populate() mechanism should consume a 
MemoryRegionSection to work on, and call the listener via (adjusted) 
MemoryRegionSection.

Maybe I'm even able to simplify/get rid of the discard_all() callback.

-- 
Thanks,

David / dhildenb