Re: [Qemu-devel] [PATCH V6 07/10] migration: add bitmap for copied page

[Peter Xu]

On Tue, May 23, 2017 at 02:31:08PM +0300, Alexey Perevalov wrote:
> This patch adds ability to track down already copied
> pages, it's necessary for calculation vCPU block time in
> postcopy migration feature and maybe for restore after
> postcopy migration failure.
> 
> Functions which work with RAMBlock are placed into ram.c,
> due to TARGET_WORDS_BIGENDIAN is poisoned int postcopy-ram.c -
> hardware independed code.
> 
> Signed-off-by: Alexey Perevalov <address@hidden>
> ---
>  include/migration/migration.h | 16 +++++++++++
>  migration/postcopy-ram.c      | 22 ++++++++++++---
>  migration/ram.c               | 63 
> +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 97 insertions(+), 4 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 449cb07..4e05c83 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -101,6 +101,20 @@ struct MigrationIncomingState {
>      LoadStateEntry_Head loadvm_handlers;
>  
>      /*
> +     * bitmap indicates whether page copied,
> +     * based on ramblock offset
> +     * now it is using only for blocktime calculation in
> +     * postcopy migration, so livetime of this entry:
> +     * since user requested blocktime calculation,
> +     * till the end of postcopy migration
> +     * as an example it could represend following memory map
> +     * ___________________________________
> +     * |4k pages | hugepages | 4k pages

Can we really do this?

The problem is AFAIU migration stream is sending pages only in target
page size, even for huge pages... so even for huge pages we should
still need per TARGET_PAGE_SIZE bitmap?

(Please refer to ram_state_init() on init of RAMBlock.bmap)

> +     *
> +     * */
> +    unsigned long *copied_pages;
> +
> +    /*
>       * PostcopyBlocktimeContext to keep information for postcopy
>       * live migration, to calculate vCPU block time
>       * */
> @@ -279,6 +293,8 @@ int migrate_compress_threads(void);
>  int migrate_decompress_threads(void);
>  bool migrate_use_events(void);
>  bool migrate_postcopy_blocktime(void);
> +unsigned long int get_copied_bit_offset(ram_addr_t addr);
> +unsigned long int *copied_pages_bitmap_new(void);
>  
>  /* Sending on the return path - generic and then for each message type */
>  void migrate_send_rp_message(MigrationIncomingState *mis,
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 5435a40..d647769 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -94,23 +94,34 @@ static void destroy_blocktime_context(struct 
> PostcopyBlocktimeContext *ctx)
>  
>  static void postcopy_migration_cb(Notifier *n, void *data)
>  {
> -    PostcopyBlocktimeContext *ctx = container_of(n, PostcopyBlocktimeContext,
> -                                               postcopy_notifier);
>      MigrationState *s = data;
>      if (migration_has_finished(s) || migration_has_failed(s)) {
> +        MigrationIncomingState *mis = migration_incoming_get_current();
> +        PostcopyBlocktimeContext *ctx = mis->blocktime_ctx;
> +
> +        if (!ctx) {
> +            return;
> +        }

If we are in postcopy_migration_cb() already, then we have registered
the notifiers, then... could this (!ctx) really happen?

> +
>          g_free(ctx->page_fault_vcpu_time);
>          /* g_free is NULL robust */
>          ctx->page_fault_vcpu_time = NULL;
>          g_free(ctx->vcpu_addr);
>          ctx->vcpu_addr = NULL;
> +        g_free(mis->copied_pages);
> +        mis->copied_pages = NULL;
>      }
>  }
>  
>  static void migration_exit_cb(Notifier *n, void *data)
>  {
> -    PostcopyBlocktimeContext *ctx = container_of(n, PostcopyBlocktimeContext,
> -                                               exit_notifier);
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    PostcopyBlocktimeContext *ctx = mis->blocktime_ctx;
> +    if (!ctx) {
> +        return;
> +    }

I failed to find out why touched this up...

Thanks,

>      destroy_blocktime_context(ctx);
> +    mis->blocktime_ctx = NULL;
>  }
>  
>  static struct PostcopyBlocktimeContext *blocktime_context_new(void)
> @@ -227,6 +238,9 @@ static bool ufd_check_and_apply(int ufd, 
> MigrationIncomingState *mis)
>              mis->blocktime_ctx = blocktime_context_new();
>          }
>  
> +        if (!mis->copied_pages) {
> +            mis->copied_pages = copied_pages_bitmap_new();
> +        }
>          asked_features |= UFFD_FEATURE_THREAD_ID;
>      }
>  #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index f59fdd4..1abb6bb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2661,6 +2661,69 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>      return ret;
>  }
>  
> +static unsigned long get_total_bits_per_page(ram_addr_t mem_length,
> +        size_t page_size)
> +{
> +    unsigned long page_size_bit = find_last_bit((unsigned long *)&page_size,
> +            BITS_PER_LONG);
> +    unsigned long total_bits = mem_length >> page_size_bit;
> +    if (mem_length % page_size) {
> +        total_bits += 1;
> +    }
> +    return total_bits;
> +}
> +
> +/*
> + * this function allocates bitmap for copied pages,
> + * also it calculates
> + * how many entries do we need
> + * */
> +unsigned long int *copied_pages_bitmap_new(void)
> +{
> +    RAMBlock *block;
> +    unsigned long int total_bits = 0;
> +
> +    rcu_read_lock();
> +    RAMBLOCK_FOREACH(block) {
> +        /* in general case used_length may not be aligned
> +         * by page_size */
> +
> +        total_bits += get_total_bits_per_page(block->used_length,
> +                block->page_size);
> +    }
> +    rcu_read_unlock();
> +
> +    return bitmap_new(total_bits);
> +}
> +
> +unsigned long int get_copied_bit_offset(ram_addr_t addr)
> +{
> +    RAMBlock *block;
> +    unsigned long int iter_bit = 0;
> +
> +    rcu_read_lock();
> +    RAMBLOCK_FOREACH(block) {
> +        /* in general case used_length may not be aligned
> +         * by page_size */
> +        if (block->host == NULL) {
> +            continue;
> +        }
> +        if (addr - (ram_addr_t)block->host < block->max_length) {
> +            unsigned long page_size_bit = find_last_bit(
> +                    (unsigned long *)&block->page_size,
> +                    BITS_PER_LONG);
> +            ram_addr_t offset = addr - (ram_addr_t)block->host;
> +            iter_bit += offset >> page_size_bit;
> +            break;
> +        }
> +        iter_bit += get_total_bits_per_page(block->used_length,
> +                block->page_size);
> +    }
> +    rcu_read_unlock();
> +
> +    return iter_bit;
> +}
> +
>  static SaveVMHandlers savevm_ram_handlers = {
>      .save_live_setup = ram_save_setup,
>      .save_live_iterate = ram_save_iterate,
> -- 
> 1.8.3.1
> 

-- 
Peter Xu