[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: David Hildenbrand
Subject: Re: [PATCH v6 01/12] memory: Introduce RamDiscardMgr for RAM memory regions
Date: Tue, 23 Feb 2021 11:50:11 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0

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

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

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

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.


David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]