[Qemu-devel] [PATCH 0/3] FlatView memory savings

[Qemu-devel] [PATCH 0/3] FlatView memory savings

[Paolo Bonzini]

This gets memory API consumption down to 4 FlatViews when non-virtio
devices are in use.  virtio still wastes memory due to the "modern_as".

Alexey Kardashevskiy (1):
  memory: Share special empty FlatView

Paolo Bonzini (2):
  memory: trace FlatView creation and destruction
  memory: seek FlatView sharing candidates among children subregions

 include/exec/memory.h   |  1 -
 include/qemu/typedefs.h |  1 +
 memory.c                | 60 +++++++++++++++++++++++++++++++++++++++++++------
 trace-events            |  3 +++
 4 files changed, 57 insertions(+), 8 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/3] memory: trace FlatView creation and destruction

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h   | 1 -
 include/qemu/typedefs.h | 1 +
 memory.c                | 3 +++
 trace-events            | 3 +++
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 402824c6f2..5ed4042f87 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -48,7 +48,6 @@
 
 typedef struct MemoryRegionOps MemoryRegionOps;
 typedef struct MemoryRegionMmio MemoryRegionMmio;
-typedef struct FlatView FlatView;
 
 struct MemoryRegionMmio {
     CPUReadMemoryFunc *read[3];
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 163550214c..980d2b330e 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -30,6 +30,7 @@ typedef struct DisplaySurface DisplaySurface;
 typedef struct DriveInfo DriveInfo;
 typedef struct Error Error;
 typedef struct EventNotifier EventNotifier;
+typedef struct FlatView FlatView;
 typedef struct FWCfgEntry FWCfgEntry;
 typedef struct FWCfgIoState FWCfgIoState;
 typedef struct FWCfgMemState FWCfgMemState;
diff --git a/memory.c b/memory.c
index a26adca4d3..4952cc5d84 100644
--- a/memory.c
+++ b/memory.c
@@ -270,6 +270,7 @@ static FlatView *flatview_new(MemoryRegion *mr_root)
     view->ref = 1;
     view->root = mr_root;
     memory_region_ref(mr_root);
+    trace_flatview_new(view, mr_root);
 
     return view;
 }
@@ -295,6 +296,7 @@ static void flatview_destroy(FlatView *view)
 {
     int i;
 
+    trace_flatview_destroy(view, view->root);
     if (view->dispatch) {
         address_space_dispatch_free(view->dispatch);
     }
diff --git a/trace-events b/trace-events
index 1f50f56d9d..1d2eb5d3e4 100644
--- a/trace-events
+++ b/trace-events
@@ -64,6 +64,8 @@ memory_region_tb_read(int cpu_index, uint64_t addr, uint64_t value, unsigned siz
 memory_region_tb_write(int cpu_index, uint64_t addr, uint64_t value, unsigned size) "cpu %d addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
+flatview_new(FlatView *view, MemoryRegion *root) "%p (root %p)"
+flatview_destroy(FlatView *view, MemoryRegion *root) "%p (root %p)"
 
 ### Guest events, keep at bottom
 
-- 
2.13.5

[Qemu-devel] [PATCH 2/3] memory: seek FlatView sharing candidates among children subregions

[Paolo Bonzini]

A container can be used instead of an alias to allow switching between
multiple subregions.  In this case we cannot directly share the
subregions (since they only belong to a single parent), but if the
subregions are aliases we can in turn walk those.

While this does not reduce much the number of FlatViews that are created,
it makes it possible to share the PCI bus master FlatViews and their
AddressSpaceDispatch structures.  For 112 virtio-net-pci devices, boot time
is reduced from 25 to 10 seconds and memory consumption from 1.4 to 1 G.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/memory.c b/memory.c
index 4952cc5d84..3207ae55e2 100644
--- a/memory.c
+++ b/memory.c
@@ -733,12 +733,41 @@ static void render_memory_region(FlatView *view,
 
 static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
 {
-    while (mr->alias && !mr->alias_offset &&
-           int128_ge(mr->size, mr->alias->size)) {
-        /* The alias is included in its entirety.  Use it as
-         * the "real" root, so that we can share more FlatViews.
-         */
-        mr = mr->alias;
+    while (mr->enabled) {
+        if (mr->alias && !mr->alias_offset &&
+            int128_ge(mr->size, mr->alias->size)) {
+            /* The alias is included in its entirety.  Use it as
+             * the "real" root, so that we can share more FlatViews.
+             */
+            mr = mr->alias;
+            continue;
+        }
+
+        if (!mr->terminates) {
+            unsigned int found = 0;
+            MemoryRegion *child, *next = NULL;
+            QTAILQ_FOREACH(child, &mr->subregions, subregions_link) {
+                if (child->enabled) {
+                    if (++found > 1) {
+                        next = NULL;
+                        break;
+                    }
+                    if (!child->addr && int128_ge(mr->size, child->size)) {
+                        /* A child is included in its entirety.  If it's the only
+                         * enabled one, use it in the hope of finding an alias down the
+                         * way. This will also let us share FlatViews.
+                         */
+                        next = child;
+                    }
+                }
+            }
+            if (next) {
+                mr = next;
+                continue;
+            }
+        }
+
+        break;
     }
 
     return mr;
-- 
2.13.5

Re: [Qemu-devel] [PATCH 2/3] memory: seek FlatView sharing candidates among children subregions

[Alexey Kardashevskiy]

On 21/09/17 22:07, Paolo Bonzini wrote:
> A container can be used instead of an alias to allow switching between
> multiple subregions.  In this case we cannot directly share the
> subregions (since they only belong to a single parent), but if the
> subregions are aliases we can in turn walk those.
> 
> While this does not reduce much the number of FlatViews that are created,
> it makes it possible to share the PCI bus master FlatViews and their
> AddressSpaceDispatch structures.  For 112 virtio-net-pci devices, boot time
> is reduced from 25 to 10 seconds and memory consumption from 1.4 to 1 G.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  memory.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 4952cc5d84..3207ae55e2 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -733,12 +733,41 @@ static void render_memory_region(FlatView *view,
>  
>  static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
>  {
> -    while (mr->alias && !mr->alias_offset &&
> -           int128_ge(mr->size, mr->alias->size)) {
> -        /* The alias is included in its entirety.  Use it as
> -         * the "real" root, so that we can share more FlatViews.
> -         */
> -        mr = mr->alias;
> +    while (mr->enabled) {
> +        if (mr->alias && !mr->alias_offset &&
> +            int128_ge(mr->size, mr->alias->size)) {
> +            /* The alias is included in its entirety.  Use it as
> +             * the "real" root, so that we can share more FlatViews.
> +             */
> +            mr = mr->alias;
> +            continue;
> +        }
> +
> +        if (!mr->terminates) {

Can an MR have terminates==true and children by the same time? If so, what for?


> +            unsigned int found = 0;
> +            MemoryRegion *child, *next = NULL;
> +            QTAILQ_FOREACH(child, &mr->subregions, subregions_link) {
> +                if (child->enabled) {
> +                    if (++found > 1) {
> +                        next = NULL;
> +                        break;
> +                    }
> +                    if (!child->addr && int128_ge(mr->size, child->size)) {


Ah, I tried this one but in v4's 18/18 (that hinting thing), did not work
out but for a different reason.


> +                        /* A child is included in its entirety.  If it's the only
> +                         * enabled one, use it in the hope of finding an alias down the
> +                         * way. This will also let us share FlatViews.
> +                         */
> +                        next = child;
> +                    }
> +                }
> +            }
> +            if (next) {
> +                mr = next;
> +                continue;
> +            }
> +        }
> +
> +        break;
>      }
>  
>      return mr;
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/3] memory: seek FlatView sharing candidates among children subregions

[Paolo Bonzini]

On 21/09/2017 15:39, Alexey Kardashevskiy wrote:
> On 21/09/17 22:07, Paolo Bonzini wrote:
>> A container can be used instead of an alias to allow switching between
>> multiple subregions.  In this case we cannot directly share the
>> subregions (since they only belong to a single parent), but if the
>> subregions are aliases we can in turn walk those.
>>
>> While this does not reduce much the number of FlatViews that are created,
>> it makes it possible to share the PCI bus master FlatViews and their
>> AddressSpaceDispatch structures.  For 112 virtio-net-pci devices, boot time
>> is reduced from 25 to 10 seconds and memory consumption from 1.4 to 1 G.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  memory.c | 41 +++++++++++++++++++++++++++++++++++------
>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index 4952cc5d84..3207ae55e2 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -733,12 +733,41 @@ static void render_memory_region(FlatView *view,
>>  
>>  static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
>>  {
>> -    while (mr->alias && !mr->alias_offset &&
>> -           int128_ge(mr->size, mr->alias->size)) {
>> -        /* The alias is included in its entirety.  Use it as
>> -         * the "real" root, so that we can share more FlatViews.
>> -         */
>> -        mr = mr->alias;
>> +    while (mr->enabled) {
>> +        if (mr->alias && !mr->alias_offset &&
>> +            int128_ge(mr->size, mr->alias->size)) {
>> +            /* The alias is included in its entirety.  Use it as
>> +             * the "real" root, so that we can share more FlatViews.
>> +             */
>> +            mr = mr->alias;
>> +            continue;
>> +        }
>> +
>> +        if (!mr->terminates) {
> 
> Can an MR have terminates==true and children by the same time? If so, what for?

Yes, children override the parent.  But more important, !mr->terminates
means that patch 3 doesn't think that MR produces an empty FlatView.

> 
>> +            unsigned int found = 0;
>> +            MemoryRegion *child, *next = NULL;
>> +            QTAILQ_FOREACH(child, &mr->subregions, subregions_link) {
>> +                if (child->enabled) {
>> +                    if (++found > 1) {
>> +                        next = NULL;
>> +                        break;
>> +                    }
>> +                    if (!child->addr && int128_ge(mr->size, child->size)) {
> 
> 
> Ah, I tried this one but in v4's 18/18 (that hinting thing), did not work
> out but for a different reason.

Yeah, I did take some inspiration from your code, but we were thinking
of different scenarios (I wanted to cover the child->enabled == true).

Paolo

> 
>> +                        /* A child is included in its entirety.  If it's the only
>> +                         * enabled one, use it in the hope of finding an alias down the
>> +                         * way. This will also let us share FlatViews.
>> +                         */
>> +                        next = child;
>> +                    }
>> +                }
>> +            }
>> +            if (next) {
>> +                mr = next;
>> +                continue;
>> +            }
>> +        }
>> +
>> +        break;
>>      }
>>  
>>      return mr;
>>
> 
> 

Re: [Qemu-devel] [PATCH 2/3] memory: seek FlatView sharing candidates among children subregions

[Alexey Kardashevskiy]

On 21/09/17 23:57, Paolo Bonzini wrote:
> On 21/09/2017 15:39, Alexey Kardashevskiy wrote:
>> On 21/09/17 22:07, Paolo Bonzini wrote:
>>> A container can be used instead of an alias to allow switching between
>>> multiple subregions.  In this case we cannot directly share the
>>> subregions (since they only belong to a single parent), but if the
>>> subregions are aliases we can in turn walk those.
>>>
>>> While this does not reduce much the number of FlatViews that are created,
>>> it makes it possible to share the PCI bus master FlatViews and their
>>> AddressSpaceDispatch structures.  For 112 virtio-net-pci devices, boot time
>>> is reduced from 25 to 10 seconds and memory consumption from 1.4 to 1 G.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  memory.c | 41 +++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 4952cc5d84..3207ae55e2 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -733,12 +733,41 @@ static void render_memory_region(FlatView *view,
>>>  
>>>  static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
>>>  {
>>> -    while (mr->alias && !mr->alias_offset &&
>>> -           int128_ge(mr->size, mr->alias->size)) {
>>> -        /* The alias is included in its entirety.  Use it as
>>> -         * the "real" root, so that we can share more FlatViews.
>>> -         */
>>> -        mr = mr->alias;
>>> +    while (mr->enabled) {
>>> +        if (mr->alias && !mr->alias_offset &&
>>> +            int128_ge(mr->size, mr->alias->size)) {
>>> +            /* The alias is included in its entirety.  Use it as
>>> +             * the "real" root, so that we can share more FlatViews.
>>> +             */
>>> +            mr = mr->alias;
>>> +            continue;
>>> +        }
>>> +
>>> +        if (!mr->terminates) {
>>
>> Can an MR have terminates==true and children by the same time? If so, what for?
> 
> Yes, children override the parent. > But more important, !mr->terminates
> means that patch 3 doesn't think that MR produces an empty FlatView.


I see, after some debugging only MRs with @terminates==true appear in the
dispatch tree which makes sense. But I am still struggling to understand
what @terminates really means here in plain english.



-- 
Alexey

Re: [Qemu-devel] [PATCH 2/3] memory: seek FlatView sharing candidates among children subregions

[Paolo Bonzini]

On 22/09/2017 06:45, Alexey Kardashevskiy wrote:
> On 21/09/17 23:57, Paolo Bonzini wrote:
>> On 21/09/2017 15:39, Alexey Kardashevskiy wrote:
>>> On 21/09/17 22:07, Paolo Bonzini wrote:
>>>> A container can be used instead of an alias to allow switching between
>>>> multiple subregions.  In this case we cannot directly share the
>>>> subregions (since they only belong to a single parent), but if the
>>>> subregions are aliases we can in turn walk those.
>>>>
>>>> While this does not reduce much the number of FlatViews that are created,
>>>> it makes it possible to share the PCI bus master FlatViews and their
>>>> AddressSpaceDispatch structures.  For 112 virtio-net-pci devices, boot time
>>>> is reduced from 25 to 10 seconds and memory consumption from 1.4 to 1 G.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  memory.c | 41 +++++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 35 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/memory.c b/memory.c
>>>> index 4952cc5d84..3207ae55e2 100644
>>>> --- a/memory.c
>>>> +++ b/memory.c
>>>> @@ -733,12 +733,41 @@ static void render_memory_region(FlatView *view,
>>>>  
>>>>  static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
>>>>  {
>>>> -    while (mr->alias && !mr->alias_offset &&
>>>> -           int128_ge(mr->size, mr->alias->size)) {
>>>> -        /* The alias is included in its entirety.  Use it as
>>>> -         * the "real" root, so that we can share more FlatViews.
>>>> -         */
>>>> -        mr = mr->alias;
>>>> +    while (mr->enabled) {
>>>> +        if (mr->alias && !mr->alias_offset &&
>>>> +            int128_ge(mr->size, mr->alias->size)) {
>>>> +            /* The alias is included in its entirety.  Use it as
>>>> +             * the "real" root, so that we can share more FlatViews.
>>>> +             */
>>>> +            mr = mr->alias;
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        if (!mr->terminates) {
>>>
>>> Can an MR have terminates==true and children by the same time? If so, what for?
>>
>> Yes, children override the parent. > But more important, !mr->terminates
>> means that patch 3 doesn't think that MR produces an empty FlatView.
> 
> 
> I see, after some debugging only MRs with @terminates==true appear in the
> dispatch tree which makes sense. But I am still struggling to understand
> what @terminates really means here in plain english.

As you found out, perhaps "dispatched" would be a better name than
"terminates".  A MR can be "alias", "terminates" or "!terminates".
Aliases are resolved early.  If a range is allocated to a "terminates"
region A (and not to any other "terminates" region in A's subtree), it
dispatches to A.

If a range is allocated to a "!terminates" region B (and to any other
"terminates" region in B's subtree, QEMU doesn't add it to the FlatView
and it will be dispatched to another memory region: B's parent, another
region in B's parent's subtree, B's grandparent, another region in B's
grandparent's subtree, and so on---if nobody accepts it, it goes to
unassigned_mem_ops.

Paolo

[Qemu-devel] [PATCH 3/3] memory: Share special empty FlatView

[Paolo Bonzini]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

This shares an cached empty FlatView among address spaces. The empty
FV is used every time when a root MR would render into a FV without
memory sections, which happens when MR or its children are not enabled
(or zero-sized, which isn't caught yet). The empty_view is not NULL to
keep the rest of memory API intact; it also has a dispatch tree for the
same reason.

On POWER8 with 255 CPUs, 255 virtio-net, 40 PCI bridges guest this halves
the amount of FlatView's in use (557 -> 260) and dispatch tables
(~800000 -> ~370000).  In an unrelated experiment with 112 non-virtio
devices on x86 ("-M pc"), only 4 FlatViews are alive while the guest runs.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Message-Id: <20170921085110.25598-16-aik@ozlabs.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 3207ae55e2..cb369223b1 100644
--- a/memory.c
+++ b/memory.c
@@ -316,6 +316,7 @@ static void flatview_ref(FlatView *view)
 static void flatview_unref(FlatView *view)
 {
     if (atomic_fetch_dec(&view->ref) == 1) {
+        assert(view->root);
         call_rcu(view, flatview_destroy, rcu);
     }
 }
@@ -761,16 +762,19 @@ static MemoryRegion *memory_region_get_flatview_root(MemoryRegion *mr)
                     }
                 }
             }
+            if (found == 0) {
+                return NULL;
+            }
             if (next) {
                 mr = next;
                 continue;
             }
         }
 
-        break;
+        return mr;
     }
 
-    return mr;
+    return NULL;
 }
 
 /* Render a memory topology into a list of disjoint absolute ranges. */
@@ -962,12 +966,22 @@ static void address_space_update_topology_pass(AddressSpace *as,
 
 static void flatviews_init(void)
 {
+    static FlatView *empty_view;
+
     if (flat_views) {
         return;
     }
 
     flat_views = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
                                        (GDestroyNotify) flatview_unref);
+    if (!empty_view) {
+        empty_view = generate_memory_topology(NULL);
+        /* We keep it alive forever in the global variable.  */
+        flatview_ref(empty_view);
+    } else {
+        g_hash_table_replace(flat_views, NULL, empty_view);
+        flatview_ref(empty_view);
+    }
 }
 
 static void flatviews_reset(void)
-- 
2.13.5