Skip to contentSkip to conversation

Background snapshots

PATCH In review Denis Plotnikov 4 participants 34 messages Jul 18, 2018 – Sep 5, 2018

lore
Share & tools
lore
#0 lore raw

cover Background snapshots

The workflow to make a snapshot is the following:
1. Pause the vm
2. Make a snapshot of block devices using the scheme of your choice
3. Turn on background-snapshot migration capability
4. Start the migration using the destination (migration stream) of your choice. The migration will resume the vm execution by itself
when it has the devices' states saved and is ready to start ram writing to the migration stream.
5. Listen to the migration finish event

The bakground snapshot works with support of KVM patch:
"x86: mmu: report failed memory access to the userspace" (not applied to the mainstream, it's in the kvm mailing list)

--
Change log:

v0 => v1:
============
the patch series has been split in smaller chunks

Denis Plotnikov (17):
migration: add background snapshot capability
bitops: add some atomic versions of bitmap operations
threads: add infrastructure to process sigsegv
background snapshot: make a dedicated type for ram block list
ram: extend the data structures for background snapshotting background snapshot: add helpers to manage a copy of ram block list background snapshot: introduce page buffer
migration: add helpers to change VM memory protection rights background snapshot: extend RAM request for holding a page copy
pointer
background snapshots: adapt the page queueing code for using page
copies
background snapshot: add a memory page copying function
ram: add background snapshot support in ram page saving part of
migration
background snapshot: add write-protected page access handler function
kvm: add failed memeory access exit reason
kvm: add vCPU failed memeory access processing
migration: move the device state saving logic to a separate function background snapshot: enable background snapshot

 include/exec/ram_addr.h   |   7 +
 include/exec/ramlist.h    |   4 +-
 include/qemu/bitops.h     |  25 +++
 include/qemu/thread.h     |   5 +
 linux-headers/linux/kvm.h |   5 +
 migration/migration.c     | 140 +++++++++++++-
 migration/migration.h     |   1 +
 migration/ram.c           | 374 ++++++++++++++++++++++++++++++++++++--
 migration/ram.h           |  17 +-
 migration/savevm.c        |  91 +++++-----
 migration/savevm.h        |   2 +
 qapi/migration.json       |   6 +-
 target/i386/kvm.c         |  17 ++
 util/qemu-thread-posix.c  |  52 ++++++
 14 files changed, 684 insertions(+), 62 deletions(-)
Signature · 1 line
2.17.0
#3 lore raw

PATCH 2/17 bitops: add some atomic versions of bitmap operations

1. test bit
2. test and set bit

---
 include/qemu/bitops.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
include/qemu/bitops.h +25 -0
index 3f0926cf40..72afcfaec5 100644
@@ -94,6 +94,21 @@ static inline int test_and_set_bit(long nr, unsigned long *addr)
     return (old & mask) != 0;
 }
 
+/**
+ * test_and_set_bit_atomic - Set a bit atomically and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ */
+static inline int test_and_set_bit_atomic(long nr, unsigned long *addr)
+{
+    unsigned long mask = BIT_MASK(nr);
+    unsigned long *p = addr + BIT_WORD(nr);
+    unsigned long old;
+
+    old = atomic_fetch_or(p, mask);
+    return (old & mask) != 0;
+}
+
 /**
  * test_and_clear_bit - Clear a bit and return its old value
  * @nr: Bit to clear
@@ -134,6 +149,16 @@ static inline int test_bit(long nr, const unsigned long *addr)
     return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
 
+/**
+ * test_bit_atomic - Determine whether a bit is set atomicallly
+ * @nr: bit number to test
+ * @addr: Address to start counting from
+ */
+static inline int test_bit_atomic(long nr, const unsigned long *addr)
+{
+    long valid_nr = nr & (BITS_PER_LONG - 1);
+    return 1UL & (atomic_read(&addr[BIT_WORD(nr)]) >> valid_nr);
+}
 /**
  * find_last_bit - find the last set bit in a memory region
  * @addr: The address to start the search at
Signature · 1 line
2.17.0
#5 lore raw

PATCH 3/17 threads: add infrastructure to process sigsegv

In reply to Denis Plotnikov

Allows to define sigsegv handler temporary for all threads. This is useful to implement copy-on-write logic while linux usefaultfd doesn't support write-protected faults. In the future, switch to using WP userfaultfd when it's available.

---
 include/qemu/thread.h    |  5 ++++
 util/qemu-thread-posix.c | 52 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
include/qemu/thread.h +5 -0
index 9910f49b3a..d6fed833fa 100644
@@ -210,4 +210,9 @@ void qemu_lockcnt_inc_and_unlock(QemuLockCnt *lockcnt);
  */
 unsigned qemu_lockcnt_count(QemuLockCnt *lockcnt);
 
+
+typedef void (*sigsegv_handler)(int signum, siginfo_t *siginfo, void *sigctx);
+void sigsegv_user_handler_set(sigsegv_handler handler);
+void sigsegv_user_handler_reset(void);
+
 #endif
util/qemu-thread-posix.c +52 -0

72 lines not on this page · raw message

Signature · 1 line
2.17.0
#7 lore raw

PATCH 4/17 background snapshot: make a dedicated type for ram block list

In reply to Denis Plotnikov

The type will be used later to hold a list of ram blocks which memory content to be used for the vm state saving when making a background snapshot.

---
 include/exec/ramlist.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
include/exec/ramlist.h +3 -1
index 2e2ac6cb99..e0231d3bec 100644
@@ -44,11 +44,13 @@ typedef struct {
     unsigned long *blocks[];
 } DirtyMemoryBlocks;
 
+typedef QLIST_HEAD(, RAMBlock) RamBlockList;
+
 typedef struct RAMList {
     QemuMutex mutex;
     RAMBlock *mru_block;
     /* RCU-enabled, writes protected by the ramlist lock. */
-    QLIST_HEAD(, RAMBlock) blocks;
+    RamBlockList blocks;
     DirtyMemoryBlocks *dirty_memory[DIRTY_MEMORY_NUM];
     uint32_t version;
     QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
Signature · 1 line
2.17.0
#8 lore raw

PATCH 5/17 ram: extend the data structures for background snapshotting

In reply to Denis Plotnikov
---
 include/exec/ram_addr.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/exec/ram_addr.h +7 -0
index 6cbc02aa0f..5b403d537d 100644
@@ -36,6 +36,8 @@ struct RAMBlock {
     char idstr[256];
     /* RCU-enabled, writes protected by the ramlist lock */
     QLIST_ENTRY(RAMBlock) next;
+    /* blocks used for background snapshot */
+    QLIST_ENTRY(RAMBlock) bgs_next;
     QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
     int fd;
     size_t page_size;
@@ -49,6 +51,11 @@ struct RAMBlock {
     unsigned long *unsentmap;
     /* bitmap of already received pages in postcopy */
     unsigned long *receivedmap;
+    /* The following 2 are for background snapshot */
+    /* Pages currently being copied */
+    unsigned long *touched_map;
+    /* Pages has been copied already */
+    unsigned long *copied_map;
 };
 
 static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
Signature · 1 line
2.17.0
#10 lore raw

PATCH 6/17 background snapshot: add helpers to manage a copy of ram block list

In reply to Denis Plotnikov

The VM ramblock list may be changed during the snapshotting. We want to make sure that we have the same ramblock list as it was at the time of snapshot beginning.
So, we create a copy of the list at the beginning of the snapshotting and use it during the process.

---
 migration/ram.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  6 ++++++
 2 files changed, 57 insertions(+)
migration/ram.c +51 -0
index 021d583b9b..4399c31606 100644
@@ -1508,6 +1508,57 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     return pages;
 }
 
+/* BackGround Snapshot Blocks */
+static RamBlockList bgs_blocks;
+
+static RamBlockList *ram_bgs_block_list_get(void)
+{
+    return &bgs_blocks;
+}
+
+void ram_block_list_create(void)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+
+    qemu_mutex_lock_ramlist();
+    QLIST_FOREACH(block, &ram_list.blocks, next) {
+        memory_region_ref(block->mr);
+        QLIST_INSERT_HEAD(block_list, block, bgs_next);
+    }
+    qemu_mutex_unlock_ramlist();
+}
+
+void ram_block_list_destroy(void)
+{
+    RAMBlock *next, *block;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+
+    QLIST_FOREACH_SAFE(block, block_list, bgs_next, next) {
+        QLIST_REMOVE(block, bgs_next);
+        memory_region_unref(block->mr);
+    }
+}
+
+RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+
+    QLIST_FOREACH(block, block_list, bgs_next) {
+        /* This case append when the block is not mapped. */
+        if (block->host == NULL) {
+            continue;
+        }
+
+        if (address - block->host < block->max_length) {
+            *page_offset = (address - block->host) & TARGET_PAGE_MASK;
+            return block;
+        }
+    }
+
+    return NULL;
+}
 /**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *
migration/ram.h +6 -0
index 64d81e9f1d..385579cae9 100644
@@ -31,6 +31,7 @@
 
 #include "qemu-common.h"
 #include "exec/cpu-common.h"
+#include "exec/ramlist.h"
 
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
@@ -61,5 +62,10 @@ 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);
+/* For background snapshot */
+void ram_block_list_create(void);
+void ram_block_list_destroy(void);
+
+RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
 
 #endif
Signature · 1 line
2.17.0
#17 lore raw

PATCH 9/17 background snapshot: extend RAM request for holding a page copy pointer

In reply to Denis Plotnikov

This pointer is going to be used to transfer a memory.
Once the memory page is copied the content the snapshot interested in is saved for writing and we can make the page writable again.

---
 migration/ram.c | 3 +++
 1 file changed, 3 insertions(+)
migration/ram.c +3 -0
index ce3dead932..dc7dfe0726 100644
@@ -188,6 +188,7 @@ struct RAMSrcPageRequest {
     RAMBlock *rb;
     hwaddr    offset;
     hwaddr    len;
+    void     *page_copy;
 
     QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
 };
@@ -265,6 +266,8 @@ struct PageSearchStatus {
     unsigned long page;
     /* Set once we wrap around */
     bool         complete_round;
+    /* Pointer to the cached page */
+    void *page_copy;
 };
 typedef struct PageSearchStatus PageSearchStatus;
 
Signature · 1 line
2.17.0
#22 lore raw

PATCH 12/17 ram: add background snapshot support in ram page saving part of migration

In reply to Denis Plotnikov

Unify the function saving the ram pages for using in both the migration and the background snapshots.

---
 migration/ram.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
migration/ram.c +17 -0
index 10b6fdf23e..b1623e96e7 100644
@@ -1803,11 +1803,28 @@ static int ram_find_and_save_block(RAMState *rs, bool last_stage)
         if (!found) {
             /* priority queue empty, so just search for something dirty */
             found = find_dirty_block(rs, &pss, &again);
+
+            if (found && migrate_background_snapshot()) {
+                /* make a copy of the page and
+                 * pass it to the page search status */
+                int ret;
+                ret = ram_copy_page(pss.block, pss.page, &pss.page_copy);
+                if (ret == 0) {
+                    found = false;
+                    pages = 0;
+                } else if (ret < 0) {
+                    return ret;
+                }
+            }
         }
 
         if (found) {
             pages = ram_save_host_page(rs, &pss, last_stage);
         }
+
+        if (pss.page_copy) {
+            ram_page_buffer_decrease_used();
+        }
     } while (!pages && again);
 
     rs->last_seen_block = pss.block;
Signature · 1 line
2.17.0
#23 lore raw

PATCH 13/17 background snapshot: add write-protected page access handler function

In reply to Denis Plotnikov

The handler does all the necessary operations to save the page being accessed to the snapshot file(stream).

---
 migration/ram.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  1 +
 2 files changed, 44 insertions(+)
migration/ram.c +43 -0
index b1623e96e7..04a4bf708d 100644
@@ -1762,6 +1762,49 @@ int ram_copy_page(RAMBlock *block, unsigned long page_nr,
     return 1;
 }
 
+/**
+ * ram_process_page_fault
+ *
+ * Used in the background snapshot to queue the copy of the memory
+ * page for writing.
+ *
+ * Returns:
+ *    0 > - on error
+ *    0   - success
+ *
+ * @address:  guest address
+ *
+ */
+int ram_process_page_fault(void *address)
+{
+    int ret;
+    void *page_copy = NULL;
+    unsigned long page_nr;
+    ram_addr_t offset;
+
+    RAMBlock *block = ram_bgs_block_find(address, &offset);
+
+    if (!block) {
+        return -1;
+    }
+
+    page_nr = offset >> TARGET_PAGE_BITS;
+
+    ret = ram_copy_page(block, page_nr, &page_copy);
+
+    if (ret < 0) {
+        return ret;
+    } else if (ret > 0) {
+        if (ram_save_queue_pages(block, NULL, offset,
+                                 TARGET_PAGE_SIZE, page_copy)) {
+            ram_page_buffer_free(page_copy);
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 /**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *
migration/ram.h +1 -0
index 76ab0a3377..2b565ce620 100644
@@ -76,4 +76,5 @@ int ram_block_list_set_readonly(void);
 int ram_block_list_set_writable(void);
 
 int ram_copy_page(RAMBlock *block, unsigned long page_nr, void **page_copy);
+int ram_process_page_fault(void *address);
 #endif
Signature · 1 line
2.17.0
#24 lore raw

PATCH 14/17 kvm: add failed memeory access exit reason

In reply to Denis Plotnikov

The patch allows qemu to be aware of how to read kvm failed memeory access.

This is a temporary patch. It sould be removed on importing the kvm failed memeory access exit from the linux branch.

---
 linux-headers/linux/kvm.h | 5 +++++
 1 file changed, 5 insertions(+)
linux-headers/linux/kvm.h +5 -0
index d92c9b2f0e..ea63b681d8 100644
@@ -235,6 +235,7 @@ struct kvm_hyperv_exit {
 #define KVM_EXIT_S390_STSI        25
 #define KVM_EXIT_IOAPIC_EOI       26
 #define KVM_EXIT_HYPERV           27
+#define KVM_EXIT_FAIL_MEM_ACCESS  28
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
@@ -392,6 +393,10 @@ struct kvm_run {
 		} eoi;
 		/* KVM_EXIT_HYPERV */
 		struct kvm_hyperv_exit hyperv;
+		/* KVM_EXIT_FAIL_MEM_ACCESS */
+		struct {
+			__u64 hva;
+		} fail_mem_access;
 		/* Fix the size of the union. */
 		char padding[256];
 	};
Signature · 1 line
2.17.0
#25 lore raw

PATCH 15/17 kvm: add vCPU failed memeory access processing

In reply to Denis Plotnikov

Is done with support of the KVM patch returning the faulting address.

---
 target/i386/kvm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
target/i386/kvm.c +17 -0
index 3ac5302bc5..55b8860d1a 100644
@@ -45,6 +45,8 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "migration/blocker.h"
+#include "migration/savevm.h"
+#include "migration/ram.h"
 #include "exec/memattrs.h"
 #include "trace.h"
 
@@ -3130,6 +3132,18 @@ static bool host_supports_vmx(void)
     return ecx & CPUID_EXT_VMX;
 }
 
+static int kvm_handle_fail_mem_access(CPUState *cpu)
+{
+    struct kvm_run *run = cpu->kvm_run;
+    int ret = ram_process_page_fault((void *)run->fail_mem_access.hva);
+
+    if (ret >= 0) {
+        cpu_resume(cpu);
+    }
+
+    return ret;
+}
+
 #define VMX_INVALID_GUEST_STATE 0x80000021
 
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
@@ -3188,6 +3202,9 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
         ioapic_eoi_broadcast(run->eoi.vector);
         ret = 0;
         break;
+    case KVM_EXIT_FAIL_MEM_ACCESS:
+        ret = kvm_handle_fail_mem_access(cs);
+        break;
     default:
         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
         ret = -1;
Signature · 1 line
2.17.0
#1 lore raw

PATCH 1/17 migration: add background snapshot capability

In reply to Denis Plotnikov

The capability is used for the background vmstate saving using the migration infrastructure.
Background vmstate saving means that the majority of vmstate (RAM) is saved in the background when VM's vCPUS are running. This helps to reduce the VM downtime on VM snapshotting.

---
 migration/migration.c | 35 +++++++++++++++++++++++++++++++++++
 migration/migration.h |  1 +
 qapi/migration.json   |  6 +++++-
 3 files changed, 41 insertions(+), 1 deletion(-)
migration/migration.c +35 -0
index d780601f0c..87096d23ef 100644
@@ -697,6 +697,12 @@ static bool migrate_caps_check(bool *cap_list,
             return false;
         }
 
+        if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
+            error_setg(errp, "Postcopy is not compatible "
+                        "with background snapshot");
+            return false;
+        }
+
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
          * special support.
@@ -711,6 +717,26 @@ static bool migrate_caps_check(bool *cap_list,
         }
     }
 
+    if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
+        if (cap_list[MIGRATION_CAPABILITY_RELEASE_RAM]) {
+            error_setg(errp, "Background snapshot is not compatible "
+                        "with release ram capability");
+            return false;
+        }
+
+        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
+            error_setg(errp, "Background snapshot is not "
+                        "currently compatible with compression");
+            return false;
+        }
+
+        if (cap_list[MIGRATION_CAPABILITY_XBZRLE]) {
+            error_setg(errp, "Background snapshot is not "
+                        "currently compatible with XBZLRE");
+            return false;
+        }
+    }
+
     return true;
 }
 
@@ -1635,6 +1661,15 @@ bool migrate_use_block_incremental(void)
     return s->parameters.block_incremental;
 }
 
+bool migrate_background_snapshot(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
migration/migration.h +1 -0
index 663415fe48..21babbc008 100644
@@ -201,6 +201,7 @@ int migrate_compress_level(void);
 int migrate_compress_threads(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
+bool migrate_background_snapshot(void);
 
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_shut(MigrationIncomingState *mis,
qapi/migration.json +5 -1
index 03f57c9616..eb5f52b7ff 100644
@@ -352,12 +352,16 @@
 #
 # @x-multifd: Use more than one fd for migration (since 2.11)
 #
+# @background-snapshot: Using migration infrastructure makes VM snapshot
+#         saving its RAM in background. This reduces  VM downtime. (since 2.12)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
-           'block', 'return-path', 'pause-before-switchover', 'x-multifd' ] }
+           'block', 'return-path', 'pause-before-switchover', 'x-multifd',
+           'background-snapshot' ] }
 
 ##
 # @MigrationCapabilityStatus:
Signature · 1 line
2.17.0
#12 lore raw

PATCH 7/17 background snapshot: introduce page buffer

In reply to Denis Plotnikov

To limit the amount of memory used by the background snapshot a memory limiter is used which called "page buffer".
In fact, it's a memory page counter limiting the page allocation. Currently, the limit of pages is hardcoded but its setter is the matter of the future work.

The background snapshot makes a copy of the page before writing it to the snapshot destination, but it doesn't use a preallocated buffer, because the background snapshot employs the migration infrastructure which, in turn, uses madvice to release the buffer.
The advice issuing is hard to track, so here we rely on anonymous memory mapping and releasing it by the existing code.

---
 migration/ram.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  3 ++
 2 files changed, 94 insertions(+)
migration/ram.c +91 -0
index 4399c31606..27d3403435 100644
@@ -192,6 +192,16 @@ struct RAMSrcPageRequest {
     QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
 };
 
+/* Page buffer used for background snapshot */
+typedef struct RAMPageBuffer {
+    /* Page buffer capacity in host pages */
+    int capacity;
+    /* Current number of pages in the buffer */
+    int used;
+    /* Event to notify that buffer usage is under capacity */
+    QemuEvent used_decreased;
+} RAMPageBuffer;
+
 /* State of RAM for migration */
 struct RAMState {
     /* QEMUFile used for this migration */
@@ -230,6 +240,11 @@ struct RAMState {
     /* Queue of outstanding page requests from the destination */
     QemuMutex src_page_req_mutex;
     QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
+    /* The following 2 are for background snapshot */
+    /* Buffer data to store copies of ram pages while async vm saving */
+    RAMPageBuffer page_buffer;
+    /* Event to notify that a page copying just has finished*/
+    QemuEvent page_copying_done;
 };
 typedef struct RAMState RAMState;
 
@@ -1540,6 +1555,52 @@ void ram_block_list_destroy(void)
     }
 }
 
+static void ram_page_buffer_decrease_used(void)
+{
+    qemu_event_reset(&ram_state->page_buffer.used_decreased);
+    atomic_dec(&ram_state->page_buffer.used);
+    qemu_event_set(&ram_state->page_buffer.used_decreased);
+}
+
+static void ram_page_buffer_increase_used_wait(void)
+{
+    int used, *used_ptr;
+    RAMState *rs = ram_state;
+    used_ptr = &rs->page_buffer.used;
+    do {
+        used = atomic_read(used_ptr);
+        if (rs->page_buffer.capacity > used) {
+            if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
+                return;
+            } else {
+                continue;
+            }
+        } else {
+            qemu_event_wait(&rs->page_buffer.used_decreased);
+        }
+    } while (true);
+}
+
+void *ram_page_buffer_get(void)
+{
+    void *page;
+    ram_page_buffer_increase_used_wait();
+    page = mmap(0, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
+                MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+    if (page == MAP_FAILED) {
+        ram_page_buffer_decrease_used();
+        page = NULL;
+    }
+    return page;
+}
+
+int ram_page_buffer_free(void *buffer)
+{
+    ram_page_buffer_decrease_used();
+    return qemu_madvise(buffer, TARGET_PAGE_SIZE, MADV_DONTNEED);
+}
+
 RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
 {
     RAMBlock *block = NULL;
@@ -1559,6 +1620,7 @@ RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
 
     return NULL;
 }
+
 /**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *
@@ -1651,6 +1713,13 @@ static void xbzrle_load_cleanup(void)
 
 static void ram_state_cleanup(RAMState **rsp)
 {
+    if (migrate_background_snapshot()) {
+        qemu_event_destroy(&(*rsp)->page_buffer.used_decreased);
+        /* In case somebody is still waiting for the event */
+        qemu_event_set(&(*rsp)->page_copying_done);
+        qemu_event_destroy(&(*rsp)->page_copying_done);
+    }
+
     migration_page_queue_free(*rsp);
     qemu_mutex_destroy(&(*rsp)->bitmap_mutex);
     qemu_mutex_destroy(&(*rsp)->src_page_req_mutex);
@@ -1689,6 +1758,13 @@ static void ram_save_cleanup(void *opaque)
         block->bmap = NULL;
         g_free(block->unsentmap);
         block->unsentmap = NULL;
+
+        if (migrate_background_snapshot()) {
+            g_free(block->touched_map);
+            block->touched_map = NULL;
+            g_free(block->copied_map);
+            block->copied_map = NULL;
+        }
     }
 
     xbzrle_cleanup();
@@ -1703,6 +1779,10 @@ static void ram_state_reset(RAMState *rs)
     rs->last_page = 0;
     rs->last_version = ram_list.version;
     rs->ram_bulk_stage = true;
+
+    /* page buffer capacity in number of pages */
+    rs->page_buffer.capacity = 1000;
+    rs->page_buffer.used = 0;
 }
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
@@ -2180,6 +2260,11 @@ static int ram_state_init(RAMState **rsp)
      */
     (*rsp)->migration_dirty_pages = ram_bytes_total() >> TARGET_PAGE_BITS;
 
+    if (migrate_background_snapshot()) {
+        qemu_event_init(&ram_state->page_buffer.used_decreased, false);
+        qemu_event_init(&ram_state->page_copying_done, false);
+    }
+
     ram_state_reset(*rsp);
 
     return 0;
@@ -2196,10 +2281,16 @@ static void ram_list_init_bitmaps(void)
             pages = block->max_length >> TARGET_PAGE_BITS;
             block->bmap = bitmap_new(pages);
             bitmap_set(block->bmap, 0, pages);
+
             if (migrate_postcopy_ram()) {
                 block->unsentmap = bitmap_new(pages);
                 bitmap_set(block->unsentmap, 0, pages);
             }
+
+            if (migrate_background_snapshot()) {
+                block->touched_map = bitmap_new(pages);
+                block->copied_map = bitmap_new(pages);
+            }
         }
     }
 }
migration/ram.h +3 -0
index 385579cae9..7c802c1d7f 100644
@@ -68,4 +68,7 @@ void ram_block_list_destroy(void);
 
 RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
 
+void *ram_page_buffer_get(void);
+int ram_page_buffer_free(void *buffer);
+
 #endif
Signature · 1 line
2.17.0
#15 lore raw

PATCH 8/17 migration: add helpers to change VM memory protection rights

In reply to Denis Plotnikov
---
 migration/ram.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  3 +++
 2 files changed, 57 insertions(+)
migration/ram.c +54 -0
index 27d3403435..ce3dead932 100644
@@ -1555,6 +1555,60 @@ void ram_block_list_destroy(void)
     }
 }
 
+static int mem_protect(void *addr, uint64_t length, int prot)
+{
+    int ret = mprotect(addr, length, prot);
+
+    if (ret < 0) {
+        error_report("%s: Can't change protection on ram block at %p",
+                     __func__, addr);
+    }
+
+    return ret;
+}
+
+static int ram_set_ro(void *addr, uint64_t length)
+{
+    return mem_protect(addr, length, PROT_READ);
+}
+
+static int ram_set_rw(void *addr, uint64_t length)
+{
+    return mem_protect(addr, length, PROT_READ | PROT_WRITE);
+}
+
+int ram_block_list_set_readonly(void)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+    int ret = 0;
+
+    QLIST_FOREACH(block, block_list, bgs_next) {
+        ret = ram_set_ro(block->host, block->used_length);
+        if (ret) {
+            break;
+        }
+    }
+
+    return ret;
+}
+
+int ram_block_list_set_writable(void)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+    int ret = 0;
+
+    QLIST_FOREACH(block, block_list, bgs_next) {
+        ret = ram_set_rw(block->host, block->used_length);
+        if (ret) {
+            break;
+        }
+    }
+
+    return ret;
+}
+
 static void ram_page_buffer_decrease_used(void)
 {
     qemu_event_reset(&ram_state->page_buffer.used_decreased);
migration/ram.h +3 -0
index 7c802c1d7f..4c463597a5 100644
@@ -71,4 +71,7 @@ RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
 void *ram_page_buffer_get(void);
 int ram_page_buffer_free(void *buffer);
 
+int ram_block_list_set_readonly(void);
+int ram_block_list_set_writable(void);
+
 #endif
Signature · 1 line
2.17.0
#18 lore raw

PATCH 10/17 background snapshots: adapt the page queueing code for using page copies

In reply to Denis Plotnikov

The background snapshot uses memeory page copying to seal the page memory content. The patch adapts the migration infrastructure to save copies of the pages.

---
 migration/migration.c |  2 +-
 migration/ram.c       | 59 ++++++++++++++++++++++++++++++++-----------
 migration/ram.h       |  3 ++-
 3 files changed, 47 insertions(+), 17 deletions(-)
migration/migration.c +1 -1
index 87096d23ef..131d0904e4 100644
@@ -1716,7 +1716,7 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
         return;
     }
 
-    if (ram_save_queue_pages(rbname, start, len)) {
+    if (ram_save_queue_pages(NULL, rbname, start, len, NULL)) {
         mark_source_rp_bad(ms);
     }
 }
migration/ram.c +44 -15

141 lines not on this page · raw message

migration/ram.h +2 -1
index 4c463597a5..c3679ba65e 100644
@@ -46,7 +46,8 @@ int multifd_load_setup(void);
 int multifd_load_cleanup(Error **errp);
 
 uint64_t ram_pagesize_summary(void);
-int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
+int ram_save_queue_pages(RAMBlock *block, const char *rbname,
+                         ram_addr_t start, ram_addr_t len, void *page_copy);
 void acct_update_position(QEMUFile *f, size_t size, bool zero);
 void ram_debug_dump_bitmap(unsigned long *todump, bool expected,
                            unsigned long pages);
Signature · 1 line
2.17.0
#21 lore raw

PATCH 11/17 background snapshot: add a memory page copying function

In reply to Denis Plotnikov

It's the only function making a memory page copy. It supports multithreading semantics ensuring that the page is copied by one thread only and releasing the copied page from write protection.

---
 migration/ram.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  1 +
 2 files changed, 57 insertions(+)
migration/ram.c +56 -0
index 3c4ccd85b4..10b6fdf23e 100644
@@ -1706,6 +1706,62 @@ RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
     return NULL;
 }
 
+/**
+ * ram_copy_page: make a page copy
+ *
+ * Used in the background snapshot to make a copy of a memeory page.
+ * Ensures that the memeory page is copied only once.
+ * When a page copy is done, restores read/write access to the memory
+ * page.
+ * If a page is being copied by another thread, wait until the copying
+ * ends and exit.
+ *
+ * Returns:
+ *   -1 - on error
+ *    0 - the page wasn't copied by the function call
+ *    1 - the page has been copied
+ *
+ * @block:     RAM block to use
+ * @page_nr:   the page number to copy
+ * @page_copy: the pointer to return a page copy
+ *
+ */
+int ram_copy_page(RAMBlock *block, unsigned long page_nr,
+                          void **page_copy)
+{
+    void *host_page;
+
+    if (test_and_set_bit_atomic(page_nr, block->touched_map)) {
+        while (!test_bit_atomic(page_nr, block->copied_map)) {
+            /* the page is being copied -- wait for the end of the copying
+             * and check once again
+             */
+            qemu_event_wait(&ram_state->page_copying_done);
+        }
+        return 0;
+    }
+
+    *page_copy = ram_page_buffer_get();
+    if (!*page_copy) {
+        return -1;
+    }
+
+    host_page = block->host + (page_nr << TARGET_PAGE_BITS);
+    memcpy(*page_copy, host_page, TARGET_PAGE_SIZE);
+
+    if (ram_set_rw(host_page, TARGET_PAGE_SIZE)) {
+        ram_page_buffer_free(*page_copy);
+        *page_copy = NULL;
+        return -1;
+    }
+
+    set_bit_atomic(page_nr, block->copied_map);
+    qemu_event_set(&ram_state->page_copying_done);
+    qemu_event_reset(&ram_state->page_copying_done);
+
+    return 1;
+}
+
 /**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *
migration/ram.h +1 -0
index c3679ba65e..76ab0a3377 100644
@@ -75,4 +75,5 @@ int ram_page_buffer_free(void *buffer);
 int ram_block_list_set_readonly(void);
 int ram_block_list_set_writable(void);
 
+int ram_copy_page(RAMBlock *block, unsigned long page_nr, void **page_copy);
 #endif
Signature · 1 line
2.17.0
#27 lore raw

PATCH 16/17 migration: move the device state saving logic to a separate function

In reply to Denis Plotnikov

The logic being split will be reused by the background snapshot.

---
 migration/savevm.c | 91 +++++++++++++++++++++++++---------------------
 migration/savevm.h |  2 +
 2 files changed, 52 insertions(+), 41 deletions(-)
migration/savevm.c +50 -41
index f202c3de3a..36074ec9de 100644
@@ -1107,51 +1107,15 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
     qemu_fflush(f);
 }
 
-int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
-                                       bool inactivate_disks)
+int qemu_savevm_state_save(QEMUFile *f, bool inactivate_disks,
+                           bool send_eof)
 {
-    QJSON *vmdesc;
     int vmdesc_len;
     SaveStateEntry *se;
     int ret;
-    bool in_postcopy = migration_in_postcopy();
-
-    trace_savevm_state_complete_precopy();
+    QJSON *vmdesc = qjson_new();
 
     cpu_synchronize_all_states();
-
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!se->ops ||
-            (in_postcopy && se->ops->has_postcopy &&
-             se->ops->has_postcopy(se->opaque)) ||
-            (in_postcopy && !iterable_only) ||
-            !se->ops->save_live_complete_precopy) {
-            continue;
-        }
-
-        if (se->ops && se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
-                continue;
-            }
-        }
-        trace_savevm_section_start(se->idstr, se->section_id);
-
-        save_section_header(f, se, QEMU_VM_SECTION_END);
-
-        ret = se->ops->save_live_complete_precopy(f, se->opaque);
-        trace_savevm_section_end(se->idstr, se->section_id, ret);
-        save_section_footer(f, se);
-        if (ret < 0) {
-            qemu_file_set_error(f, ret);
-            return -1;
-        }
-    }
-
-    if (iterable_only) {
-        return 0;
-    }
-
-    vmdesc = qjson_new();
     json_prop_int(vmdesc, "page_size", qemu_target_page_size());
     json_start_array(vmdesc, "devices");
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
@@ -1193,8 +1157,8 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
             return ret;
         }
     }
-    if (!in_postcopy) {
-        /* Postcopy stream will still be going */
+
+    if (send_eof) {
         qemu_put_byte(f, QEMU_VM_EOF);
     }
 
@@ -1213,6 +1177,51 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
     return 0;
 }
 
+int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
+                                       bool inactivate_disks)
+{
+    SaveStateEntry *se;
+    int ret;
+    bool in_postcopy = migration_in_postcopy();
+
+    trace_savevm_state_complete_precopy();
+
+    cpu_synchronize_all_states();
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        if (!se->ops ||
+            (in_postcopy && se->ops->has_postcopy &&
+             se->ops->has_postcopy(se->opaque)) ||
+            (in_postcopy && !iterable_only) ||
+            !se->ops->save_live_complete_precopy) {
+            continue;
+        }
+
+        if (se->ops && se->ops->is_active) {
+            if (!se->ops->is_active(se->opaque)) {
+                continue;
+            }
+        }
+        trace_savevm_section_start(se->idstr, se->section_id);
+
+        save_section_header(f, se, QEMU_VM_SECTION_END);
+
+        ret = se->ops->save_live_complete_precopy(f, se->opaque);
+        trace_savevm_section_end(se->idstr, se->section_id, ret);
+        save_section_footer(f, se);
+        if (ret < 0) {
+            qemu_file_set_error(f, ret);
+            return -1;
+        }
+    }
+
+    if (iterable_only) {
+        return 0;
+    }
+
+    return qemu_savevm_state_save(f, inactivate_disks, !in_postcopy);
+}
+
 /* Give an estimate of the amount left to be transferred,
  * the result is split into the amount for units that can and
  * for units that can't do postcopy.
migration/savevm.h +2 -0
index 295c4a1f2c..0e8d7aecf8 100644
@@ -40,6 +40,8 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
 void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
                                uint64_t *res_non_postcopiable,
                                uint64_t *res_postcopiable);
+int qemu_savevm_state_save(QEMUFile *f, bool inactivate_disks,
+                           bool send_eof);
 void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
 void qemu_savevm_send_open_return_path(QEMUFile *f);
 int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
Signature · 1 line
2.17.0
#28 lore raw

PATCH 17/17 background snapshot: enable background snapshot

In reply to Denis Plotnikov

The patch enables to save vmstate to a migration thread in the background: ram is being saved while vCPUs are running. This is done to reduce downtime on vm snapshotting: the majority of vmstate is ram, the rest of devices consumes only a few MB of memory on a typical vm.
By this moment, there were no means to make a snapshot without full vm stopping. From now, one can save a vm state having the vm stopped only for a couple of seconds to save devices' states. Then the vm resumed and ram is written without the vm full stopping.

To use async vm state it's needed to enable "background-snapshot" migration capability and start the migration

---
 migration/migration.c | 103 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 2 deletions(-)
migration/migration.c +101 -2
index 131d0904e4..935e5b6f2e 100644
@@ -2188,6 +2188,99 @@ bool migrate_colo_enabled(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
 }
 
+static void background_snapshot_sigsegv_handler(int unused0, siginfo_t *info,
+                                                void *unused1)
+{
+    if (ram_process_page_fault(info->si_addr)) {
+        assert(false);
+    }
+}
+
+static void *background_snapshot_thread(void *opaque)
+{
+    MigrationState *m = opaque;
+    QIOChannelBuffer *bioc;
+    QEMUFile *fb;
+    int res = 0;
+
+    rcu_register_thread();
+
+    qemu_file_set_rate_limit(m->to_dst_file, INT64_MAX);
+
+    qemu_mutex_lock_iothread();
+    vm_stop(RUN_STATE_PAUSED);
+
+    qemu_savevm_state_header(m->to_dst_file);
+    qemu_mutex_unlock_iothread();
+    qemu_savevm_state_setup(m->to_dst_file);
+    qemu_mutex_lock_iothread();
+
+    migrate_set_state(&m->state, MIGRATION_STATUS_SETUP,
+                      MIGRATION_STATUS_ACTIVE);
+
+    /* save the device state to the temporary buffer */
+    bioc = qio_channel_buffer_new(4096);
+    qio_channel_set_name(QIO_CHANNEL(bioc), "vmstate-buffer");
+    fb = qemu_fopen_channel_output(QIO_CHANNEL(bioc));
+    object_unref(OBJECT(bioc));
+
+    ram_block_list_create();
+    ram_block_list_set_readonly();
+
+    if (global_state_store()) {
+        goto exit;
+    }
+
+    if (qemu_savevm_state_save(fb, false, true) < 0) {
+        migrate_set_state(&m->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
+        goto exit;
+    }
+
+    sigsegv_user_handler_set(background_snapshot_sigsegv_handler);
+
+    vm_start();
+    qemu_mutex_unlock_iothread();
+
+    while (!res) {
+        res = qemu_savevm_state_iterate(m->to_dst_file, false);
+
+        if (res < 0 || qemu_file_get_error(m->to_dst_file)) {
+            migrate_set_state(&m->state, MIGRATION_STATUS_ACTIVE,
+                              MIGRATION_STATUS_FAILED);
+            goto exit;
+        }
+    }
+
+    /*
+     * By this moment we have RAM saved into the stream
+     * The next step is to flush the device state right after the
+     * RAM saved. The rest of device state is stored in
+     * the temporary buffer. Do flush the buffer into the stream.
+     */
+    qemu_put_buffer(m->to_dst_file, bioc->data, bioc->usage);
+    qemu_fflush(m->to_dst_file);
+
+    if (qemu_file_get_error(m->to_dst_file)) {
+        migrate_set_state(&m->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
+        goto exit;
+    }
+
+    migrate_set_state(&m->state, MIGRATION_STATUS_ACTIVE,
+                                 MIGRATION_STATUS_COMPLETED);
+exit:
+    ram_block_list_set_writable();
+    ram_block_list_destroy();
+    sigsegv_user_handler_reset();
+    qemu_fclose(fb);
+    qemu_mutex_lock_iothread();
+    qemu_savevm_state_cleanup();
+    qemu_mutex_unlock_iothread();
+    rcu_unregister_thread();
+    return NULL;
+}
+
 /*
  * Master migration thread on the source VM.
  * It drives the migration and pumps the data down the outgoing channel.
@@ -2402,8 +2495,14 @@ void migrate_fd_connect(MigrationState *s)
         migrate_fd_cleanup(s);
         return;
     }
-    qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
-                       QEMU_THREAD_JOINABLE);
+    if (migrate_background_snapshot()) {
+        qemu_thread_create(&s->thread, "bg_snapshot",
+                           background_snapshot_thread, s,
+                           QEMU_THREAD_JOINABLE);
+    } else {
+        qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
+                           QEMU_THREAD_JOINABLE);
+    }
     s->migration_thread_running = true;
 }
 
Signature · 1 line
2.17.0
#6 lore raw

Re: [Qemu-devel] [PATCH v1 03/17] threads: add infrastructure to process sigsegv

In reply to Denis Plotnikov
Quoted 31 lines · Denis Plotnikov

Allows to define sigsegv handler temporary for all threads. This is useful to implement copy-on-write logic while linux usefaultfd doesn't support write-protected faults. In the future, switch to using WP userfaultfd when it's available.

---
 include/qemu/thread.h    |  5 ++++
 util/qemu-thread-posix.c | 52 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
include/qemu/thread.h +5 -0
index 9910f49b3a..d6fed833fa 100644
@@ -210,4 +210,9 @@ void qemu_lockcnt_inc_and_unlock(QemuLockCnt *lockcnt);
  */
 unsigned qemu_lockcnt_count(QemuLockCnt *lockcnt);
 
+
+typedef void (*sigsegv_handler)(int signum, siginfo_t *siginfo, void *sigctx);
+void sigsegv_user_handler_set(sigsegv_handler handler);
+void sigsegv_user_handler_reset(void);
+
 #endif
util/qemu-thread-posix.c +0 -0
index 7306475899..5424b7106d 100644

util/qemu-thread-posix.c static void qemu_thread_set_name(QemuThread *thread, const char *name) · line 496

Quoted 8 lines
util/qemu-thread-posix.c +5 -0
@@ -489,6 +489,47 @@ static void qemu_thread_set_name(QemuThread *thread, const char *name)
 #endif
 }
 
+static sigsegv_handler sigsegv_user_handler;
+
+void sigsegv_user_handler_set(sigsegv_handler handler)
+{
+    assert(handler);

We can also assert !sigsegv_user_handler here to make sure there won't accidentally be two users that want to specify a SIGSEGV handler in parallel.

Quoted 17 lines
util/qemu-thread-posix.c +17 -0
+    atomic_set(&sigsegv_user_handler, handler);
+}
+
+static sigsegv_handler sigsegv_user_handler_get(void)
+{
+    return atomic_read(&sigsegv_user_handler);
+}
+
+void sigsegv_user_handler_reset(void)
+{
+    atomic_set(&sigsegv_user_handler, NULL);
+}
+
+static void sigsegv_default_handler(int signum, siginfo_t *siginfo, void *sigctx)
+{
+    sigsegv_handler handler = sigsegv_user_handler_get();
+

util/qemu-thread-posix.c static void qemu_thread_set_name(QemuThread *thread, const char *name) · line 523

Quoted 10 lines
util/qemu-thread-posix.c +10 -0
+    if (!handler) {
+        /*
+         * remove the sigsegv handler if it's not set by user
+         * this will lead to re-raising the error without a handler
+         * and exiting from the program with "Segmentation fault"
+         */
+        int err;
+        struct sigaction act;
+        memset(&act, 0, sizeof(act));
+        act.sa_flags = SA_RESETHAND;

I'm not sure whether this is the correct way to use SA_RESETHAND.

SA_RESETHAND
Restore the signal action to the default upon entry to the signal handler. This flag is meaningful only when establishing a signal han‐ dler. SA_ONESHOT is an obsolete, nonstandard synonym for this flag.

My understanding is that it's used as a "oneshot" pattern, but here you didn't really specify any handler. My wild guess is that the handler is actually specified to SIG_DFL (which is, 0) after you bzero-ed the sigaction, hence got what we want.

I'm not sure whether we need this complexity - could we just unset the handler outside itself when reset?

util/qemu-thread-posix.c static void qemu_thread_set_name(QemuThread *thread, const char *name) · line 529

Quoted 6 lines
util/qemu-thread-posix.c +6 -0
+        err = sigaction(SIGSEGV, &act, NULL);
+        if (err) {
+            error_exit(err, __func__);
+        }
+    } else {
+        handler(signum, siginfo, sigctx);

Here not sure whether we can return something from the user handler showing that whether it correctly handled the SIGSEGV signal, otherwise how could we identify it's an expected event or a real SIGSEGV fault?

Quoted 19 lines
util/qemu-thread-posix.c +6 -0
+    }
+}
+
 void qemu_thread_create(QemuThread *thread, const char *name,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
@@ -496,14 +537,25 @@ void qemu_thread_create(QemuThread *thread, const char *name,
     sigset_t set, oldset;
     int err;
     pthread_attr_t attr;
+    struct sigaction act;
 
     err = pthread_attr_init(&attr);
     if (err) {
         error_exit(err, __func__);
     }
 
+    memset(&act, 0, sizeof(act));
+    act.sa_flags = SA_SIGINFO;

util/qemu-thread-posix.c void qemu_thread_create(QemuThread *thread, const char *name, · line 558

Quoted 10 lines
util/qemu-thread-posix.c +8 -0
+    act.sa_sigaction = sigsegv_default_handler;
+    err = sigaction(SIGSEGV, &act, NULL);
+    if (err) {
+        error_exit(err, __func__);
+    }
+
     /* Leave signal handling to the iothread.  */
     sigfillset(&set);
+    /* ...all but SIGSEGV */
+    sigdelset(&set, SIGSEGV);

Why we need to enable SIGSEGV on all threads explicitly? Could we just keep the old behavior just like what the other signals do?

Quoted 7 lines
util/qemu-thread-posix.c +0 -0
     pthread_sigmask(SIG_SETMASK, &set, &oldset);
     err = pthread_create(&thread->thread, &attr, start_routine, arg);
     if (err)
Signature · 1 line
2.17.0

Regards,

Signature · 1 line
Peter Xu
#2 lore raw

Re: [Qemu-devel] [PATCH v1 01/17] migration: add background snapshot capability

In reply to Denis Plotnikov
Quoted 90 lines · Denis Plotnikov

The capability is used for the background vmstate saving using the migration infrastructure.
Background vmstate saving means that the majority of vmstate (RAM) is saved in the background when VM's vCPUS are running. This helps to reduce the VM downtime on VM snapshotting.

---
 migration/migration.c | 35 +++++++++++++++++++++++++++++++++++
 migration/migration.h |  1 +
 qapi/migration.json   |  6 +++++-
 3 files changed, 41 insertions(+), 1 deletion(-)
migration/migration.c +35 -0
index d780601f0c..87096d23ef 100644
@@ -697,6 +697,12 @@ static bool migrate_caps_check(bool *cap_list,
             return false;
         }
 
+        if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
+            error_setg(errp, "Postcopy is not compatible "
+                        "with background snapshot");
+            return false;
+        }
+
         /* This check is reasonably expensive, so only when it's being
          * set the first time, also it's only the destination that needs
          * special support.
@@ -711,6 +717,26 @@ static bool migrate_caps_check(bool *cap_list,
         }
     }
 
+    if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
+        if (cap_list[MIGRATION_CAPABILITY_RELEASE_RAM]) {
+            error_setg(errp, "Background snapshot is not compatible "
+                        "with release ram capability");
+            return false;
+        }
+
+        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
+            error_setg(errp, "Background snapshot is not "
+                        "currently compatible with compression");
+            return false;
+        }
+
+        if (cap_list[MIGRATION_CAPABILITY_XBZRLE]) {
+            error_setg(errp, "Background snapshot is not "
+                        "currently compatible with XBZLRE");
+            return false;
+        }
+    }
+
     return true;
 }
 
@@ -1635,6 +1661,15 @@ bool migrate_use_block_incremental(void)
     return s->parameters.block_incremental;
 }
 
+bool migrate_background_snapshot(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT];
+}
+
 /* migration thread support */
 /*
  * Something bad happened to the RP stream, mark an error
migration/migration.h +1 -0
index 663415fe48..21babbc008 100644
@@ -201,6 +201,7 @@ int migrate_compress_level(void);
 int migrate_compress_threads(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
+bool migrate_background_snapshot(void);
 
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_shut(MigrationIncomingState *mis,
qapi/migration.json +0 -0
index 03f57c9616..eb5f52b7ff 100644

qapi/migration.json line 357

Quoted 6 lines
qapi/migration.json +3 -0
@@ -352,12 +352,16 @@
 #
 # @x-multifd: Use more than one fd for migration (since 2.11)
 #
+# @background-snapshot: Using migration infrastructure makes VM snapshot
+#         saving its RAM in background. This reduces  VM downtime. (since 2.12)
+#

Please specify "(since 3.1)" in your next post.

The sentence is not that clear to me. For example, "saving its RAM in background" is done in every migration, not only for the live snapshot. The words "reduces VM downtime" sounds also like for a live migration rather than snapshots. My try:

If enabled, the migration stream will be a snapshot of the VM exactly at the point when the migration procedure starts.

Quoted 15 lines
qapi/migration.json +2 -1
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
-           'block', 'return-path', 'pause-before-switchover', 'x-multifd' ] }
+           'block', 'return-path', 'pause-before-switchover', 'x-multifd',
+           'background-snapshot' ] }
 
 ##
 # @MigrationCapabilityStatus:
Signature · 1 line
2.17.0

Regards,

Signature · 1 line
Peter Xu
#11 lore raw

Re: [Qemu-devel] [PATCH v1 06/17] background snapshot: add helpers to manage a copy of ram block list

In reply to Denis Plotnikov
Quoted 5 lines · Denis Plotnikov

The VM ramblock list may be changed during the snapshotting. We want to make sure that we have the same ramblock list as it was at the time of snapshot beginning.
So, we create a copy of the list at the beginning of the snapshotting and use it during the process.

Could I ask why the list might change?

Quoted 20 lines
---
 migration/ram.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  6 ++++++
 2 files changed, 57 insertions(+)
migration/ram.c +5 -0
index 021d583b9b..4399c31606 100644
@@ -1508,6 +1508,57 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     return pages;
 }
 
+/* BackGround Snapshot Blocks */
+static RamBlockList bgs_blocks;
+
+static RamBlockList *ram_bgs_block_list_get(void)
+{

migration/ram.c static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, · line 1525

Quoted 10 lines
migration/ram.c +10 -0
+    return &bgs_blocks;
+}
+
+void ram_block_list_create(void)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+
+    qemu_mutex_lock_ramlist();
+    QLIST_FOREACH(block, &ram_list.blocks, next) {

Even if we want to copy a list of ramblocks, we possibly don't need the ones that aren't migratable? Please refer to RAMBLOCK_FOREACH_MIGRATABLE(). Then I think...

Quoted 15 lines
migration/ram.c +15 -0
+        memory_region_ref(block->mr);
+        QLIST_INSERT_HEAD(block_list, block, bgs_next);
+    }
+    qemu_mutex_unlock_ramlist();
+}
+
+void ram_block_list_destroy(void)
+{
+    RAMBlock *next, *block;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+
+    QLIST_FOREACH_SAFE(block, block_list, bgs_next, next) {
+        QLIST_REMOVE(block, bgs_next);
+        memory_region_unref(block->mr);
+    }

migration/ram.c static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss, · line 1550

Quoted 10 lines
migration/ram.c +10 -0
+}
+
+RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+
+    QLIST_FOREACH(block, block_list, bgs_next) {
+        /* This case append when the block is not mapped. */
+        if (block->host == NULL) {

... things like this should not happen.

Thanks,

Quoted 41 lines
migration/ram.c +11 -0
+            continue;
+        }
+
+        if (address - block->host < block->max_length) {
+            *page_offset = (address - block->host) & TARGET_PAGE_MASK;
+            return block;
+        }
+    }
+
+    return NULL;
+}
 /**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *
migration/ram.h +6 -0
index 64d81e9f1d..385579cae9 100644
@@ -31,6 +31,7 @@
 
 #include "qemu-common.h"
 #include "exec/cpu-common.h"
+#include "exec/ramlist.h"
 
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
@@ -61,5 +62,10 @@ 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);
+/* For background snapshot */
+void ram_block_list_create(void);
+void ram_block_list_destroy(void);
+
+RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
 
 #endif
Signature · 1 line
2.17.0
Signature · 1 line
Peter Xu
#9 lore raw

Re: [Qemu-devel] [PATCH v1 05/17] ram: extend the data structures for background snapshotting

In reply to Denis Plotnikov
Quoted 34 lines · Denis Plotnikov
---
 include/exec/ram_addr.h | 7 +++++++
 1 file changed, 7 insertions(+)
include/exec/ram_addr.h +7 -0
index 6cbc02aa0f..5b403d537d 100644
@@ -36,6 +36,8 @@ struct RAMBlock {
     char idstr[256];
     /* RCU-enabled, writes protected by the ramlist lock */
     QLIST_ENTRY(RAMBlock) next;
+    /* blocks used for background snapshot */
+    QLIST_ENTRY(RAMBlock) bgs_next;
     QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
     int fd;
     size_t page_size;
@@ -49,6 +51,11 @@ struct RAMBlock {
     unsigned long *unsentmap;
     /* bitmap of already received pages in postcopy */
     unsigned long *receivedmap;
+    /* The following 2 are for background snapshot */
+    /* Pages currently being copied */
+    unsigned long *touched_map;
+    /* Pages has been copied already */
+    unsigned long *copied_map;
 };
 
 static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
Signature · 1 line
2.17.0

IMHO these new fields can be put into the patches where they are firstly used. Isolating them do not really help much for anything AFAICT...

Regards,

Signature · 1 line
Peter Xu
#19 lore raw

Re: [Qemu-devel] [PATCH v1 10/17] background snapshots: adapt the page queueing code for using page copies

In reply to Denis Plotnikov
Quoted 3 lines · Denis Plotnikov

The background snapshot uses memeory page copying to seal the page memory content. The patch adapts the migration infrastructure to save copies of the pages.

Again, since previous page only defined some fields that are firstly used in this patch, I think you can squash that into this one.

Quoted 124 lines
---
 migration/migration.c |  2 +-
 migration/ram.c       | 59 ++++++++++++++++++++++++++++++++-----------
 migration/ram.h       |  3 ++-
 3 files changed, 47 insertions(+), 17 deletions(-)
migration/migration.c +1 -1
index 87096d23ef..131d0904e4 100644
@@ -1716,7 +1716,7 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
         return;
     }
 
-    if (ram_save_queue_pages(rbname, start, len)) {
+    if (ram_save_queue_pages(NULL, rbname, start, len, NULL)) {
         mark_source_rp_bad(ms);
     }
 }
migration/ram.c +30 -12
index dc7dfe0726..3c4ccd85b4 100644
@@ -976,7 +976,12 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
     RAMBlock *block = pss->block;
     ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
 
-    p = block->host + offset;
+    if (pss->page_copy) {
+        p = pss->page_copy;
+    } else {
+        p = block->host + offset;
+    }
+
     trace_ram_save_page(block->idstr, (uint64_t)offset, p);
 
     /* In doubt sent page as normal */
@@ -1003,13 +1008,18 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
     } else {
         pages = save_zero_page(rs, block, offset, p);
         if (pages > 0) {
-            /* Must let xbzrle know, otherwise a previous (now 0'd) cached
-             * page would be stale
-             */
-            xbzrle_cache_zero_page(rs, current_addr);
-            ram_release_pages(block->idstr, offset, pages);
+            if (pss->page_copy) {
+                qemu_madvise(p, TARGET_PAGE_SIZE, MADV_DONTNEED);
+            } else {
+                /* Must let xbzrle know, otherwise a previous (now 0'd) cached
+                 * page would be stale
+                 */
+                xbzrle_cache_zero_page(rs, current_addr);
+                ram_release_pages(block->idstr, offset, pages);
+            }
         } else if (!rs->ram_bulk_stage &&
-                   !migration_in_postcopy() && migrate_use_xbzrle()) {
+                   !migration_in_postcopy() && migrate_use_xbzrle() &&
+                   !migrate_background_snapshot()) {
             pages = save_xbzrle_page(rs, &p, current_addr, block,
                                      offset, last_stage);
             if (!last_stage) {
@@ -1026,9 +1036,10 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
         ram_counters.transferred +=
             save_page_header(rs, rs->f, block, offset | RAM_SAVE_FLAG_PAGE);
         if (send_async) {
-            qemu_put_buffer_async(rs->f, p, TARGET_PAGE_SIZE,
-                                  migrate_release_ram() &
-                                  migration_in_postcopy());
+            bool may_free = migrate_background_snapshot() ||
+                            (migrate_release_ram() &&
+                             migration_in_postcopy());
+            qemu_put_buffer_async(rs->f, p, TARGET_PAGE_SIZE, may_free);
         } else {
             qemu_put_buffer(rs->f, p, TARGET_PAGE_SIZE);
         }
@@ -1269,7 +1280,8 @@ static bool find_dirty_block(RAMState *rs, PageSearchStatus *pss, bool *again)
  * @rs: current RAM state
  * @offset: used to return the offset within the RAMBlock
  */
-static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
+static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset,
+                              void **page_copy)
 {
     RAMBlock *block = NULL;
 
@@ -1279,10 +1291,14 @@ static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
                                 QSIMPLEQ_FIRST(&rs->src_page_requests);
         block = entry->rb;
         *offset = entry->offset;
+        *page_copy = entry->page_copy;
 
         if (entry->len > TARGET_PAGE_SIZE) {
             entry->len -= TARGET_PAGE_SIZE;
             entry->offset += TARGET_PAGE_SIZE;
+            if (entry->page_copy) {
+                entry->page_copy += TARGET_PAGE_SIZE / sizeof(void *);
+            }
         } else {
             memory_region_unref(block->mr);
             QSIMPLEQ_REMOVE_HEAD(&rs->src_page_requests, next_req);
@@ -1309,9 +1325,10 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
     RAMBlock  *block;
     ram_addr_t offset;
     bool dirty;
+    void *page_copy;
 
     do {
-        block = unqueue_page(rs, &offset);
+        block = unqueue_page(rs, &offset, &page_copy);
         /*
          * We're sending this page, and since it's postcopy nothing else
          * will dirty it, and we must make sure it doesn't get sent again
@@ -1349,6 +1366,7 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
          */
         pss->block = block;
         pss->page = offset >> TARGET_PAGE_BITS;
+        pss->page_copy = page_copy;
     }
 
     return !!block;

migration/ram.c static void migration_page_queue_free(RAMState *rs) · line 1408

Quoted 5 lines
migration/ram.c +2 -0
@@ -1386,17 +1404,25 @@ static void migration_page_queue_free(RAMState *rs)
  *
  * @rbname: Name of the RAMBLock of the request. NULL means the
  *          same that last one.
+ * @block: RAMBlock to use. block and rbname have mutualy exclusive
+ *         semantic with higher priority of the block.

If "mutual exclusive" then no priority at all? Maybe simply:

@block: RAMBlock to use. When @block is provided, then @rbname is ignored.

migration/ram.c static void migration_page_queue_free(RAMState *rs) · line 1411

Quoted 3 lines
migration/ram.c +1 -0
  * @start: starting address from the start of the RAMBlock
  * @len: length (in bytes) to send
+ * @page_copy: the address the page should be written from

Maybe:

@page_copy: the page to copy to destination. If not specified, will
use the page data specified by @start and @len.

Quoted 28 lines
migration/ram.c +8 -2
  */
-int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
+int ram_save_queue_pages(RAMBlock *block, const char *rbname,
+                         ram_addr_t start, ram_addr_t len, void *page_copy)
 {
     RAMBlock *ramblock;
     RAMState *rs = ram_state;
 
     ram_counters.postcopy_requests++;
+
     rcu_read_lock();
-    if (!rbname) {
+
+    if (block) {
+        ramblock = block;
+    } else if (!rbname) {
         /* Reuse last RAMBlock */
         ramblock = rs->last_req_rb;
 
@@ -1431,6 +1457,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len)
     new_entry->rb = ramblock;
     new_entry->offset = start;
     new_entry->len = len;
+    new_entry->page_copy = page_copy;
 
     memory_region_ref(ramblock->mr);
     qemu_mutex_lock(&rs->src_page_req_mutex);

migration/ram.c static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss, · line 1499

Quoted 6 lines
migration/ram.c +2 -1
@@ -1468,7 +1495,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
          * xbzrle can do better than compression.
          */
         if (migrate_use_compression() &&
-            (rs->ram_bulk_stage || !migrate_use_xbzrle())) {
+            (rs->ram_bulk_stage || !migrate_use_xbzrle()) &&
+            !migrate_background_snapshot()) {

This seems unecessary - in the first patch you have already made sure that compression is not enabled during a live snapshot, so we should be fine since we check migrate_use_compression() first.

Quoted 29 lines
migration/ram.c +1 -0
             res = ram_save_compressed_page(rs, pss, last_stage);
         } else {
             res = ram_save_page(rs, pss, last_stage);
@@ -1706,6 +1734,7 @@ static int ram_find_and_save_block(RAMState *rs, bool last_stage)
     pss.block = rs->last_seen_block;
     pss.page = rs->last_page;
     pss.complete_round = false;
+    pss.page_copy = NULL;
 
     if (!pss.block) {
         pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
migration/ram.h +2 -1
index 4c463597a5..c3679ba65e 100644
@@ -46,7 +46,8 @@ int multifd_load_setup(void);
 int multifd_load_cleanup(Error **errp);
 
 uint64_t ram_pagesize_summary(void);
-int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
+int ram_save_queue_pages(RAMBlock *block, const char *rbname,
+                         ram_addr_t start, ram_addr_t len, void *page_copy);
 void acct_update_position(QEMUFile *f, size_t size, bool zero);
 void ram_debug_dump_bitmap(unsigned long *todump, bool expected,
                            unsigned long pages);
Signature · 1 line
2.17.0

Regards,

Signature · 1 line
Peter Xu
#13 lore raw

Re: [Qemu-devel] [PATCH v1 07/17] background snapshot: introduce page buffer

In reply to Denis Plotnikov
Quoted 53 lines · Denis Plotnikov

To limit the amount of memory used by the background snapshot a memory limiter is used which called "page buffer".
In fact, it's a memory page counter limiting the page allocation. Currently, the limit of pages is hardcoded but its setter is the matter of the future work.

The background snapshot makes a copy of the page before writing it to the snapshot destination, but it doesn't use a preallocated buffer, because the background snapshot employs the migration infrastructure which, in turn, uses madvice to release the buffer.
The advice issuing is hard to track, so here we rely on anonymous memory mapping and releasing it by the existing code.

---
 migration/ram.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  3 ++
 2 files changed, 94 insertions(+)
migration/ram.c +15 -0
index 4399c31606..27d3403435 100644
@@ -192,6 +192,16 @@ struct RAMSrcPageRequest {
     QSIMPLEQ_ENTRY(RAMSrcPageRequest) next_req;
 };
 
+/* Page buffer used for background snapshot */
+typedef struct RAMPageBuffer {
+    /* Page buffer capacity in host pages */
+    int capacity;
+    /* Current number of pages in the buffer */
+    int used;
+    /* Event to notify that buffer usage is under capacity */
+    QemuEvent used_decreased;
+} RAMPageBuffer;
+
 /* State of RAM for migration */
 struct RAMState {
     /* QEMUFile used for this migration */
@@ -230,6 +240,11 @@ struct RAMState {
     /* Queue of outstanding page requests from the destination */
     QemuMutex src_page_req_mutex;
     QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) src_page_requests;
+    /* The following 2 are for background snapshot */
+    /* Buffer data to store copies of ram pages while async vm saving */
+    RAMPageBuffer page_buffer;
+    /* Event to notify that a page copying just has finished*/
+    QemuEvent page_copying_done;
 };
 typedef struct RAMState RAMState;
 

migration/ram.c void ram_block_list_destroy(void) · line 1560

Quoted 6 lines
migration/ram.c +3 -0
@@ -1540,6 +1555,52 @@ void ram_block_list_destroy(void)
     }
 }
 
+static void ram_page_buffer_decrease_used(void)
+{
+    qemu_event_reset(&ram_state->page_buffer.used_decreased);

Could I ask why do we need to reset it before set?

Quoted 11 lines
migration/ram.c +11 -0
+    atomic_dec(&ram_state->page_buffer.used);
+    qemu_event_set(&ram_state->page_buffer.used_decreased);
+}
+
+static void ram_page_buffer_increase_used_wait(void)
+{
+    int used, *used_ptr;
+    RAMState *rs = ram_state;
+    used_ptr = &rs->page_buffer.used;
+    do {
+        used = atomic_read(used_ptr);

migration/ram.c void ram_block_list_destroy(void) · line 1581

Quoted 10 lines
migration/ram.c +10 -0
+        if (rs->page_buffer.capacity > used) {
+            if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
+                return;
+            } else {
+                continue;
+            }
+        } else {
+            qemu_event_wait(&rs->page_buffer.used_decreased);
+        }
+    } while (true);

AFAIU this whole function tries to serialize the usage of page_buffer.used. Have you ever tried to simply use e.g. a mutex to protect it, and just implement the function in the simplest (but complete) way first? I believe this is for performance's sake but I'm not sure if it will run faster if the critical section of the mutex is very small (here, only the update of page_buffer_used). It seems that there are multiple ongoing works on the list that have tried to implement their own data structures (possibly lockless), including this one. For example, Guangrong has proposed a lockless fifo just in ~1.5 month:

https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg00526.html

Lots of discussions there, and IIUC the conclusion is that we can consider to just backport an existing implementation from the kernel:

https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg08807.html

I'm just hoping that these "data structure" works might not block the function itself from being merged. Same thing applies to this series. I never meant that you should drop existing work and do it again using mutex, but I just spoke this out in case it makes some sense to move the work forward faster.

Quoted 10 lines
migration/ram.c +10 -0
+}
+
+void *ram_page_buffer_get(void)
+{
+    void *page;
+    ram_page_buffer_increase_used_wait();
+    page = mmap(0, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
+                MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+    if (page == MAP_FAILED) {

migration/ram.c void ram_block_list_destroy(void) · line 1601

Quoted 10 lines
migration/ram.c +10 -0
+        ram_page_buffer_decrease_used();
+        page = NULL;
+    }
+    return page;
+}
+
+int ram_page_buffer_free(void *buffer)
+{
+    ram_page_buffer_decrease_used();
+    return qemu_madvise(buffer, TARGET_PAGE_SIZE, MADV_DONTNEED);

Just to confirm: is this really exactly the same as munmap()?

Quoted 14 lines
migration/ram.c +3 -0
+}
+
 RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
 {
     RAMBlock *block = NULL;
@@ -1559,6 +1620,7 @@ RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset)
 
     return NULL;
 }
+
 /**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *

migration/ram.c static void xbzrle_load_cleanup(void) · line 1720

Quoted 8 lines
migration/ram.c +5 -0
@@ -1651,6 +1713,13 @@ static void xbzrle_load_cleanup(void)
 
 static void ram_state_cleanup(RAMState **rsp)
 {
+    if (migrate_background_snapshot()) {
+        qemu_event_destroy(&(*rsp)->page_buffer.used_decreased);
+        /* In case somebody is still waiting for the event */
+        qemu_event_set(&(*rsp)->page_copying_done);
+        qemu_event_destroy(&(*rsp)->page_copying_done);

We set this since "there might be someone waiting", however immediately we destroy it... Then what if someone reads it after we destroy it? Don't we need to sync somehow before destruction happens?

Quoted 75 lines
migration/ram.c +24 -0
+    }
+
     migration_page_queue_free(*rsp);
     qemu_mutex_destroy(&(*rsp)->bitmap_mutex);
     qemu_mutex_destroy(&(*rsp)->src_page_req_mutex);
@@ -1689,6 +1758,13 @@ static void ram_save_cleanup(void *opaque)
         block->bmap = NULL;
         g_free(block->unsentmap);
         block->unsentmap = NULL;
+
+        if (migrate_background_snapshot()) {
+            g_free(block->touched_map);
+            block->touched_map = NULL;
+            g_free(block->copied_map);
+            block->copied_map = NULL;
+        }
     }
 
     xbzrle_cleanup();
@@ -1703,6 +1779,10 @@ static void ram_state_reset(RAMState *rs)
     rs->last_page = 0;
     rs->last_version = ram_list.version;
     rs->ram_bulk_stage = true;
+
+    /* page buffer capacity in number of pages */
+    rs->page_buffer.capacity = 1000;
+    rs->page_buffer.used = 0;
 }
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
@@ -2180,6 +2260,11 @@ static int ram_state_init(RAMState **rsp)
      */
     (*rsp)->migration_dirty_pages = ram_bytes_total() >> TARGET_PAGE_BITS;
 
+    if (migrate_background_snapshot()) {
+        qemu_event_init(&ram_state->page_buffer.used_decreased, false);
+        qemu_event_init(&ram_state->page_copying_done, false);
+    }
+
     ram_state_reset(*rsp);
 
     return 0;
@@ -2196,10 +2281,16 @@ static void ram_list_init_bitmaps(void)
             pages = block->max_length >> TARGET_PAGE_BITS;
             block->bmap = bitmap_new(pages);
             bitmap_set(block->bmap, 0, pages);
+
             if (migrate_postcopy_ram()) {
                 block->unsentmap = bitmap_new(pages);
                 bitmap_set(block->unsentmap, 0, pages);
             }
+
+            if (migrate_background_snapshot()) {
+                block->touched_map = bitmap_new(pages);
+                block->copied_map = bitmap_new(pages);
+            }
         }
     }
 }
migration/ram.h +3 -0
index 385579cae9..7c802c1d7f 100644
@@ -68,4 +68,7 @@ void ram_block_list_destroy(void);
 
 RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
 
+void *ram_page_buffer_get(void);
+int ram_page_buffer_free(void *buffer);
+
 #endif
Signature · 1 line
2.17.0

Regards,

Signature · 1 line
Peter Xu
#29 lore raw
In reply to Denis Plotnikov
Quoted 10 lines · Denis Plotnikov

The workflow to make a snapshot is the following:
1. Pause the vm
2. Make a snapshot of block devices using the scheme of your choice
3. Turn on background-snapshot migration capability
4. Start the migration using the destination (migration stream) of your choice. The migration will resume the vm execution by itself
when it has the devices' states saved and is ready to start ram writing to the migration stream.
5. Listen to the migration finish event

Quoted 3 lines

The bakground snapshot works with support of KVM patch:
"x86: mmu: report failed memory access to the userspace" (not applied to the mainstream, it's in the kvm mailing list)

Hello, Denis,

Do you mind to push your tree to an online repository in case to make review easier?

Thanks,

Signature · 1 line
Peter Xu
#26 lore raw

Re: [Qemu-devel] [PATCH v1 15/17] kvm: add vCPU failed memeory access processing

In reply to Denis Plotnikov
Quoted 3 lines · Denis Plotnikov

Is done with support of the KVM patch returning the faulting address.

I feel like these two kvm-related patches can be put at the end of the series as an extension to kvm support. E.g., without these kvm patches one should still be able to do live snapshot with TCG. Then even if your KVM patches are not settled (along with the header update) there's still chance that your framework can be merged first.

Quoted 50 lines
---
 target/i386/kvm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
target/i386/kvm.c +17 -0
index 3ac5302bc5..55b8860d1a 100644
@@ -45,6 +45,8 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "migration/blocker.h"
+#include "migration/savevm.h"
+#include "migration/ram.h"
 #include "exec/memattrs.h"
 #include "trace.h"
 
@@ -3130,6 +3132,18 @@ static bool host_supports_vmx(void)
     return ecx & CPUID_EXT_VMX;
 }
 
+static int kvm_handle_fail_mem_access(CPUState *cpu)
+{
+    struct kvm_run *run = cpu->kvm_run;
+    int ret = ram_process_page_fault((void *)run->fail_mem_access.hva);
+
+    if (ret >= 0) {
+        cpu_resume(cpu);
+    }
+
+    return ret;
+}
+
 #define VMX_INVALID_GUEST_STATE 0x80000021
 
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
@@ -3188,6 +3202,9 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
         ioapic_eoi_broadcast(run->eoi.vector);
         ret = 0;
         break;
+    case KVM_EXIT_FAIL_MEM_ACCESS:
+        ret = kvm_handle_fail_mem_access(cs);
+        break;
     default:
         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
         ret = -1;
Signature · 1 line
2.17.0

Regards,

Signature · 1 line
Peter Xu
#14 lore raw

Re: [Qemu-devel] [PATCH v1 07/17] background snapshot: introduce page buffer

In reply to Denis Plotnikov

migration/ram.c void ram_block_list_destroy(void) · line 1562

Quoted 5 lines · Denis Plotnikov
migration/ram.c +5 -0
+static void ram_page_buffer_decrease_used(void)
+{
+    qemu_event_reset(&ram_state->page_buffer.used_decreased);
+    atomic_dec(&ram_state->page_buffer.used);
+    qemu_event_set(&ram_state->page_buffer.used_decreased);

As Peter noted, only the qemu_event_set is needed here, while...

Quoted 9 lines
migration/ram.c +9 -0
+}
+
+static void ram_page_buffer_increase_used_wait(void)
+{
+    int used, *used_ptr;
+    RAMState *rs = ram_state;
+    used_ptr = &rs->page_buffer.used;
+    do {
+        used = atomic_read(used_ptr);

migration/ram.c void ram_block_list_destroy(void) · line 1581

Quoted 10 lines
migration/ram.c +10 -0
+        if (rs->page_buffer.capacity > used) {
+            if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
+                return;
+            } else {
+                continue;
+            }
+        } else {
+            qemu_event_wait(&rs->page_buffer.used_decreased);
+        }
+    } while (true);

... the right idiom for using qemu_event_wait is this:

if test condition
reset
if test condition
wait

So something like

    for(;;) {
        used = atomic_read(used_ptr);
        if (rs->page_buffer.capacity <= used) {
            qemu_event_reset(&rs->page_buffer.used_decreased);
            used = atomic_read(used_ptr);
            if (rs->page_buffer.capacity <= used) {
                qemu_event_wait(&rs->page_buffer.used_decreased);
                continue;
            }
        }

        if (atomic_cmpxchg(used_ptr, used, used + 1) == used) {
            return;
        }
    }

Note that there must be a single thread doing the above. Otherwise, you have to use mutex and condvar.

Paolo

#16 lore raw

Re: [Qemu-devel] [PATCH v1 08/17] migration: add helpers to change VM memory protection rights

In reply to Denis Plotnikov
Quoted 54 lines · * Denis Plotnikov ([email protected]) wrote:
---
 migration/ram.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
 migration/ram.h |  3 +++
 2 files changed, 57 insertions(+)
migration/ram.c +40 -0
index 27d3403435..ce3dead932 100644
@@ -1555,6 +1555,60 @@ void ram_block_list_destroy(void)
     }
 }
 
+static int mem_protect(void *addr, uint64_t length, int prot)
+{
+    int ret = mprotect(addr, length, prot);
+
+    if (ret < 0) {
+        error_report("%s: Can't change protection on ram block at %p",
+                     __func__, addr);
+    }
+
+    return ret;
+}
+
+static int ram_set_ro(void *addr, uint64_t length)
+{
+    return mem_protect(addr, length, PROT_READ);
+}
+
+static int ram_set_rw(void *addr, uint64_t length)
+{
+    return mem_protect(addr, length, PROT_READ | PROT_WRITE);
+}
+
+int ram_block_list_set_readonly(void)
+{
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+    int ret = 0;
+
+    QLIST_FOREACH(block, block_list, bgs_next) {
+        ret = ram_set_ro(block->host, block->used_length);
+        if (ret) {
+            break;
+        }
+    }
+
+    return ret;
+}
+
+int ram_block_list_set_writable(void)
+{

migration/ram.c void ram_block_list_destroy(void) · line 1607

Quoted 10 lines
migration/ram.c +10 -0
+    RAMBlock *block = NULL;
+    RamBlockList *block_list = ram_bgs_block_list_get();
+    int ret = 0;
+
+    QLIST_FOREACH(block, block_list, bgs_next) {
+        ret = ram_set_rw(block->host, block->used_length);
+        if (ret) {
+            break;
+        }
+    }

Do you need to keep track of who was writeable previously to avoid changing blocks that should be read-only, Or are all RAMBlocks currently mapped rw?

Dave

Quoted 21 lines
migration/ram.c +3 -0
+    return ret;
+}
+
 static void ram_page_buffer_decrease_used(void)
 {
     qemu_event_reset(&ram_state->page_buffer.used_decreased);
migration/ram.h +3 -0
index 7c802c1d7f..4c463597a5 100644
@@ -71,4 +71,7 @@ RAMBlock *ram_bgs_block_find(uint8_t *address, ram_addr_t *page_offset);
 void *ram_page_buffer_get(void);
 int ram_page_buffer_free(void *buffer);
 
+int ram_block_list_set_readonly(void);
+int ram_block_list_set_writable(void);
+
 #endif
Signature · 1 line
2.17.0

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

#20 lore raw

Re: [Qemu-devel] [PATCH v1 10/17] background snapshots: adapt the page queueing code for using page copies

In reply to Denis Plotnikov

On Wed, Jul 18, 2018 at 06:41:53PM +0300, Denis Plotnikov wrote:

[...]

migration/ram.c static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage) · line 1009

Quoted 3 lines
migration/ram.c +0 -0
@@ -1003,13 +1008,18 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
     } else {
         pages = save_zero_page(rs, block, offset, p);

Now save_zero_page() is not called by ram_save_page(). I'd suggest that you rebase to the latest master in your next post since the code base looks old.

Thanks,

Signature · 1 line
Peter Xu
#30 lore raw
In reply to Peter Xu

Hi Peter

I moved the code to the repository https://github.com/denis-plotnikov/qemu/tree/background-snapshot-kvm.
the current version includes fixes with respect to your comments for version 1.
I moved KVM related patches to the end of the branch (formerly patch series).
Since, the KVM patches and the other parts to modify (vhost an others) are needless in favor of upcoming userfaltfd, I would ask you to review the general framework which is able to work with tcg.

Thanks in advance!

Denis

Quoted 22 lines · Peter Xu
Quoted 10 lines · Denis Plotnikov

The workflow to make a snapshot is the following:
1. Pause the vm
2. Make a snapshot of block devices using the scheme of your choice
3. Turn on background-snapshot migration capability
4. Start the migration using the destination (migration stream) of your choice. The migration will resume the vm execution by itself
when it has the devices' states saved and is ready to start ram writing to the migration stream.
5. Listen to the migration finish event

Quoted 3 lines

The bakground snapshot works with support of KVM patch:
"x86: mmu: report failed memory access to the userspace" (not applied to the mainstream, it's in the kvm mailing list)

Hello, Denis,

Do you mind to push your tree to an online repository in case to make review easier?

Thanks,

Signature · 2 lines
Best,
Denis
#31 lore raw
Quoted 1 line · Denis Plotnikov

Hi Peter

Hi, Denis,

Quoted 12 lines

I moved the code to the repository
https://github.com/denis-plotnikov/qemu/tree/background-snapshot-kvm.
the current version includes fixes with respect to your comments for version 1.
I moved KVM related patches to the end of the branch (formerly patch series).
Since, the KVM patches and the other parts to modify (vhost an others) are needless in favor of upcoming userfaltfd,
I would ask you to review the general framework which is able to work with tcg.

Quoted 1 line

Thanks in advance!

Thank you for pushing the tree.

I might have made a mistake before that I thought this work is at least working for TCG, but I think I was wrong. The problem is (I'm trying to repeat Dave's question that you seems haven't yet answered):
even for TCG there could be use cases where the process might access guest memory from the kernel space (e.g., vhost, or any system calls that with a guest memory buffer passed in). I'm afraid mprotect() and the whole signal-based mechanism cannot be able to address these page faults, then we'll encounter adhoc errors and we'll need to fix all these places up. Userfaultfd-wp should not have this problem.

I think the general idea of the work is good, but I'm not sure whether we can merge the work if we don't settle these issues.

Regards,

Signature · 1 line
Peter Xu
#32 lore raw

Hi Peter,

Thanks for the reply.

Ok, I understand about tcg.
So my only option is to wait for userfaultfd-wp.
Do you know if anyone is currently working on this? And if so, then is there any estimations when the userfaultfd is ready?

Denis

Quoted 36 lines · Peter Xu
Quoted 1 line · Denis Plotnikov

Hi Peter

Hi, Denis,

Quoted 12 lines

I moved the code to the repository
https://github.com/denis-plotnikov/qemu/tree/background-snapshot-kvm.
the current version includes fixes with respect to your comments for version 1.
I moved KVM related patches to the end of the branch (formerly patch series).
Since, the KVM patches and the other parts to modify (vhost an others) are needless in favor of upcoming userfaltfd,
I would ask you to review the general framework which is able to work with tcg.

Quoted 1 line

Thanks in advance!

Thank you for pushing the tree.

I might have made a mistake before that I thought this work is at least working for TCG, but I think I was wrong. The problem is (I'm trying to repeat Dave's question that you seems haven't yet answered):
even for TCG there could be use cases where the process might access guest memory from the kernel space (e.g., vhost, or any system calls that with a guest memory buffer passed in). I'm afraid mprotect() and the whole signal-based mechanism cannot be able to address these page faults, then we'll encounter adhoc errors and we'll need to fix all these places up. Userfaultfd-wp should not have this problem.

I think the general idea of the work is good, but I'm not sure whether we can merge the work if we don't settle these issues.

Regards,

Signature · 2 lines
Best,
Denis
#33 lore raw
Quoted 8 lines · Denis Plotnikov

Hi Peter,

Thanks for the reply.

Ok, I understand about tcg.
So my only option is to wait for userfaultfd-wp.
Do you know if anyone is currently working on this? And if so, then is there any estimations when the userfaultfd is ready?

I am working on it, but it's only progressing slowly. The latest status is that I can run with QEMU/tcg now with Andrea's tree (will need two more patches to be applied upon from me) however all the changes are still unmature (e.g., one of the patch is discussing on the linux-mm list, the other one I haven't discussed with anyone; but these patches worked for me).

Let me know if want to play with the tree, at least I can share with you on what I got (so maybe at least you can try to run your live snapshot with QEMU/tcg with my userfaultfd tree; if error occurred we can investigate). Again, it is always welcomed if you want to participate in the kernel work too.

Let me know what you think about it.

Regards,

Signature · 1 line
Peter Xu