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);
>
> +
> + /* 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).
>
> 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; leaving as->malloced in
do_address_space_destroy and the reference count in
address_space_destroy is not a big complication.
Paolo
