| Message ID | 20170921120751.3027-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | FlatView memory savings | expand |
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; >
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; >> > >
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.
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
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;
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(-)