Patchwork [14/23] memory: transaction API

login
register
mail settings
Submitter Avi Kivity
Date July 25, 2011, 2:02 p.m.
Message ID <1311602584-23409-15-git-send-email-avi@redhat.com>
Download mbox | patch
Permalink /patch/106675/
State New
Headers show

Comments

Avi Kivity - July 25, 2011, 2:02 p.m.
Allow changes to the memory hierarchy to be accumulated and
made visible all at once.  This reduces computational effort,
especially when an accelerator (e.g. kvm) is involved.

Useful when a single register update causes multiple changes
to an address space.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 memory.c |   20 ++++++++++++++++++++
 memory.h |    8 ++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)
Anthony Liguori - July 25, 2011, 7:16 p.m.
On 07/25/2011 09:02 AM, Avi Kivity wrote:
> Allow changes to the memory hierarchy to be accumulated and
> made visible all at once.  This reduces computational effort,
> especially when an accelerator (e.g. kvm) is involved.
>
> Useful when a single register update causes multiple changes
> to an address space.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>

What's the motivation for this?  Was this just because it seemed neat to 
do or did you run into a performance issue you were trying to solve?


> ---
>   memory.c |   20 ++++++++++++++++++++
>   memory.h |    8 ++++++++
>   2 files changed, 28 insertions(+), 0 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 009ad33..370e189 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -18,6 +18,8 @@
>   #include "kvm.h"
>   #include<assert.h>
>
> +unsigned memory_region_transaction_depth = 0;

Shouldn't this be static?

Regards,

Anthony Liguori

> +
>   typedef struct AddrRange AddrRange;
>
>   struct AddrRange {
> @@ -623,6 +625,10 @@ static void address_space_update_topology(AddressSpace *as)
>
>   static void memory_region_update_topology(void)
>   {
> +    if (memory_region_transaction_depth) {
> +        return;
> +    }
> +
>       if (address_space_memory.root) {
>           address_space_update_topology(&address_space_memory);
>       }
> @@ -631,6 +637,20 @@ static void memory_region_update_topology(void)
>       }
>   }
>
> +void memory_region_transaction_begin(void)
> +{
> +    ++memory_region_transaction_depth;
> +}
> +
> +void memory_region_transaction_commit(void)
> +{
> +    if (!memory_region_transaction_depth) {
> +        abort();
> +    }
> +    --memory_region_transaction_depth;
> +    memory_region_update_topology();
> +}
> +
>   void memory_region_init(MemoryRegion *mr,
>                           const char *name,
>                           uint64_t size)
> diff --git a/memory.h b/memory.h
> index e4c0ad1..cb3a9b6 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -246,6 +246,14 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>   void memory_region_del_subregion(MemoryRegion *mr,
>                                    MemoryRegion *subregion);
>
> +/* Start a transaction; changes will be accumulated and made visible only
> + * when the transaction ends.
> + */
> +void memory_region_transaction_begin(void);
> +/* Commit a transaction and make changes visible to the guest.
> + */
> +void memory_region_transaction_commit(void);
> +
>   #endif
>
>   #endif
Avi Kivity - July 26, 2011, 10:48 a.m.
On 07/25/2011 10:16 PM, Anthony Liguori wrote:
> On 07/25/2011 09:02 AM, Avi Kivity wrote:
>> Allow changes to the memory hierarchy to be accumulated and
>> made visible all at once.  This reduces computational effort,
>> especially when an accelerator (e.g. kvm) is involved.
>>
>> Useful when a single register update causes multiple changes
>> to an address space.
>>
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>
> What's the motivation for this?  Was this just because it seemed neat 
> to do or did you run into a performance issue you were trying to solve?

Both cirrus and 440fx need this; look at i440fx_update_memory_mappings() 
for example, it blindly updates mappings even for PAMs which haven't 
changed.

These issues can be also solved by more care on the caller's side, or by 
making the API richer, but it's good to have a no-thought-required 
solution, particularly as it's so easy to do.
Avi Kivity - July 26, 2011, 11:39 a.m.
On 07/26/2011 01:48 PM, Avi Kivity wrote:
> On 07/25/2011 10:16 PM, Anthony Liguori wrote:
>> On 07/25/2011 09:02 AM, Avi Kivity wrote:
>>> Allow changes to the memory hierarchy to be accumulated and
>>> made visible all at once.  This reduces computational effort,
>>> especially when an accelerator (e.g. kvm) is involved.
>>>
>>> Useful when a single register update causes multiple changes
>>> to an address space.
>>>
>>> Signed-off-by: Avi Kivity<avi@redhat.com>
>>
>> What's the motivation for this?  Was this just because it seemed neat 
>> to do or did you run into a performance issue you were trying to solve?
>
> Both cirrus and 440fx need this; look at 
> i440fx_update_memory_mappings() for example, it blindly updates 
> mappings even for PAMs which haven't changed.
>
> These issues can be also solved by more care on the caller's side, or 
> by making the API richer, but it's good to have a no-thought-required 
> solution, particularly as it's so easy to do.
>

I should note that updating the memory map is relatively slow with kvm 
due to the need to wait for an rcu grace period; though recent kernels 
are faster.  So any saving here has a large effect.

Patch

diff --git a/memory.c b/memory.c
index 009ad33..370e189 100644
--- a/memory.c
+++ b/memory.c
@@ -18,6 +18,8 @@ 
 #include "kvm.h"
 #include <assert.h>
 
+unsigned memory_region_transaction_depth = 0;
+
 typedef struct AddrRange AddrRange;
 
 struct AddrRange {
@@ -623,6 +625,10 @@  static void address_space_update_topology(AddressSpace *as)
 
 static void memory_region_update_topology(void)
 {
+    if (memory_region_transaction_depth) {
+        return;
+    }
+
     if (address_space_memory.root) {
         address_space_update_topology(&address_space_memory);
     }
@@ -631,6 +637,20 @@  static void memory_region_update_topology(void)
     }
 }
 
+void memory_region_transaction_begin(void)
+{
+    ++memory_region_transaction_depth;
+}
+
+void memory_region_transaction_commit(void)
+{
+    if (!memory_region_transaction_depth) {
+        abort();
+    }
+    --memory_region_transaction_depth;
+    memory_region_update_topology();
+}
+
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)
diff --git a/memory.h b/memory.h
index e4c0ad1..cb3a9b6 100644
--- a/memory.h
+++ b/memory.h
@@ -246,6 +246,14 @@  void memory_region_add_subregion_overlap(MemoryRegion *mr,
 void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion);
 
+/* Start a transaction; changes will be accumulated and made visible only
+ * when the transaction ends.
+ */
+void memory_region_transaction_begin(void);
+/* Commit a transaction and make changes visible to the guest.
+ */
+void memory_region_transaction_commit(void);
+
 #endif
 
 #endif