Re: [Qemu-devel] [PATCH v1 2/2] migration: add bitmap for copied page

[Juan Quintela]

Alexey Perevalov <address@hidden> wrote:

Hi

I think that it would make things clearer if we do a s/copied/received/
As what we are tracking here are the pages that have already been
received.


> This patch adds ability to track down already copied
> pages, it's necessary for calculation vCPU block time in
> postcopy migration feature, maybe for restore after
> postcopy migration failure.
> Also it's necessary to solve shared memory issue in
> postcopy livemigration. Information about copied pages
> will be transferred to the software virtual bridge
> (e.g. OVS-VSWITCHD), to avoid fallocate (unmap) for
> already copied pages. fallocate syscall is required for
> remmaped shared memory, due to remmaping itself blocks
> ioctl(UFFDIO_COPY, ioctl in this case will end with EEXIT
> error (struct page is exists after remmap).
>
> Bitmap is placed into RAMBlock as another postcopy/precopy
> related bitmaps.

Why are we not using the TARGET_PAGE_SIZE as units of the bitmap?


> copied bitmap is not releasing due to ramblocks is not releasing
> too.

RAMBlocks are used for other reasons, not only migration.  This bitmaps
can be released on the ram_load_cleanup() function.  See my patches on
list about how to use it.

> Signed-off-by: Alexey Perevalov <address@hidden>
> ---
>  include/exec/ram_addr.h       |  3 +++
>  include/migration/migration.h |  3 +++
>  migration/postcopy-ram.c      |  7 ++++++
>  migration/ram.c               | 54 
> ++++++++++++++++++++++++++++++++++++++++++-
>  migration/ram.h               |  5 ++++
>  migration/savevm.c            |  1 +
>  6 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 140efa8..56cdf16 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -47,6 +47,9 @@ struct RAMBlock {
>       * of the postcopy phase
>       */
>      unsigned long *unsentmap;
> +    /* bitmap of already copied pages in postcopy */
       /* bitmap of already received pages */

Why do we want to use it only for postcopy?  And not in the general case?

> +    unsigned long *copiedmap;

       unsigned long *receivedmap;

> +    size_t nr_copiedmap;

I think we don't need it, just use on its only use:

   rb->max_length >> qemu_page_target_bits()?

>  };
>  
>  static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 79b5484..8005c11 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -226,4 +226,7 @@ void savevm_skip_configuration(void);
>  int global_state_store(void);
>  void global_state_store_running(void);
>  
> +size_t get_copiedmap_size(RAMBlock *rb);
> +void movecopiedmap(void *dst, RAMBlock *rb, size_t len);
> +
>  #endif

After my last PULL Request, this needs to be into:

   include/migration/misc.h

And we can rename the functions to:

size_t received_map_pages(RAMBlock *rb);
void received_map_copy(void *dst, RAMBlock *rb, size_t len);


> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index f6244ee..e13b22e 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -576,6 +576,13 @@ int postcopy_place_page(MigrationIncomingState *mis, 
> void *host, void *from,
>      copy_struct.len = pagesize;
>      copy_struct.mode = 0;
>  
> +
> +    /* copied page isn't feature of blocktime calculation,
> +     * it's more general entity, so keep it here,
> +     * but gup betwean two following operation could be high,
> +     * and in this case blocktime for such small interval will be lost */
> +    set_copiedmap_by_addr(host, rb);
> 
>      /* copy also acks to the kernel waking the stalled thread up
>       * TODO: We can inhibit that ack and only do it if it was requested
>       * which would be slightly cheaper, but we'd have to be careful
> diff --git a/migration/ram.c b/migration/ram.c
> index 4ed7c2c..8466e59 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -149,6 +149,58 @@ out:
>      return ret;
>  }
>  
> +void init_copiedmap(void)

received_map_init(void);

> +{
> +    RAMBlock *rb;
> +    RAMBLOCK_FOREACH(rb) {
> +        unsigned long pages;
> +        pages = rb->max_length >> find_first_bit(&rb->page_size,
> +                8 * sizeof(rb->page_size));
> +        /* need for destination, bitmap_new calls
> +         * g_try_malloc0 and this function
> +         * Attempts to allocate @n_bytes, initialized to 0'sh */
> +        rb->copiedmap = bitmap_new(pages);
> +        rb->nr_copiedmap = pages;
> +    }
> +}
> +
> +static unsigned long int get_copied_bit_offset(void *host_addr,
> RAMBlock *rb)


received_map_get_offset(...)

> +{
> +    uint64_t host_addr_offset = (uint64_t)(uintptr_t)(host_addr
> +                                                      
> +    int page_shift = find_first_bit(&rb->page_size, 8 * 
> sizeof(rb->page_size));

with my suggesiton this becomes:

uint64_t offset = (uint64_t)(uintptr_t)(host_addr - (void *)rb->host);
return offset >> qemu_target_page_bits();

I wonder that we don't have a function to pass an absolute address to an
relative page :p


> +int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb)

  int receive_map_test_addr(...) ?
  
> +{
> +    return test_bit(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
> +}
> +
> +void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb)

void receive_map_set_addr()


> @@ -1852,11 +1904,11 @@ int ram_discard_range(const char *rbname, uint64_t 
> start, size_t length)
>  {
>      int ret = -1;
>  
> -    trace_ram_discard_range(rbname, start, length);
>  
>      rcu_read_lock();
>      RAMBlock *rb = qemu_ram_block_by_name(rbname);
>  
> +    trace_ram_discard_range(rbname, start, length);

Why is that bit included in this patch?  I think this is independent of
the rest of the patch, no?

Thanks for the effort.

Later, Juan.