Re: [Qemu-devel] [PATCH qemu v3 01/13] memory: Postpone flatview and dispatch tree building till all devices are added

[Alexey Kardashevskiy]

On 19/09/17 17:40, Paolo Bonzini wrote:
> On 19/09/2017 08:57, Alexey Kardashevskiy wrote:
>>>> 3) you can consult the list (or hash table :)) of live FlatViews (which
>>>> means you keep it live after memory_region_transaction_commit ends, and
>>>> only clear it on the next call), and reuse an existing FlatView.  Note
>>>> that the number of distinct FlatViews should be very few, 
>>>
>>> I keep missing this bit - why few? Each virtio-pci device creates 2 AS,
>>> with proxy->modern_bar and pci_dev->bus_master_container_region which are
>>> unique and not aliases. Remember, 500 virtio devices is my test case ;)
> 
> Argh, no, it's me who keeps missing the bit.
> 
>> More details: pci_dev->bus_master_container_region is a root and it is
>> enabled but its only child pci_dev->bus_master_enable_region is not. Ok, in
>> flatview_topology_update() I can render a FV, see that it is empty
>> (view->nr==0) and share an empty FV in this case too, this halves the
>> number of FVs (from ~1000 to ~500 for 500 virtio devices).
> 
> We can also resolve "fake" aliases, obtained through a region with only
> one subregion.

I just do this now (will post soon):


empty_view = generate_memory_topology(NULL);
[...]

new_view = generate_memory_topology(physmr);

if (!new_view->nr) {
    flatview_ref(empty_view);
    flatview_unref(new_view);
    new_view = empty_view;

[...]

g_hash_table_insert(views, physmr, new_view);

and that's it. For non empty FVs I need to render FVs anyway and for
all-disabled/empty FVs it is really cheap so no big loss here.


> 
>> But proxy->modern_bar (which has an modern_cfg alias which is a root of an
>> AS) is enabled since it is created and I could disable it and enable
>> afterwards but since a PCI device enablement is done by writing to the
>> config space, I kind of stuck here.
>>
>> I can do something like this and it helps a lot (now with -S I end up
>> having 4 FVs and much better start time) but it is kinda hacky and "memory:
>> Postpone flatview and dispatch tree building till all devices are added"
>> solves this better imho, no?
> 
> I think modern_bar (actually modern_cfg) shouldn't have its own address
> space, it's a pretty wasteful way to compute the dispatch tree where a
> simple linked list or array would be enough:

Why would we need an array there anyway? This AS is only used for
virtio_address_space_write() and virtio_address_space_read() so we only
need here s/virtio_address_space_write/memory_region_dispatch_write/ (and
the same for reading), no?

>
> memory-region: virtio-pci
>   00000000fe000000-00000000fe003fff (prio 1, i/o): virtio-pci
>     00000000fe000000-00000000fe000fff (prio 0, i/o): virtio-pci-common
>     00000000fe001000-00000000fe001fff (prio 0, i/o): virtio-pci-isr
>     00000000fe002000-00000000fe002fff (prio 0, i/o): virtio-pci-device
>     00000000fe003000-00000000fe003fff (prio 0, i/o): virtio-pci-notify





-- 
Alexey