[Qemu-devel] [PATCH v9 0/3] Add bitmap for received pages in postcopy migration

[Qemu-devel] [PATCH v9 0/3] Add bitmap for received pages in postcopy migration

[Alexey Perevalov]

This is 9th version of
[PATCH v1 0/2] Add bitmap for copied pages in postcopy migration
cover message from there

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 

----------------------------------------------------------------
V8 -> V9
    - patch: "migration: fix incorrect postcopy recved_bitmap" from
"[RFC 00/29] Migration: postcopy failure recovery" patch set was squashed
into the latest patch of this patchset, getpagesize was replaced to
qemu_target_page_size, as David suggested.
    - for the sake of API uniformity semantics of all functions were
changed, now RAMBlock *rb is the first argument, as well as in bitmap API.
    - Also define TARGET_PAGE_BITS was replaced to qemu_target_page_bits in
all other places of this patchset, for uniformity and maintenance. 

V7 -> V8
    - removed unnecessary renaming and moving of block variable to ram_load's
function scope
    - ramblock_recv_map_init became static function

V6 -> V7
    - rebased on
[PATCH v7 0/5] Create setup/cleanup methods for migration incoming side
    - live time of the received map was changed (ram_load_setup/ram_load_cleanup) 

V5 -> V6
    - call ramblock_recv_map_init from migration_fd_process_incoming (Peter suggested)But finalization is still in ram_load_cleanup as Juan suggested.    

V4 -> V5
    - remove ramblock_recv_bitmap_clear_range in favor to bitmap_clear (comment from David)
    - single invocation place for ramblock_recv_bitmap_set (comment from Peter)
    - minor changes like removing comment from qemu_ufd_copy_ioctl and local variable from
ramblock_recv_map_init (comment from Peter)

V3 -> V4
    - clear_bit instead of ramblock_recv_bitmap_clear in ramblock_recv_bitmap_clear_range,
it reduced number of operation (comment from Juan)
    - for postcopy ramblock_recv_bitmap_set is calling after page was copied,
only in case of success (comment from David)
    - indentation fixes (comment from Juan)

V2 -> V3
    - ramblock_recv_map_init call is placed into migration_incoming_get_current,
looks like it's general place for both precopy and postcopy case.
    - received bitmap memory releasing is placed into ram_load_cleanup,
unfortunatelly, it calls only in case of precopy.
    - precopy case and discard ram block case
    - function renaming, and another minor cleanups

V1 -> V2
    - change in terminology s/copied/received/g
    - granularity became TARGET_PAGE_SIZE, but not actual page size of the
ramblock
    - movecopiedmap & get_copiedmap_size were removed, until patch set where
it will be necessary
    - releasing memory of receivedmap was added into ram_load_cleanup
    - new patch "migration: introduce qemu_ufd_copy_ioctl helper"

Patchset is based on:
commit 6d60e295ef020759a03b90724d0342012c189ba2
"Merge remote-tracking branch 'remotes/jnsnow/tags/ide-pull-request' into staging"

Alexey Perevalov (3):
  migration: postcopy_place_page factoring out
  migration: introduce qemu_ufd_copy_ioctl helper
  migration: add bitmap for received page

 include/exec/ram_addr.h  | 10 +++++++++
 migration/postcopy-ram.c | 54 +++++++++++++++++++++++++++++++-----------------
 migration/postcopy-ram.h |  4 ++--
 migration/ram.c          | 49 +++++++++++++++++++++++++++++++++++++++++--
 migration/ram.h          |  6 ++++++
 5 files changed, 100 insertions(+), 23 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v9 1/3] 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: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
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, 11 insertions(+), 10 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 7e21e6f..996e64d 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -564,9 +564,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;
@@ -595,11 +596,11 @@ 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)
 {
     trace_postcopy_place_page_zero(host);
 
-    if (pagesize == getpagesize()) {
+    if (qemu_ram_pagesize(rb) == getpagesize()) {
         struct uffdio_zeropage zero_struct;
         zero_struct.range.start = (uint64_t)(uintptr_t)host;
         zero_struct.range.len = getpagesize();
@@ -629,7 +630,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;
@@ -692,14 +693,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 1b08296..9cc1b17 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2470,10 +2470,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 v9 2/3] migration: introduce qemu_ufd_copy_ioctl helper

[Alexey Perevalov]

Just for placing auxilary operations inside helper,
auxilary operations like: track received pages,
notify about copying operation in futher patches.

Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 migration/postcopy-ram.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 996e64d..be497bb 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -559,6 +559,25 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
     return 0;
 }
 
+static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
+        void *from_addr, uint64_t pagesize)
+{
+    if (from_addr) {
+        struct uffdio_copy copy_struct;
+        copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
+        copy_struct.src = (uint64_t)(uintptr_t)from_addr;
+        copy_struct.len = pagesize;
+        copy_struct.mode = 0;
+        return ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
+    } else {
+        struct uffdio_zeropage zero_struct;
+        zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
+        zero_struct.range.len = pagesize;
+        zero_struct.mode = 0;
+        return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
+    }
+}
+
 /*
  * Place a host page (from) at (host) atomically
  * returns 0 on success
@@ -566,20 +585,14 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
                         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;
-    copy_struct.len = pagesize;
-    copy_struct.mode = 0;
-
     /* 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
      * of the order of updating our page state.
      */
-    if (ioctl(mis->userfault_fd, UFFDIO_COPY, &copy_struct)) {
+    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize)) {
         int e = errno;
         error_report("%s: %s copy host: %p from: %p (size: %zd)",
                      __func__, strerror(e), host, from, pagesize);
@@ -601,12 +614,7 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
     trace_postcopy_place_page_zero(host);
 
     if (qemu_ram_pagesize(rb) == getpagesize()) {
-        struct uffdio_zeropage zero_struct;
-        zero_struct.range.start = (uint64_t)(uintptr_t)host;
-        zero_struct.range.len = getpagesize();
-        zero_struct.mode = 0;
-
-        if (ioctl(mis->userfault_fd, UFFDIO_ZEROPAGE, &zero_struct)) {
+        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize())) {
             int e = errno;
             error_report("%s: %s zero host: %p",
                          __func__, strerror(e), host);
-- 
1.9.1

[Qemu-devel] [PATCH v9 3/3] migration: add bitmap for received page

[Alexey Perevalov]

This patch adds ability to track down already received
pages, it's necessary for calculation vCPU block time in
postcopy migration feature, and for recovery after
postcopy migration failure.

Also it's necessary to solve shared memory issue in
postcopy livemigration. Information about received pages
will be transferred to the software virtual bridge
(e.g. OVS-VSWITCHD), to avoid fallocate (unmap) for
already received 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.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 include/exec/ram_addr.h  | 10 ++++++++++
 migration/postcopy-ram.c | 17 ++++++++++++-----
 migration/ram.c          | 45 +++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h          |  6 ++++++
 4 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index c04f4f6..bb902bb 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -47,6 +47,8 @@ struct RAMBlock {
      * of the postcopy phase
      */
     unsigned long *unsentmap;
+    /* bitmap of already received pages in postcopy */
+    unsigned long *receivedmap;
 };
 
 static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
@@ -60,6 +62,14 @@ static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
     return (char *)block->host + offset;
 }
 
+static inline unsigned long int ramblock_recv_bitmap_offset(void *host_addr,
+                                                            RAMBlock *rb)
+{
+    uint64_t host_addr_offset =
+            (uint64_t)(uintptr_t)(host_addr - (void *)rb->host);
+    return host_addr_offset >> TARGET_PAGE_BITS;
+}
+
 long qemu_getrampagesize(void);
 unsigned long last_ram_page(void);
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index be497bb..7a414eb 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -560,22 +560,28 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
 }
 
 static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
-        void *from_addr, uint64_t pagesize)
+                               void *from_addr, uint64_t pagesize, RAMBlock *rb)
 {
+    int ret;
     if (from_addr) {
         struct uffdio_copy copy_struct;
         copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
         copy_struct.src = (uint64_t)(uintptr_t)from_addr;
         copy_struct.len = pagesize;
         copy_struct.mode = 0;
-        return ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
+        ret = ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
     } else {
         struct uffdio_zeropage zero_struct;
         zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
         zero_struct.range.len = pagesize;
         zero_struct.mode = 0;
-        return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
+        ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
+    }
+    if (!ret) {
+        ramblock_recv_bitmap_set_range(rb, host_addr,
+                                       pagesize / qemu_target_page_size());
     }
+    return ret;
 }
 
 /*
@@ -592,7 +598,7 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
      * which would be slightly cheaper, but we'd have to be careful
      * of the order of updating our page state.
      */
-    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize)) {
+    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
         int e = errno;
         error_report("%s: %s copy host: %p from: %p (size: %zd)",
                      __func__, strerror(e), host, from, pagesize);
@@ -614,7 +620,8 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
     trace_postcopy_place_page_zero(host);
 
     if (qemu_ram_pagesize(rb) == getpagesize()) {
-        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize())) {
+        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize(),
+                                rb)) {
             int e = errno;
             error_report("%s: %s zero host: %p",
                          __func__, strerror(e), host);
diff --git a/migration/ram.c b/migration/ram.c
index 9cc1b17..d14b8bb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -44,6 +44,7 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "exec/ram_addr.h"
+#include "exec/target_page.h"
 #include "qemu/rcu_queue.h"
 #include "migration/colo.h"
 
@@ -147,6 +148,40 @@ out:
     return ret;
 }
 
+static void ramblock_recv_map_init(void)
+{
+    RAMBlock *rb;
+
+    RAMBLOCK_FOREACH(rb) {
+        assert(!rb->receivedmap);
+        rb->receivedmap = bitmap_new(rb->max_length >> qemu_target_page_bits());
+    }
+}
+
+int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr)
+{
+    return test_bit(ramblock_recv_bitmap_offset(host_addr, rb),
+                    rb->receivedmap);
+}
+
+void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr)
+{
+    set_bit_atomic(ramblock_recv_bitmap_offset(host_addr, rb), rb->receivedmap);
+}
+
+void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr,
+                                    size_t nr)
+{
+    bitmap_set_atomic(rb->receivedmap,
+                      ramblock_recv_bitmap_offset(host_addr, rb),
+                      nr);
+}
+
+void ramblock_recv_bitmap_clear(RAMBlock *rb, void *host_addr)
+{
+    clear_bit(ramblock_recv_bitmap_offset(host_addr, rb), rb->receivedmap);
+}
+
 /*
  * An outstanding page request, on the source, having been received
  * and queued
@@ -1793,6 +1828,8 @@ int ram_discard_range(const char *rbname, uint64_t start, size_t length)
         goto err;
     }
 
+    bitmap_clear(rb->receivedmap, start >> qemu_target_page_bits(),
+                 length >> qemu_target_page_bits());
     ret = ram_block_discard_range(rb, start, length);
 
 err:
@@ -2324,13 +2361,20 @@ static int ram_load_setup(QEMUFile *f, void *opaque)
 {
     xbzrle_load_setup();
     compress_threads_load_setup();
+    ramblock_recv_map_init();
     return 0;
 }
 
 static int ram_load_cleanup(void *opaque)
 {
+    RAMBlock *rb;
     xbzrle_load_cleanup();
     compress_threads_load_cleanup();
+
+    RAMBLOCK_FOREACH(rb) {
+        g_free(rb->receivedmap);
+        rb->receivedmap = NULL;
+    }
     return 0;
 }
 
@@ -2545,6 +2589,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
                 ret = -EINVAL;
                 break;
             }
+            ramblock_recv_bitmap_set(block, host);
             trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
         }
 
diff --git a/migration/ram.h b/migration/ram.h
index c081fde..4db9922 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -52,4 +52,10 @@ 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);
+
+int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
+void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
+void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr, size_t nr);
+void ramblock_recv_bitmap_clear(RAMBlock *rb, void *host_addr);
+
 #endif
-- 
1.9.1

Re: [Qemu-devel] [PATCH v9 3/3] migration: add bitmap for received page

[Dr. David Alan Gilbert]

* Alexey Perevalov (a.perevalov@samsung.com) wrote:
> This patch adds ability to track down already received
> pages, it's necessary for calculation vCPU block time in
> postcopy migration feature, and for recovery after
> postcopy migration failure.
> 
> Also it's necessary to solve shared memory issue in
> postcopy livemigration. Information about received pages
> will be transferred to the software virtual bridge
> (e.g. OVS-VSWITCHD), to avoid fallocate (unmap) for
> already received 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.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  include/exec/ram_addr.h  | 10 ++++++++++
>  migration/postcopy-ram.c | 17 ++++++++++++-----
>  migration/ram.c          | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  migration/ram.h          |  6 ++++++
>  4 files changed, 73 insertions(+), 5 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index c04f4f6..bb902bb 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -47,6 +47,8 @@ struct RAMBlock {
>       * of the postcopy phase
>       */
>      unsigned long *unsentmap;
> +    /* bitmap of already received pages in postcopy */
> +    unsigned long *receivedmap;
>  };
>  
>  static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> @@ -60,6 +62,14 @@ static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
>      return (char *)block->host + offset;
>  }
>  
> +static inline unsigned long int ramblock_recv_bitmap_offset(void *host_addr,
> +                                                            RAMBlock *rb)
> +{
> +    uint64_t host_addr_offset =
> +            (uint64_t)(uintptr_t)(host_addr - (void *)rb->host);
> +    return host_addr_offset >> TARGET_PAGE_BITS;
> +}
> +
>  long qemu_getrampagesize(void);
>  unsigned long last_ram_page(void);
>  RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index be497bb..7a414eb 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -560,22 +560,28 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>  }
>  
>  static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
> -        void *from_addr, uint64_t pagesize)
> +                               void *from_addr, uint64_t pagesize, RAMBlock *rb)
>  {
> +    int ret;
>      if (from_addr) {
>          struct uffdio_copy copy_struct;
>          copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
>          copy_struct.src = (uint64_t)(uintptr_t)from_addr;
>          copy_struct.len = pagesize;
>          copy_struct.mode = 0;
> -        return ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
> +        ret = ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
>      } else {
>          struct uffdio_zeropage zero_struct;
>          zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
>          zero_struct.range.len = pagesize;
>          zero_struct.mode = 0;
> -        return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> +        ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> +    }
> +    if (!ret) {
> +        ramblock_recv_bitmap_set_range(rb, host_addr,
> +                                       pagesize / qemu_target_page_size());
>      }
> +    return ret;
>  }
>  
>  /*
> @@ -592,7 +598,7 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>       * which would be slightly cheaper, but we'd have to be careful
>       * of the order of updating our page state.
>       */
> -    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize)) {
> +    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
>          int e = errno;
>          error_report("%s: %s copy host: %p from: %p (size: %zd)",
>                       __func__, strerror(e), host, from, pagesize);
> @@ -614,7 +620,8 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
>      trace_postcopy_place_page_zero(host);
>  
>      if (qemu_ram_pagesize(rb) == getpagesize()) {
> -        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize())) {
> +        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize(),
> +                                rb)) {
>              int e = errno;
>              error_report("%s: %s zero host: %p",
>                           __func__, strerror(e), host);
> diff --git a/migration/ram.c b/migration/ram.c
> index 9cc1b17..d14b8bb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -44,6 +44,7 @@
>  #include "qemu/error-report.h"
>  #include "trace.h"
>  #include "exec/ram_addr.h"
> +#include "exec/target_page.h"
>  #include "qemu/rcu_queue.h"
>  #include "migration/colo.h"
>  
> @@ -147,6 +148,40 @@ out:
>      return ret;
>  }
>  
> +static void ramblock_recv_map_init(void)
> +{
> +    RAMBlock *rb;
> +
> +    RAMBLOCK_FOREACH(rb) {
> +        assert(!rb->receivedmap);
> +        rb->receivedmap = bitmap_new(rb->max_length >> qemu_target_page_bits());
> +    }
> +}
> +
> +int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr)
> +{
> +    return test_bit(ramblock_recv_bitmap_offset(host_addr, rb),
> +                    rb->receivedmap);
> +}
> +
> +void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr)
> +{
> +    set_bit_atomic(ramblock_recv_bitmap_offset(host_addr, rb), rb->receivedmap);
> +}
> +
> +void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr,
> +                                    size_t nr)
> +{
> +    bitmap_set_atomic(rb->receivedmap,
> +                      ramblock_recv_bitmap_offset(host_addr, rb),
> +                      nr);
> +}

Can you remember what the logic was behind making the 'set's atomic
but not the clears?  It's probably at least worth a comment.

Other than that, I think this series is fine; it just needs one of the
series that makes use of it to get it merged
(although the 1st two can go in by themselves)

Dave



> +void ramblock_recv_bitmap_clear(RAMBlock *rb, void *host_addr)
> +{
> +    clear_bit(ramblock_recv_bitmap_offset(host_addr, rb), rb->receivedmap);
> +}
> +
>  /*
>   * An outstanding page request, on the source, having been received
>   * and queued
> @@ -1793,6 +1828,8 @@ int ram_discard_range(const char *rbname, uint64_t start, size_t length)
>          goto err;
>      }
>  
> +    bitmap_clear(rb->receivedmap, start >> qemu_target_page_bits(),
> +                 length >> qemu_target_page_bits());
>      ret = ram_block_discard_range(rb, start, length);
>  
>  err:
> @@ -2324,13 +2361,20 @@ static int ram_load_setup(QEMUFile *f, void *opaque)
>  {
>      xbzrle_load_setup();
>      compress_threads_load_setup();
> +    ramblock_recv_map_init();
>      return 0;
>  }
>  
>  static int ram_load_cleanup(void *opaque)
>  {
> +    RAMBlock *rb;
>      xbzrle_load_cleanup();
>      compress_threads_load_cleanup();
> +
> +    RAMBLOCK_FOREACH(rb) {
> +        g_free(rb->receivedmap);
> +        rb->receivedmap = NULL;
> +    }
>      return 0;
>  }
>  
> @@ -2545,6 +2589,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>                  ret = -EINVAL;
>                  break;
>              }
> +            ramblock_recv_bitmap_set(block, host);
>              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
>          }
>  
> diff --git a/migration/ram.h b/migration/ram.h
> index c081fde..4db9922 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -52,4 +52,10 @@ 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);
> +
> +int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
> +void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
> +void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr, size_t nr);
> +void ramblock_recv_bitmap_clear(RAMBlock *rb, void *host_addr);
> +
>  #endif
> -- 
> 1.9.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v9 3/3] migration: add bitmap for received page

[Alexey Perevalov]

On 09/29/2017 03:15 PM, Dr. David Alan Gilbert wrote:
> * Alexey Perevalov (a.perevalov@samsung.com) wrote:
>> This patch adds ability to track down already received
>> pages, it's necessary for calculation vCPU block time in
>> postcopy migration feature, and for recovery after
>> postcopy migration failure.
>>
>> Also it's necessary to solve shared memory issue in
>> postcopy livemigration. Information about received pages
>> will be transferred to the software virtual bridge
>> (e.g. OVS-VSWITCHD), to avoid fallocate (unmap) for
>> already received 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.
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   include/exec/ram_addr.h  | 10 ++++++++++
>>   migration/postcopy-ram.c | 17 ++++++++++++-----
>>   migration/ram.c          | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   migration/ram.h          |  6 ++++++
>>   4 files changed, 73 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index c04f4f6..bb902bb 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -47,6 +47,8 @@ struct RAMBlock {
>>        * of the postcopy phase
>>        */
>>       unsigned long *unsentmap;
>> +    /* bitmap of already received pages in postcopy */
>> +    unsigned long *receivedmap;
>>   };
>>   
>>   static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
>> @@ -60,6 +62,14 @@ static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
>>       return (char *)block->host + offset;
>>   }
>>   
>> +static inline unsigned long int ramblock_recv_bitmap_offset(void *host_addr,
>> +                                                            RAMBlock *rb)
>> +{
>> +    uint64_t host_addr_offset =
>> +            (uint64_t)(uintptr_t)(host_addr - (void *)rb->host);
>> +    return host_addr_offset >> TARGET_PAGE_BITS;
>> +}
>> +
>>   long qemu_getrampagesize(void);
>>   unsigned long last_ram_page(void);
>>   RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index be497bb..7a414eb 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -560,22 +560,28 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>>   }
>>   
>>   static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
>> -        void *from_addr, uint64_t pagesize)
>> +                               void *from_addr, uint64_t pagesize, RAMBlock *rb)
>>   {
>> +    int ret;
>>       if (from_addr) {
>>           struct uffdio_copy copy_struct;
>>           copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
>>           copy_struct.src = (uint64_t)(uintptr_t)from_addr;
>>           copy_struct.len = pagesize;
>>           copy_struct.mode = 0;
>> -        return ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
>> +        ret = ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
>>       } else {
>>           struct uffdio_zeropage zero_struct;
>>           zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
>>           zero_struct.range.len = pagesize;
>>           zero_struct.mode = 0;
>> -        return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
>> +        ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
>> +    }
>> +    if (!ret) {
>> +        ramblock_recv_bitmap_set_range(rb, host_addr,
>> +                                       pagesize / qemu_target_page_size());
>>       }
>> +    return ret;
>>   }
>>   
>>   /*
>> @@ -592,7 +598,7 @@ int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>>        * which would be slightly cheaper, but we'd have to be careful
>>        * of the order of updating our page state.
>>        */
>> -    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize)) {
>> +    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
>>           int e = errno;
>>           error_report("%s: %s copy host: %p from: %p (size: %zd)",
>>                        __func__, strerror(e), host, from, pagesize);
>> @@ -614,7 +620,8 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
>>       trace_postcopy_place_page_zero(host);
>>   
>>       if (qemu_ram_pagesize(rb) == getpagesize()) {
>> -        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize())) {
>> +        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, NULL, getpagesize(),
>> +                                rb)) {
>>               int e = errno;
>>               error_report("%s: %s zero host: %p",
>>                            __func__, strerror(e), host);
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 9cc1b17..d14b8bb 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -44,6 +44,7 @@
>>   #include "qemu/error-report.h"
>>   #include "trace.h"
>>   #include "exec/ram_addr.h"
>> +#include "exec/target_page.h"
>>   #include "qemu/rcu_queue.h"
>>   #include "migration/colo.h"
>>   
>> @@ -147,6 +148,40 @@ out:
>>       return ret;
>>   }
>>   
>> +static void ramblock_recv_map_init(void)
>> +{
>> +    RAMBlock *rb;
>> +
>> +    RAMBLOCK_FOREACH(rb) {
>> +        assert(!rb->receivedmap);
>> +        rb->receivedmap = bitmap_new(rb->max_length >> qemu_target_page_bits());
>> +    }
>> +}
>> +
>> +int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr)
>> +{
>> +    return test_bit(ramblock_recv_bitmap_offset(host_addr, rb),
>> +                    rb->receivedmap);
>> +}
>> +
>> +void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr)
>> +{
>> +    set_bit_atomic(ramblock_recv_bitmap_offset(host_addr, rb), rb->receivedmap);
>> +}
>> +
>> +void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr,
>> +                                    size_t nr)
>> +{
>> +    bitmap_set_atomic(rb->receivedmap,
>> +                      ramblock_recv_bitmap_offset(host_addr, rb),
>> +                      nr);
>> +}
> Can you remember what the logic was behind making the 'set's atomic
> but not the clears?  It's probably at least worth a comment.
*bitmap_clear didn't use in my series and another 2, so now I think it's 
redundant function,
and it worth to remove it from patch.

> Other than that, I think this series is fine; it just needs one of the
> series that makes use of it to get it merged
> (although the 1st two can go in by themselves)
>
> Dave
>
>
>
>> +void ramblock_recv_bitmap_clear(RAMBlock *rb, void *host_addr)
>> +{
>> +    clear_bit(ramblock_recv_bitmap_offset(host_addr, rb), rb->receivedmap);
>> +}
>> +
>>   /*
>>    * An outstanding page request, on the source, having been received
>>    * and queued
>> @@ -1793,6 +1828,8 @@ int ram_discard_range(const char *rbname, uint64_t start, size_t length)
>>           goto err;
>>       }
>>   
>> +    bitmap_clear(rb->receivedmap, start >> qemu_target_page_bits(),
>> +                 length >> qemu_target_page_bits());
>>       ret = ram_block_discard_range(rb, start, length);
>>   
>>   err:
>> @@ -2324,13 +2361,20 @@ static int ram_load_setup(QEMUFile *f, void *opaque)
>>   {
>>       xbzrle_load_setup();
>>       compress_threads_load_setup();
>> +    ramblock_recv_map_init();
>>       return 0;
>>   }
>>   
>>   static int ram_load_cleanup(void *opaque)
>>   {
>> +    RAMBlock *rb;
>>       xbzrle_load_cleanup();
>>       compress_threads_load_cleanup();
>> +
>> +    RAMBLOCK_FOREACH(rb) {
>> +        g_free(rb->receivedmap);
>> +        rb->receivedmap = NULL;
>> +    }
>>       return 0;
>>   }
>>   
>> @@ -2545,6 +2589,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>                   ret = -EINVAL;
>>                   break;
>>               }
>> +            ramblock_recv_bitmap_set(block, host);
>>               trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
>>           }
>>   
>> diff --git a/migration/ram.h b/migration/ram.h
>> index c081fde..4db9922 100644
>> --- a/migration/ram.h
>> +++ b/migration/ram.h
>> @@ -52,4 +52,10 @@ 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);
>> +
>> +int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
>> +void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
>> +void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr, size_t nr);
>> +void ramblock_recv_bitmap_clear(RAMBlock *rb, void *host_addr);
>> +
>>   #endif
>> -- 
>> 1.9.1
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
>

-- 
Best regards,
Alexey Perevalov