Re: [Qemu-devel] [PATCH qemu v2 11/13] memory: Share FlatView's and dispatch trees between address spaces

[Paolo Bonzini <pbonzini@redhat.com>]

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