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

[Paolo Bonzini <pbonzini@redhat.com>]

On 18/09/2017 12:17, Alexey Kardashevskiy wrote:
> +        physmr = memory_region_unalias_entire(old_view->root);
> +
> +        key = !physmr->enabled ? 0 : physmr;
> +        new_view = (FlatView *) g_hash_table_lookup(views, key);
> +        if (new_view) {
> +            continue;
> +        }
> +
> +        new_view = generate_memory_topology(physmr);
> +
> +        new_view->dispatch = address_space_dispatch_new(new_view);
> +        for (i = 0; i < new_view->nr; i++) {
> +            MemoryRegionSection mrs =
> +                section_from_flat_range(&new_view->ranges[i], new_view);
> +            flatview_add_to_dispatch(new_view, &mrs);
> +        }
> +        address_space_dispatch_compact(new_view->dispatch);
> +
> +        g_hash_table_insert(views, key, new_view);
> +    }
> +
> +    /* Replace FVs in ASes */
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        old_view = as->current_map;
> +        physmr = memory_region_unalias_entire(old_view->root);
> +
> +        key = !physmr->enabled ? 0 : physmr;

This is duplicate, perhaps memory_region_unalias_entire should do it
instead?  Does it make sense for flatview->root to be NULL, or does it
break something else?  If something breaks, disregard the remaining
comments.

(BTW, maybe you can rename the function to memory_region_get_flatview_root).

> 
> +
> +    /* Unref FVs from temporary table */
> +    g_hash_table_foreach_remove(views, flatview_unref_g_hash, 0);
> +    g_hash_table_unref(views);
>  }

You can avoid g_hash_table_foreach_remove and also flatview_unref_g_hash
instead use g_hash_table_new_full (casting flatview_unref to
GDestroyNotify should work fine).

> 
> @@ -2690,13 +2721,6 @@ AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
>  {
>      AddressSpace *as;
>  
> -    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> -        if (root == as->root && as->malloced) {
> -            as->ref_count++;
> -            return as;
> -        }
> -    }
> -
>      as = g_malloc0(sizeof *as);
>      address_space_init(as, root, name);
>      as->malloced = true;

Is this on purpose because it's now pointless?  Maybe it should be
pointed out in the commit message.

Paolo