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