[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qemu v2 11/13] memory: Share FlatView's and disp
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-devel] [PATCH qemu v2 11/13] memory: Share FlatView's and dispatch trees between address spaces |
Date: |
Sat, 16 Sep 2017 10:58:40 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
On 15/09/17 19:25, Paolo Bonzini wrote:
> On 15/09/2017 10:40, Alexey Kardashevskiy wrote:
>> +
>> +static bool flatview_can_share(FlatView *old_view, FlatView *new_view)
>> +{
>> + MemoryRegion *old_root = memory_region_unalias_entire(old_view->root);
>> + MemoryRegion *new_root = memory_region_unalias_entire(new_view->root);
>> +
>> + if (old_root == new_root) {
>> + return true;
>> + }
>> +
>> + if (!old_root->enabled && !new_root->enabled) {
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>
> I think you can improve this by keeping the root in the address space
> (removing the previous patch). Instead, the FlatView's root can be the
> one with resolved aliases. Then old_root is just old_view->root, and if
> an empty FlatView has a NULL root this just becomes
>
> the_other_view->root == memory_region_unalias_entire(as->root);
Ok!
>
>>
>> +
>> + /* Build list of unique FlatViews, FV::root is the key */
>> + QTAILQ_FOREACH(old_view, &flat_views, flat_views_link) {
>> + found = false;
>> + QTAILQ_FOREACH(new_view, &fvs_tmp, flat_views_link) {
>> + if (flatview_can_share(old_view, new_view)) {
>> + found = true;
>> + break;
>> + }
>> + }
>> + if (found) {
>> + continue;
>> + }
>> +
>> + new_view = generate_memory_topology(old_view->root);
>> + flatview_render_new(old_view, new_view);
>> + QTAILQ_INSERT_TAIL(&fvs_tmp, new_view, flat_views_link);
>> + }
>
> I don't understand the algorithm here. Why is it visiting &flat_views
> rather than the list of address spaces? I would have thought it enough
> to do
>
> for each address space
> if there is a (new) flat view that we can share
> add a reference
> else
> render a new flat view and add it to fvs_tmp
>
> flat_views = fvs_tmp;
>
> for each address space
> old_view = address_space_to_flatview(as);
> find the new flat view to use in fvs_tmp
> address_space_update_topology_pass(..., false);
> address_space_update_topology_pass(..., true);
> QTAILQ_REMOVE(&old_view->address_spaces, ...);
> atomic_rcu_set(&as->current_map, new_view);
> flatview_unref(old_view);
> QTAILQ_INSERT_TAIL(&new_view->address_spaces, ...);
>
>
> It's also not clear to me what you need the FlatView's address_spaces
> list for. (It's probably something trivial that I'm missing, or maybe
> it goes away with the previous change).
It is trivial - the first version added a global list of FVs and shared
them among ASes. Every transactoion_commit would produce a new FV and
replace it in all ASes which old FV was shared among. The decision whether
to share FV or not was made in address_space_init() which a very different
place in code.
In v2 I moved the sharing decision to the commit part and noooow having a
global list of FVs and ASes list in each FV seems redundant so I'll remove
it in v3, thanks for pointing out :)
And I'll probably drop FV allocation in address_space_init as it is going
to be rebuild at commit time anyway.
>
>>
>> AddressSpace *address_space_init_shareable(MemoryRegion *root, const char
>> *name)
>> {
>> AddressSpace *as;
>>
>> - QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>> - if (root == address_space_root(as) && as->malloced) {
>> - as->ref_count++;
>> - return as;
>> - }
>> - }
>> -
>> as = g_malloc0(sizeof *as);
>> address_space_init(as, root, name);
>> - as->malloced = true;
>> +
>> return as;
>> }
>>
>
> This belongs in the next patch;
By "this" you mean removal of "malloced", not the AS loop above, right?
> leaving as->malloced in
> do_address_space_destroy and the reference count in
> address_space_destroy is not a big complication.
But why would we want to leave them anyway?
thanks for the quick review!
--
Alexey
- [Qemu-devel] [PATCH qemu v2 04/13] memory: Move FlatView allocation to a helper, (continued)
- [Qemu-devel] [PATCH qemu v2 04/13] memory: Move FlatView allocation to a helper, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 06/13] memory: Remove AddressSpace pointer from AddressSpaceDispatch, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 08/13] memory: Cleanup after switching to FlatView, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 09/13] memory: Rename mem_begin/mem_commit/mem_add helpers, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 10/13] memory: Move root MR from AddressSpace to FlatView, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 12/13] memory: Get rid of address_space_init_shareable, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 11/13] memory: Share FlatView's and dispatch trees between address spaces, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 07/13] memory: Switch memory from using AddressSpace to FlatView, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 13/13] memory: Add flat views to HMP "info mtree", Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 02/13] exec: Explicitely export target AS from address_space_translate_internal, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 01/13] memory: Postpone flatview and dispatch tree building till all devices are added, Alexey Kardashevskiy, 2017/09/15
- [Qemu-devel] [PATCH qemu v2 05/13] memory: Move AddressSpaceDispatch from AddressSpace to FlatView, Alexey Kardashevskiy, 2017/09/15
- Re: [Qemu-devel] [PATCH qemu v2 00/13] memory: Reduce memory use, no-reply, 2017/09/15