[14/23] memory: transaction API

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(-)

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

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.

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.