Message ID | 1311602584-23409-15-git-send-email-avi@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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(-)