[Qemu-devel] [PATCH v1 0/2] Add bitmap for copied pages in postcopy migration

[Qemu-devel] [PATCH v1 0/2] Add bitmap for copied pages in postcopy migration

[Alexey Perevalov]

This is a separate patch set, it derived from
https://www.mail-archive.com/qemu-devel@nongnu.org/msg456004.html

There are several possible use cases:
1. solve issue with postcopy live migration and shared memory.
OVS-VSWITCH requires information about copied pages, to fallocate
newly allocated pages.
2. calculation vCPU blocktime
for more details see
https://www.mail-archive.com/qemu-devel@nongnu.org/msg456004.html
3. Recovery after fail in the middle of postcopy migration 

Declaration is placed in two places include/migration/migration.h and into
migration/postcopy-ram.h, because some functions are required in virtio.

Alexey Perevalov (2):
  migration: postcopy_place_page factoring out
  migration: add bitmap for copied page

 include/exec/ram_addr.h       |  3 +++
 include/migration/migration.h |  3 +++
 migration/postcopy-ram.c      | 20 +++++++++++----
 migration/postcopy-ram.h      |  4 +--
 migration/ram.c               | 58 ++++++++++++++++++++++++++++++++++++++++---
 migration/ram.h               |  5 ++++
 migration/savevm.c            |  1 +
 7 files changed, 84 insertions(+), 10 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v1 1/2] migration: postcopy_place_page factoring out

[Alexey Perevalov]

Need to mark copied pages as closer as possible to the place where it
tracks down. That will be necessary in futher patch.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 migration/postcopy-ram.c | 13 ++++++++-----
 migration/postcopy-ram.h |  4 ++--
 migration/ram.c          |  4 ++--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 9c41887..f6244ee 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -566,9 +566,10 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
  * returns 0 on success
  */
 int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
-                        size_t pagesize)
+                        RAMBlock *rb)
 {
     struct uffdio_copy copy_struct;
+    size_t pagesize = qemu_ram_pagesize(rb);
 
     copy_struct.dst = (uint64_t)(uintptr_t)host;
     copy_struct.src = (uint64_t)(uintptr_t)from;
@@ -597,10 +598,12 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
  * returns 0 on success
  */
 int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
-                             size_t pagesize)
+                             RAMBlock *rb)
 {
+    size_t pagesize;
     trace_postcopy_place_page_zero(host);
 
+    pagesize = qemu_ram_pagesize(rb);
     if (pagesize == getpagesize()) {
         struct uffdio_zeropage zero_struct;
         zero_struct.range.start = (uint64_t)(uintptr_t)host;
@@ -631,7 +634,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
             memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size);
         }
         return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page,
-                                   pagesize);
+                                   rb);
     }
 
     return 0;
@@ -694,14 +697,14 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 }
 
 int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
-                        size_t pagesize)
+                        RAMBlock *rb)
 {
     assert(0);
     return -1;
 }
 
 int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
-                        size_t pagesize)
+                        RAMBlock *rb)
 {
     assert(0);
     return -1;
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index 52d51e8..78a3591 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -72,14 +72,14 @@ void postcopy_discard_send_finish(MigrationState *ms,
  * returns 0 on success
  */
 int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
-                        size_t pagesize);
+                        RAMBlock *rb);
 
 /*
  * Place a zero page at (host) atomically
  * returns 0 on success
  */
 int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
-                             size_t pagesize);
+                             RAMBlock *rb);
 
 /* The current postcopy state is read/set by postcopy_state_get/set
  * which update it atomically.
diff --git a/migration/ram.c b/migration/ram.c
index f387e9c..4ed7c2c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2503,10 +2503,10 @@ static int ram_load_postcopy(QEMUFile *f)
 
             if (all_zero) {
                 ret = postcopy_place_page_zero(mis, place_dest,
-                                               block->page_size);
+                                               block);
             } else {
                 ret = postcopy_place_page(mis, place_dest,
-                                          place_source, block->page_size);
+                                          place_source, block);
             }
         }
         if (!ret) {
-- 
1.9.1

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

[Alexey Perevalov]

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.

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

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 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 */
+    unsigned long *copiedmap;
+    size_t nr_copiedmap;
 };
 
 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
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)
+{
+    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)
+{
+    uint64_t host_addr_offset = (uint64_t)(uintptr_t)(host_addr
+                                                      - (void *)rb->host);
+    int page_shift = find_first_bit(&rb->page_size, 8 * sizeof(rb->page_size));
+
+    return host_addr_offset >> page_shift;
+}
+
+int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb)
+{
+    return test_bit(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
+}
+
+void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb)
+{
+    set_bit_atomic(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
+}
+
+size_t get_copiedmap_size(RAMBlock *rb)
+{
+    return rb->nr_copiedmap;
+}
+
+/*
+ * This function copies copiedmap from RAMBlock into
+ * dst memory region
+ *
+ * @dst destination address
+ * @rb RAMBlock source
+ * @len length in bytes
+ */
+void movecopiedmap(void *dst, RAMBlock *rb, size_t len)
+{
+    memcpy(dst, rb->copiedmap, len);
+}
+
 /*
  * An outstanding page request, on the source, having been received
  * and queued
@@ -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);
     if (!rb) {
         error_report("ram_discard_range: Failed to find block '%s'", rbname);
         goto err;
diff --git a/migration/ram.h b/migration/ram.h
index c9563d1..dc781c1 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -67,4 +67,9 @@ int ram_discard_range(const char *block_name, uint64_t start, size_t length);
 int ram_postcopy_incoming_init(MigrationIncomingState *mis);
 
 void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
+
+void init_copiedmap(void);
+int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb);
+void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb);
+
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..7b3726a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1385,6 +1385,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
         return -1;
     }
 
+    init_copiedmap();
     remote_pagesize_summary = qemu_get_be64(mis->from_src_file);
     local_pagesize_summary = ram_pagesize_summary();
 
-- 
1.9.1

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

[Juan Quintela]

Alexey Perevalov <a.perevalov@samsung.com> 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 <a.perevalov@samsung.com>
> ---
>  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.

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

[Alexey Perevalov]

On 06/13/2017 02:42 PM, Juan Quintela wrote:
> Alexey Perevalov <a.perevalov@samsung.com> 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.
yes, sounds good
>
>
>> 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?
Page size per ram block can be different, just to
reduce whole size of 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.
thanks
>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   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?
Peter, mentioned precopy case, but I don't know exactly what this case is.
So I would like to know.
>
>> +    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?
looks like I traced rb, here, and not fully remove it.
>
> Thanks for the effort.
>
> Later, Juan.
>
>
>

-- 
Best regards,
Alexey Perevalov

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

[Peter Xu]

On Tue, Jun 13, 2017 at 03:00:28PM +0300, Alexey Perevalov wrote:
> On 06/13/2017 02:42 PM, Juan Quintela wrote:

[...]

> >>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?
> Page size per ram block can be different, just to
> reduce whole size of bitmap.

I think one reason that we should use TARGET_PAGE_SIZE not
per-ramblock page size bitmap is that we need to let this
copied_bitmap/recved_bitmap work even during precopy phase, and in
precopy phase we are migrating pages in target page size, not host
page size.

Thanks,

-- 
Peter Xu

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

[Alexey Perevalov]

On 06/13/2017 02:42 PM, Juan Quintela wrote:
> Alexey Perevalov <a.perevalov@samsung.com> 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.
I saw patch "migration: Convert ram to use new load_setup()/load_cleanup()",
second version

ram_load_cleanup - IIUC, calls as load_cleanup callback, from 2 use cases:
	1. QEMU_OPTION_loadvm
	2. hmp_loadvm

in both cases: load_snapshot, qemu_loadvm_state, qemu_loadvm_state_cleanup, load_cleanup callback

I applied you patches, it based on:
Merge remote-tracking branch 
'remotes/pmaydell/tags/pull-target-arm-20170613' into staging

load_cleanup is not calling, in case of postcopy live migration, I 
initiate it by QMP command.


>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   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.
>
>
>

-- 
Best regards,
Alexey Perevalov

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

[Juan Quintela]

Alexey Perevalov <a.perevalov@samsung.com> wrote:
> On 06/13/2017 02:42 PM, Juan Quintela wrote:
>> Alexey Perevalov <a.perevalov@samsung.com> 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.
> I saw patch "migration: Convert ram to use new load_setup()/load_cleanup()",
> second version
>
> ram_load_cleanup - IIUC, calls as load_cleanup callback, from 2 use cases:
> 	1. QEMU_OPTION_loadvm
> 	2. hmp_loadvm
>
> in both cases: load_snapshot, qemu_loadvm_state,
> qemu_loadvm_state_cleanup, load_cleanup callback

Hi

Are you looking at v2?

On my tree:

int qemu_loadvm_state(QEMUFile *f)
{

....

    qemu_loadvm_state_cleanup();
    cpu_synchronize_all_post_init();

    return ret;
}

And everything that counts call qemu_loadvm_state() to load the state,
no?

Later, Juan.

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

[Alexey]

On Wed, Jun 14, 2017 at 05:29:39PM +0200, Juan Quintela wrote:
> Alexey Perevalov <a.perevalov@samsung.com> wrote:
> > On 06/13/2017 02:42 PM, Juan Quintela wrote:
> >> Alexey Perevalov <a.perevalov@samsung.com> 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.
> > I saw patch "migration: Convert ram to use new load_setup()/load_cleanup()",
> > second version
> >
> > ram_load_cleanup - IIUC, calls as load_cleanup callback, from 2 use cases:
> > 	1. QEMU_OPTION_loadvm
> > 	2. hmp_loadvm
> >
> > in both cases: load_snapshot, qemu_loadvm_state,
> > qemu_loadvm_state_cleanup, load_cleanup callback
> 
> Hi
> 
> Are you looking at v2?
> 
Yes, I took v2.
> On my tree:
> 
> int qemu_loadvm_state(QEMUFile *f)
> {
> 
> ....
it presents, but listen thread still exists
and on mis->have_listen_thread condition we're returning
from qemu_loadvm_state, before qemu_loadvm_state_cleanup invocation.
> 
>     qemu_loadvm_state_cleanup();
>     cpu_synchronize_all_post_init();
> 
>     return ret;
> }
> 
> And everything that counts call qemu_loadvm_state() to load the state,
> no?
yes, in my case qemu_loadvm_state is calling.
> 
> Later, Juan.
> 

-- 

BR
Alexey

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

[Peter Xu]

On Tue, Jun 13, 2017 at 12:36:33PM +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, 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.
> 
> copied bitmap is not releasing due to ramblocks is not releasing
> too.
> 
> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  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 */
> +    unsigned long *copiedmap;
> +    size_t nr_copiedmap;

Do we really need this?

>  };
>  
>  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
> 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);
> +

I guess this is not enough?

For postcopy, you may have missed to trap in
postcopy_place_page_zero() when pagesize == getpagesize() (we used
UFFDIO_ZEROPAGE there)?

(Btw, why not we trap all these in ram_load_postcopy()?)

For precopy, looks like it's missing as well? I believe it's in
ram_load().

>      /* 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)
> +{
> +    RAMBlock *rb;

(Nit: better with a new line after parameters.)

> +    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)
> +{
> +    uint64_t host_addr_offset = (uint64_t)(uintptr_t)(host_addr
> +                                                      - (void *)rb->host);
> +    int page_shift = find_first_bit(&rb->page_size, 8 * sizeof(rb->page_size));
> +
> +    return host_addr_offset >> page_shift;
> +}
> +
> +int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb)
> +{
> +    return test_bit(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
> +}
> +
> +void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb)
> +{
> +    set_bit_atomic(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
> +}
> +
> +size_t get_copiedmap_size(RAMBlock *rb)
> +{
> +    return rb->nr_copiedmap;
> +}

As commented by Juan, I think we need per-TARGET_PAGE_SIZE bitmap. If
so, we should be able to get rid of these helpers?

> +
> +/*
> + * This function copies copiedmap from RAMBlock into
> + * dst memory region
> + *
> + * @dst destination address
> + * @rb RAMBlock source
> + * @len length in bytes
> + */
> +void movecopiedmap(void *dst, RAMBlock *rb, size_t len)
> +{
> +    memcpy(dst, rb->copiedmap, len);
> +}
> +
>  /*
>   * An outstanding page request, on the source, having been received
>   * and queued
> @@ -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 this move?

And still, we need logic for the discard during postcopy?

Thanks,

>      if (!rb) {
>          error_report("ram_discard_range: Failed to find block '%s'", rbname);
>          goto err;
> diff --git a/migration/ram.h b/migration/ram.h
> index c9563d1..dc781c1 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -67,4 +67,9 @@ int ram_discard_range(const char *block_name, uint64_t start, size_t length);
>  int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>  
>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> +
> +void init_copiedmap(void);
> +int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb);
> +void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb);
> +
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9c320f5..7b3726a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1385,6 +1385,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
>          return -1;
>      }
>  
> +    init_copiedmap();
>      remote_pagesize_summary = qemu_get_be64(mis->from_src_file);
>      local_pagesize_summary = ram_pagesize_summary();
>  
> -- 
> 1.9.1
> 

Thanks,

-- 
Peter Xu

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

[Alexey Perevalov]

On 06/14/2017 08:12 AM, Peter Xu wrote:
> On Tue, Jun 13, 2017 at 12:36:33PM +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, 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.
>>
>> copied bitmap is not releasing due to ramblocks is not releasing
>> too.
>>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   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 */
>> +    unsigned long *copiedmap;
>> +    size_t nr_copiedmap;
> Do we really need this?
This and question about

ram_discard_range Juan already asked.
I just repeat nr_copiedmap - premature optimization )
ram_discard_range - I forgot to clean up patch.

>
>>   };
>>   
>>   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
>> 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);
>> +
> I guess this is not enough?
yes, zero pages were missed
>
> For postcopy, you may have missed to trap in
> postcopy_place_page_zero() when pagesize == getpagesize() (we used
> UFFDIO_ZEROPAGE there)?
>
> (Btw, why not we trap all these in ram_load_postcopy()?)
I tried to place

set_copiedmap_by_addr as closer as possible to ioctl. It could be important to reduce probability of
race, also when I inform OVS-VSWITCH (patches not yet published), it's important to reduce time between
mark page as copied and really coping.

>
> For precopy, looks like it's missing as well? I believe it's in
> ram_load().


>
>>       /* 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)
>> +{
>> +    RAMBlock *rb;
> (Nit: better with a new line after parameters.)
>
>> +    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)
>> +{
>> +    uint64_t host_addr_offset = (uint64_t)(uintptr_t)(host_addr
>> +                                                      - (void *)rb->host);
>> +    int page_shift = find_first_bit(&rb->page_size, 8 * sizeof(rb->page_size));
>> +
>> +    return host_addr_offset >> page_shift;
>> +}
>> +
>> +int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb)
>> +{
>> +    return test_bit(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
>> +}
>> +
>> +void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb)
>> +{
>> +    set_bit_atomic(get_copied_bit_offset(host_addr, rb), rb->copiedmap);
>> +}
>> +
>> +size_t get_copiedmap_size(RAMBlock *rb)
>> +{
>> +    return rb->nr_copiedmap;
>> +}
> As commented by Juan, I think we need per-TARGET_PAGE_SIZE bitmap. If
> so, we should be able to get rid of these helpers?
ok, it's significant argument, so TARGET_PAGE_SIZE will be
here. The size of bitmap will increase, and I pass that bitmap into 
OVS-VSWITCHD,
ok it is another story.
>
>> +
>> +/*
>> + * This function copies copiedmap from RAMBlock into
>> + * dst memory region
>> + *
>> + * @dst destination address
>> + * @rb RAMBlock source
>> + * @len length in bytes
>> + */
>> +void movecopiedmap(void *dst, RAMBlock *rb, size_t len)
>> +{
>> +    memcpy(dst, rb->copiedmap, len);
>> +}
>> +
>>   /*
>>    * An outstanding page request, on the source, having been received
>>    * and queued
>> @@ -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 this move?
>
> And still, we need logic for the discard during postcopy?
>
> Thanks,
>
>>       if (!rb) {
>>           error_report("ram_discard_range: Failed to find block '%s'", rbname);
>>           goto err;
>> diff --git a/migration/ram.h b/migration/ram.h
>> index c9563d1..dc781c1 100644
>> --- a/migration/ram.h
>> +++ b/migration/ram.h
>> @@ -67,4 +67,9 @@ int ram_discard_range(const char *block_name, uint64_t start, size_t length);
>>   int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>>   
>>   void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>> +
>> +void init_copiedmap(void);
>> +int test_copiedmap_by_addr(void *host_addr, RAMBlock *rb);
>> +void set_copiedmap_by_addr(void *host_addr, RAMBlock *rb);
>> +
>>   #endif
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 9c320f5..7b3726a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1385,6 +1385,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
>>           return -1;
>>       }
>>   
>> +    init_copiedmap();
>>       remote_pagesize_summary = qemu_get_be64(mis->from_src_file);
>>       local_pagesize_summary = ram_pagesize_summary();
>>   
>> -- 
>> 1.9.1
>>
> Thanks,
>

-- 
Best regards,
Alexey Perevalov

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

[Peter Xu]

On Wed, Jun 14, 2017 at 09:39:53AM +0300, Alexey Perevalov wrote:
> On 06/14/2017 08:12 AM, Peter Xu wrote:
> >On Tue, Jun 13, 2017 at 12:36:33PM +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, 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.
> >>
> >>copied bitmap is not releasing due to ramblocks is not releasing
> >>too.
> >>
> >>Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> >>---
> >>  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 */
> >>+    unsigned long *copiedmap;
> >>+    size_t nr_copiedmap;
> >Do we really need this?
> This and question about
> 
> ram_discard_range Juan already asked.
> I just repeat nr_copiedmap - premature optimization )
> ram_discard_range - I forgot to clean up patch.
> 
> >
> >>  };
> >>  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
> >>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);
> >>+
> >I guess this is not enough?
> yes, zero pages were missed
> >
> >For postcopy, you may have missed to trap in
> >postcopy_place_page_zero() when pagesize == getpagesize() (we used
> >UFFDIO_ZEROPAGE there)?
> >
> >(Btw, why not we trap all these in ram_load_postcopy()?)
> I tried to place
> 
> set_copiedmap_by_addr as closer as possible to ioctl. It could be important to reduce probability of
> race, also when I inform OVS-VSWITCH (patches not yet published), it's important to reduce time between
> mark page as copied and really coping.

If so, I would suggest we comment this in the codes.

Btw, could you elaborate a bit in detail on why you need to "reduce
the time" here? IMHO the ordering should matter and I can understand
(and actually we can also achieve the ordering requirement if we put
this handling in ram_load_postcopy()), but I cannot really understand
when you say "it's important to reduce time between A and B"...

Thanks,

-- 
Peter Xu

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

[Alexey Perevalov]

On 06/14/2017 09:53 AM, Peter Xu wrote:
> On Wed, Jun 14, 2017 at 09:39:53AM +0300, Alexey Perevalov wrote:
>> On 06/14/2017 08:12 AM, Peter Xu wrote:
>>> On Tue, Jun 13, 2017 at 12:36:33PM +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, 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.
>>>>
>>>> copied bitmap is not releasing due to ramblocks is not releasing
>>>> too.
>>>>
>>>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>>>> ---
>>>>   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 */
>>>> +    unsigned long *copiedmap;
>>>> +    size_t nr_copiedmap;
>>> Do we really need this?
>> This and question about
>>
>> ram_discard_range Juan already asked.
>> I just repeat nr_copiedmap - premature optimization )
>> ram_discard_range - I forgot to clean up patch.
>>
>>>>   };
>>>>   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
>>>> 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);
>>>> +
>>> I guess this is not enough?
>> yes, zero pages were missed
>>> For postcopy, you may have missed to trap in
>>> postcopy_place_page_zero() when pagesize == getpagesize() (we used
>>> UFFDIO_ZEROPAGE there)?
>>>
>>> (Btw, why not we trap all these in ram_load_postcopy()?)
>> I tried to place
>>
>> set_copiedmap_by_addr as closer as possible to ioctl. It could be important to reduce probability of
>> race, also when I inform OVS-VSWITCH (patches not yet published), it's important to reduce time between
>> mark page as copied and really coping.
> If so, I would suggest we comment this in the codes.
>
> Btw, could you elaborate a bit in detail on why you need to "reduce
> the time" here? IMHO the ordering should matter and I can understand
> (and actually we can also achieve the ordering requirement if we put
> this handling in ram_load_postcopy()), but I cannot really understand
> when you say "it's important to reduce time between A and B"...
Yes, it worth to describe it in details,
on one hand we decided to call mark_postcopy_blocktime_end before ioctl, 
because
we worried about another page fault between ioctl and 
mark_postcopy_blocktime_end
(on the same vCPU as for page fault for current ioctl)
previous pagefault ( details here Message-ID: 
<20170524112220.GJ3873@pxdev.xzpeter.org>)
on other hand I'm sending copied bitmap to the OVS-VSWITCHD, that bitmap 
is necessary to make
fallocate there, otherwise ioctl UFFDIO_COPY will return EEXITS,
page will be remmaped after VHOST_USER_SET_MEM_TABLE on OVS side (second 
mmap of currently discarded page).

Above, I described problem of shared memory with OVS,
we discussed it with David, but not yet, publicly in qemu-devel list 
(may on redhat's bugzilla).
Sorry, it's not a topic of that discussion, but...

Of course reducing time between mark page as copied and real ioctl is 
not solution, here could be following
solutions:
     1. bitmap which indicates that page was really copied successfully, 
but it's yet another bit map.
     2. every time after ioctl UFFDIO_COPY, send notification to 
OVS-VSWITCH, OVS is discarding that page
if it was an EEXIST error,
and postcopy_place_page is repeats copying page.


>
> Thanks,
>

-- 
Best regards,
Alexey Perevalov