Patchwork [02/27] split MRU ram list

login
register
mail settings
Submitter Juan Quintela
Date July 24, 2012, 6:36 p.m.
Message ID <1343155012-26316-3-git-send-email-quintela@redhat.com>
Download mbox | patch
Permalink /patch/173007/
State New
Headers show

Comments

Juan Quintela - July 24, 2012, 6:36 p.m.
From: Paolo Bonzini <pbonzini@redhat.com>

Outside the execution threads the normal, non-MRU-ized order of
the RAM blocks should always be enough.  So manage two separate
lists, which will have separate locking rules.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 cpu-all.h |    4 +++-
 exec.c    |   16 +++++++++++-----
 2 files changed, 14 insertions(+), 6 deletions(-)
Michael Roth - July 25, 2012, 8:20 p.m.
On Tue, Jul 24, 2012 at 08:36:27PM +0200, Juan Quintela wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Outside the execution threads the normal, non-MRU-ized order of
> the RAM blocks should always be enough.  So manage two separate
> lists, which will have separate locking rules.

One thing I'm noticing is that, prior to this series, we're traversing the
blocks in MRU order for migration. This seems counter-intuitive, as those are
the blocks most likely to get re-dirtied and re-sent, so it make sense to hold
off on sending those till last to reduce the amount of time the running guest
has to invalidate the target's copy of it.

This isn't as bad as it could be, since we at least don't restart the
loop on every iteration, but it might still make sense to come up with a way
to keep RAMList.blocks roughly in sync with RAMList.blocks_mru, and then
traverse that in reverse order for ram_save_iterate. The fact that we're
switching from the MRU ordering in the current version might be
obscuring performance issues as well, which is probably worth keeping in
mind.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  cpu-all.h |    4 +++-
>  exec.c    |   16 +++++++++++-----
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 82ba1d7..ca3bb24 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -476,8 +476,9 @@ typedef struct RAMBlock {
>      ram_addr_t offset;
>      ram_addr_t length;
>      uint32_t flags;
> -    char idstr[256];
> +    QLIST_ENTRY(RAMBlock) next_mru;
>      QLIST_ENTRY(RAMBlock) next;
> +    char idstr[256];
>  #if defined(__linux__) && !defined(TARGET_S390X)
>      int fd;
>  #endif
> @@ -485,6 +486,7 @@ typedef struct RAMBlock {
> 
>  typedef struct RAMList {
>      uint8_t *phys_dirty;
> +    QLIST_HEAD(, RAMBlock) blocks_mru;
>      QLIST_HEAD(, RAMBlock) blocks;
>      uint64_t dirty_pages;
>  } RAMList;
> diff --git a/exec.c b/exec.c
> index feb4795..afc472f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -112,7 +112,10 @@ static uint8_t *code_gen_ptr;
>  int phys_ram_fd;
>  static int in_migration;
> 
> -RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
> +RAMList ram_list = {
> +    .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks),
> +    .blocks_mru = QLIST_HEAD_INITIALIZER(ram_list.blocks_mru)
> +};
> 
>  static MemoryRegion *system_memory;
>  static MemoryRegion *system_io;
> @@ -2550,6 +2553,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>      new_block->length = size;
> 
>      QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
> +    QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);
> 
>      ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>                                         last_ram_offset() >> TARGET_PAGE_BITS);
> @@ -2573,6 +2577,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
> +            QLIST_REMOVE(block, next_mru);
>              g_free(block);
>              return;
>          }
> @@ -2586,6 +2591,7 @@ void qemu_ram_free(ram_addr_t addr)
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
> +            QLIST_REMOVE(block, next_mru);
>              if (block->flags & RAM_PREALLOC_MASK) {
>                  ;
>              } else if (mem_path) {
> @@ -2690,12 +2696,12 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>  {
>      RAMBlock *block;
> 
> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
> +    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
>          if (addr - block->offset < block->length) {
>              /* Move this entry to to start of the list.  */
>              if (block != QLIST_FIRST(&ram_list.blocks)) {
> -                QLIST_REMOVE(block, next);
> -                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +                QLIST_REMOVE(block, next_mru);
> +                QLIST_INSERT_HEAD(&ram_list.blocks_mru, block, next_mru);
>              }
>              if (xen_enabled()) {
>                  /* We need to check if the requested address is in the RAM
> @@ -2790,7 +2796,7 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
>          return 0;
>      }
> 
> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
> +    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
>          /* This case append when the block is not mapped. */
>          if (block->host == NULL) {
>              continue;
> -- 
> 1.7.10.4
> 
>
Avi Kivity - July 26, 2012, 1:19 p.m.
On 07/25/2012 11:20 PM, Michael Roth wrote:
> On Tue, Jul 24, 2012 at 08:36:27PM +0200, Juan Quintela wrote:
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> Outside the execution threads the normal, non-MRU-ized order of
>> the RAM blocks should always be enough.  So manage two separate
>> lists, which will have separate locking rules.
> 
> One thing I'm noticing is that, prior to this series, we're traversing the
> blocks in MRU order for migration. This seems counter-intuitive, as those are
> the blocks most likely to get re-dirtied and re-sent, so it make sense to hold
> off on sending those till last to reduce the amount of time the running guest
> has to invalidate the target's copy of it.
> 
> This isn't as bad as it could be, since we at least don't restart the
> loop on every iteration, but it might still make sense to come up with a way
> to keep RAMList.blocks roughly in sync with RAMList.blocks_mru, and then
> traverse that in reverse order for ram_save_iterate. The fact that we're
> switching from the MRU ordering in the current version might be
> obscuring performance issues as well, which is probably worth keeping in
> mind.
> 

Main memory is the only ram block which matters (the framebuffer a
remote second).  The others are ROMs.

Patch

diff --git a/cpu-all.h b/cpu-all.h
index 82ba1d7..ca3bb24 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -476,8 +476,9 @@  typedef struct RAMBlock {
     ram_addr_t offset;
     ram_addr_t length;
     uint32_t flags;
-    char idstr[256];
+    QLIST_ENTRY(RAMBlock) next_mru;
     QLIST_ENTRY(RAMBlock) next;
+    char idstr[256];
 #if defined(__linux__) && !defined(TARGET_S390X)
     int fd;
 #endif
@@ -485,6 +486,7 @@  typedef struct RAMBlock {

 typedef struct RAMList {
     uint8_t *phys_dirty;
+    QLIST_HEAD(, RAMBlock) blocks_mru;
     QLIST_HEAD(, RAMBlock) blocks;
     uint64_t dirty_pages;
 } RAMList;
diff --git a/exec.c b/exec.c
index feb4795..afc472f 100644
--- a/exec.c
+++ b/exec.c
@@ -112,7 +112,10 @@  static uint8_t *code_gen_ptr;
 int phys_ram_fd;
 static int in_migration;

-RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
+RAMList ram_list = {
+    .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks),
+    .blocks_mru = QLIST_HEAD_INITIALIZER(ram_list.blocks_mru)
+};

 static MemoryRegion *system_memory;
 static MemoryRegion *system_io;
@@ -2550,6 +2553,7 @@  ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
     new_block->length = size;

     QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
+    QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);

     ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);
@@ -2573,6 +2577,7 @@  void qemu_ram_free_from_ptr(ram_addr_t addr)
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
+            QLIST_REMOVE(block, next_mru);
             g_free(block);
             return;
         }
@@ -2586,6 +2591,7 @@  void qemu_ram_free(ram_addr_t addr)
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
+            QLIST_REMOVE(block, next_mru);
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
             } else if (mem_path) {
@@ -2690,12 +2696,12 @@  void *qemu_get_ram_ptr(ram_addr_t addr)
 {
     RAMBlock *block;

-    QLIST_FOREACH(block, &ram_list.blocks, next) {
+    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
         if (addr - block->offset < block->length) {
             /* Move this entry to to start of the list.  */
             if (block != QLIST_FIRST(&ram_list.blocks)) {
-                QLIST_REMOVE(block, next);
-                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
+                QLIST_REMOVE(block, next_mru);
+                QLIST_INSERT_HEAD(&ram_list.blocks_mru, block, next_mru);
             }
             if (xen_enabled()) {
                 /* We need to check if the requested address is in the RAM
@@ -2790,7 +2796,7 @@  int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
         return 0;
     }

-    QLIST_FOREACH(block, &ram_list.blocks, next) {
+    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
         /* This case append when the block is not mapped. */
         if (block->host == NULL) {
             continue;