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 
 ---
  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

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 
>>> ---
>>>  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 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 
>> ---
>>  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, >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 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 
> ---
>  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, >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

[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 
---
 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, >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