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

[Paolo Bonzini <pbonzini@redhat.com>]

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