Re: [PATCH 3/3] COLO: Optimize memory back-up process

[Dr. David Alan Gilbert]

* Hailiang Zhang (address@hidden) wrote:
> This patch will reduce the downtime of VM for the initial process,
> Privously, we copied all these memory in preparing stage of COLO
> while we need to stop VM, which is a time-consuming process.
> Here we optimize it by a trick, back-up every page while in migration
> process while COLO is enabled, though it affects the speed of the
> migration, but it obviously reduce the downtime of back-up all SVM'S
> memory in COLO preparing stage.
> 
> Signed-off-by: Hailiang Zhang <address@hidden>

OK, I think this is right, but it took me quite a while to understand,
I think one of the comments below might not be right:

> ---
>  migration/colo.c |  3 +++
>  migration/ram.c  | 35 +++++++++++++++++++++++++++--------
>  migration/ram.h  |  1 +
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index d30c6bc4ad..febf010571 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -26,6 +26,7 @@
>  #include "qemu/main-loop.h"
>  #include "qemu/rcu.h"
>  #include "migration/failover.h"
> +#include "migration/ram.h"
>  #ifdef CONFIG_REPLICATION
>  #include "replication.h"
>  #endif
> @@ -906,6 +907,8 @@ void *colo_process_incoming_thread(void *opaque)
>       */
>      qemu_file_set_blocking(mis->from_src_file, true);
>  
> +    colo_incoming_start_dirty_log();
> +
>      bioc = qio_channel_buffer_new(COLO_BUFFER_BASE_SIZE);
>      fb = qemu_fopen_channel_input(QIO_CHANNEL(bioc));
>      object_unref(OBJECT(bioc));
> diff --git a/migration/ram.c b/migration/ram.c
> index ed23ed1c7c..24a8aa3527 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2986,7 +2986,6 @@ int colo_init_ram_cache(void)
>                  }
>                  return -errno;
>              }
> -            memcpy(block->colo_cache, block->host, block->used_length);
>          }
>      }
>  
> @@ -3005,12 +3004,16 @@ int colo_init_ram_cache(void)
>              bitmap_set(block->bmap, 0, pages);
>          }
>      }
> +
> +    return 0;
> +}
> +
> +void colo_incoming_start_dirty_log(void)
> +{
>      ram_state = g_new0(RAMState, 1);
>      ram_state->migration_dirty_pages = 0;
>      qemu_mutex_init(&ram_state->bitmap_mutex);
>      memory_global_dirty_log_start();
> -
> -    return 0;
>  }
>  
>  /* It is need to hold the global lock to call this helper */
> @@ -3348,7 +3351,7 @@ static int ram_load_precopy(QEMUFile *f)
>  
>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
>          ram_addr_t addr, total_ram_bytes;
> -        void *host = NULL;
> +        void *host = NULL, *host_bak = NULL;
>          uint8_t ch;
>  
>          /*
> @@ -3378,13 +3381,26 @@ static int ram_load_precopy(QEMUFile *f)
>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>              RAMBlock *block = ram_block_from_stream(f, flags);
> -
>              /*
> -             * After going into COLO, we should load the Page into 
> colo_cache.
> +             * After going into COLO, we should load the Page into colo_cache
> +             * NOTE: We need to keep a copy of SVM's ram in colo_cache.
> +             * Privously, we copied all these memory in preparing stage of 
> COLO
> +             * while we need to stop VM, which is a time-consuming process.
> +             * Here we optimize it by a trick, back-up every page while in
> +             * migration process while COLO is enabled, though it affects the
> +             * speed of the migration, but it obviously reduce the downtime 
> of
> +             * back-up all SVM'S memory in COLO preparing stage.
>               */
> -            if (migration_incoming_in_colo_state()) {
> +            if (migration_incoming_colo_enabled()) {
>                  host = colo_cache_from_block_offset(block, addr);
> -            } else {
> +                /*
> +                 * After going into COLO, load the Page into colo_cache.
> +                 */
> +                if (!migration_incoming_in_colo_state()) {
> +                    host_bak = host;
> +                }
> +            }
> +            if (!migration_incoming_in_colo_state()) {
>                  host = host_from_ram_block_offset(block, addr);

So this works out as quite complicated:
   a) In normal migration we do the last one and just set:
         host = host_from_ram_block_offset(block, addr);
         host_bak = NULL

   b) At the start, when colo_enabled, but !in_colo_state
         host = colo_cache
         host_bak = host
         host = host_from_ram_block_offset

   c) in_colo_state
         host = colo_cache
         host_bak = NULL


(b) is pretty confusing, setting host twice; can't we tidy that up?

Also, that last comment 'After going into COLO' I think is really
  'Before COLO state, copy from ram into cache'

Dave

>              }
>              if (!host) {
> @@ -3506,6 +3522,9 @@ static int ram_load_precopy(QEMUFile *f)
>          if (!ret) {
>              ret = qemu_file_get_error(f);
>          }
> +        if (!ret && host_bak && host) {
> +            memcpy(host_bak, host, TARGET_PAGE_SIZE);
> +        }
>      }
>  
>      ret |= wait_for_decompress_done();
> diff --git a/migration/ram.h b/migration/ram.h
> index a553d40751..5ceaff7cb4 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -66,5 +66,6 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock 
> *rb);
>  /* ram cache */
>  int colo_init_ram_cache(void);
>  void colo_release_ram_cache(void);
> +void colo_incoming_start_dirty_log(void);
>  
>  #endif
> -- 
> 2.21.0
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK