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

[Alexey Kardashevskiy]

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, _views, flat_views_link) {
>> +found = false;
>> +QTAILQ_FOREACH(new_view, _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(_tmp, new_view, flat_views_link);
>> +}
> 
> I don't understand the algorithm here.  Why is it visiting _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(_view->address_spaces, ...);
> atomic_rcu_set(>current_map, new_view);
> flatview_unref(old_view);
> QTAILQ_INSERT_TAIL(_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 nw 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, _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

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

[Paolo Bonzini]

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, _views, flat_views_link) {
> +found = false;
> +QTAILQ_FOREACH(new_view, _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(_tmp, new_view, flat_views_link);
> +}

I don't understand the algorithm here.  Why is it visiting _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(_view->address_spaces, ...);
atomic_rcu_set(>current_map, new_view);
flatview_unref(old_view);
QTAILQ_INSERT_TAIL(_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, _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

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

[Alexey Kardashevskiy]

This allows sharing flat views between address spaces when the same root
memory region is used when creating a new address space.

This adds a global list of flat views and a list of attached address
spaces per a flat view. Each address space references a flat view.

Signed-off-by: Alexey Kardashevskiy 
---
 include/exec/memory.h |   3 +-
 memory.c  | 148 --
 2 files changed, 108 insertions(+), 43 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index be8cc1ccd3..04993f8eca 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -308,8 +308,6 @@ struct AddressSpace {
 /* All fields are private. */
 struct rcu_head rcu;
 char *name;
-int ref_count;
-bool malloced;
 
 /* Accessed via RCU.  */
 struct FlatView *current_map;
@@ -318,6 +316,7 @@ struct AddressSpace {
 struct MemoryRegionIoeventfd *ioeventfds;
 QTAILQ_HEAD(memory_listeners_as, MemoryListener) listeners;
 QTAILQ_ENTRY(AddressSpace) address_spaces_link;
+QTAILQ_ENTRY(AddressSpace) flat_view_link;
 };
 
 FlatView *address_space_to_flatview(AddressSpace *as);
diff --git a/memory.c b/memory.c
index 29f8588945..f3da9379df 100644
--- a/memory.c
+++ b/memory.c
@@ -47,6 +47,9 @@ static QTAILQ_HEAD(memory_listeners, MemoryListener) 
memory_listeners
 static QTAILQ_HEAD(, AddressSpace) address_spaces
 = QTAILQ_HEAD_INITIALIZER(address_spaces);
 
+static QTAILQ_HEAD(FlatViewList, FlatView) flat_views
+= QTAILQ_HEAD_INITIALIZER(flat_views);
+
 typedef struct AddrRange AddrRange;
 
 /*
@@ -232,6 +235,8 @@ struct FlatView {
 unsigned nr_allocated;
 struct AddressSpaceDispatch *dispatch;
 MemoryRegion *root;
+QTAILQ_ENTRY(FlatView) flat_views_link;
+QTAILQ_HEAD(address_spaces, AddressSpace) address_spaces;
 };
 
 typedef struct AddressSpaceOps AddressSpaceOps;
@@ -269,6 +274,7 @@ static FlatView *flatview_alloc(MemoryRegion *mr_root)
 view->ref = 1;
 view->root = mr_root;
 memory_region_ref(mr_root);
+QTAILQ_INIT(>address_spaces);
 
 return view;
 }
@@ -953,28 +959,99 @@ static void flatview_render_new(FlatView *old_view, 
FlatView *new_view)
 address_space_dispatch_compact(new_view->dispatch);
 }
 
-static void address_space_update_topology(AddressSpace *as)
+static MemoryRegion *memory_region_unalias_entire(MemoryRegion *mr)
 {
-FlatView *old_view = address_space_get_flatview(as);
-FlatView *new_view = generate_memory_topology(old_view->root);
-
-flatview_render_new(old_view, new_view);
-address_space_update_topology_pass(as, old_view, new_view, false);
-address_space_update_topology_pass(as, old_view, new_view, true);
-
-/* Writes are protected by the BQL.  */
-atomic_rcu_set(>current_map, new_view);
-call_rcu(old_view, flatview_unref, rcu);
-
-/* Note that all the old MemoryRegions are still alive up to this
- * point.  This relieves most MemoryListeners from the need to
- * ref/unref the MemoryRegions they get---unless they use them
- * outside the iothread mutex, in which case precise reference
- * counting is necessary.
- */
-flatview_unref(old_view);
-
-address_space_update_ioeventfds(as);
+while (mr->alias && !mr->alias_offset &&
+   int128_ge(mr->size, mr->alias->size)) {
+/* The alias is included in its entirety.  Use it as
+ * the "real" root, so that we can share more FlatViews.
+ */
+mr = mr->alias;
+}
+
+return mr;
+}
+
+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;
+}
+
+static void flatview_update_topology(void)
+{
+AddressSpace *as, *asnext;
+FlatView *old_view, *new_view, *vnext;
+struct FlatViewList fvs_tmp = QTAILQ_HEAD_INITIALIZER(fvs_tmp);
+bool found;
+
+/* Build list of unique FlatViews, FV::root is the key */
+QTAILQ_FOREACH(old_view, _views, flat_views_link) {
+found = false;
+QTAILQ_FOREACH(new_view, _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(_tmp, new_view, flat_views_link);
+}
+
+/* Replace old FVs with new ones */
+QTAILQ_FOREACH_SAFE(old_view, _views, flat_views_link, vnext) {
+flatview_ref(old_view);
+
+found = false;
+QTAILQ_FOREACH(new_view, _tmp, flat_views_link) {
+