[Qemu-devel] [PATCH 00/30] Migration thread 20121017 edition

[Qemu-devel] [PATCH 00/30] Migration thread 20121017 edition

[Juan Quintela]

Hi

This series apply on top of the refactoring that I sent yesterday.
Changes from the last version include:

- buffered_file.c is gone, its functionality is merged in migration.c
  special attention to the megre of buffered_file_thread() &
  migration_file_put_notify().

- Some more bitmap handling optimizations (thanks to Orit & Paolo for
  suggestions and code and Vinod for testing)

Please review.  Included is the pointer to the full tree.

Thanks, Juan.

The following changes since commit b6348f29d033d5a8a26f633d2ee94362595f32a4:

  target-arm/translate: Fix RRX operands (2012-10-17 19:56:46 +0200)

are available in the git repository at:

  http://repo.or.cz/r/qemu/quintela.git migration-thread-20121017

for you to fetch changes up to 486dabc29f56d8f0e692395d4a6cd483b3a77f01:

  ram: optimize migration bitmap walking (2012-10-18 09:20:34 +0200)


v3:

This is work in progress on top of the previous migration series just sent.

- Introduces a thread for migration instead of using a timer and callback
- remove the writting to the fd from the iothread lock
- make the writes synchronous
- Introduce a new pending method that returns how many bytes are pending for
  one save live section
- last patch just shows printfs to see where the time is being spent
  on the migration complete phase.
  (yes it pollutes all uses of stop on the monitor)

So far I have found that we spent a lot of time on bdrv_flush_all() It
can take from 1ms to 600ms (yes, it is not a typo).  That dwarfs the
migration default downtime time (30ms).

Stop all vcpus:

- it works now (after the changes on qemu_cpu_is_vcpu on the previous
  series) caveat is that the time that brdv_flush_all() takes is
  "unpredictable".  Any silver bullets?

  Paolo suggested to call for migration completion phase:

  bdrv_aio_flush_all();
  Sent the dirty pages;
  bdrv_drain_all()
  brdv_flush_all()
  another round through the bitmap in case that completions have
  changed some page

  Paolo, did I get it right?
  Any other suggestion?

- migrate_cancel() is not properly implemented (as in the film that we
  take no locks, ...)

- expected_downtime is not calculated.

  I am about to merge migrate_fd_put_ready & buffered_thread() and
  that would make trivial to calculate.

It outputs something like:

wakeup_request 0
time cpu_disable_ticks 0
time pause_all_vcpus 1
time runstate_set 1
time vmstate_notify 2
time bdrv_drain_all 2
time flush device /dev/disk/by-path/ip-192.168.10.200:3260-iscsi-iqn.2010-12.org.trasno:iscsi.lvm-lun-1: 3
time flush device : 3
time flush device : 3
time flush device : 3
time bdrv_flush_all 5
time monitor_protocol_event 5
vm_stop 2 5
synchronize_all_states 1
migrate RAM 37
migrate rest devices 1
complete without error 3a 44
completed 45
end completed stage 45

As you can see, we estimate that we can sent all pending data in 30ms,
it took 37ms to send the RAM (that is what we calculate).  So
estimation is quite good.

What it gives me lots of variation is on the line with device name of "time flush device".
That is what varies between 1ms to 600ms

This is in a completely idle guest.  I am running:

	while (1) {
		uint64_t delay;

		if (gettimeofday(&t0, NULL) != 0)
			perror("gettimeofday 1");
		if (usleep(ms2us(10)) != 0)
			perror("usleep");
		if (gettimeofday(&t1, NULL) != 0)
			perror("gettimeofday 2");

		t1.tv_usec -= t0.tv_usec;
		if (t1.tv_usec < 0) {
			t1.tv_usec += 1000000;
			t1.tv_sec--;
		}
		t1.tv_sec -= t0.tv_sec;

		delay = t1.tv_sec * 1000 + t1.tv_usec/1000;

		if (delay > 100)
			printf("delay of %ld ms\n", delay);
       }

To see the latency inside the guest (i.e. ask for a 10ms sleep, and see how long it takes).


[root@d1 ~]# ./timer 
delay of 161 ms
delay of 135 ms
delay of 143 ms
delay of 132 ms
delay of 131 ms
delay of 141 ms
delay of 113 ms
delay of 119 ms
delay of 114 ms


But that values are independent of migration.  Without even starting
the migration, idle guest doing nothing, we get it sometimes.



Juan Quintela (27):
  buffered_file: Move from using a timer to use a thread
  migration: make qemu_fopen_ops_buffered() return void
  migration: stop all cpus correctly
  migration: make writes blocking
  migration: remove unfreeze logic
  migration: take finer locking
  buffered_file: Unfold the trick to restart generating migration data
  buffered_file: don't flush on put buffer
  buffered_file: unfold buffered_append in buffered_put_buffer
  savevm: New save live migration method: pending
  migration: include qemu-file.h
  migration-fd: remove duplicate include
  migration: move buffered_file.c code into migration.c
  migration: move migration_fd_put_ready()
  migration: Inline qemu_fopen_ops_buffered into migrate_fd_connect
  migration: move migration notifier
  migration: move begining stage to the migration thread
  migration: move exit condition to migration thread
  migration: unfold rest of migrate_fd_put_ready() into thread
  migration: print times for end phase
  ram: rename last_block to last_seen_block
  ram: Add last_sent_block
  memory: introduce memory_region_test_and_clear_dirty
  ram: Use memory_region_test_and_clear_dirty
  fix memory.c
  migration: Only go to the iterate stage if there is anything to send
  ram: optimize migration bitmap walking

Paolo Bonzini (1):
  split MRU ram list

Umesh Deshpande (2):
  add a version number to ram_list
  protect the ramlist with a separate mutex

 Makefile.objs     |   2 +-
 arch_init.c       | 133 +++++++++++--------
 block-migration.c |  49 ++-----
 block.c           |   6 +
 buffered_file.c   | 256 -----------------------------------
 buffered_file.h   |  22 ---
 cpu-all.h         |  13 +-
 cpus.c            |  17 +++
 exec.c            |  44 +++++-
 memory.c          |  17 +++
 memory.h          |  18 +++
 migration-exec.c  |   4 +-
 migration-fd.c    |   9 +-
 migration-tcp.c   |  21 +--
 migration-unix.c  |   4 +-
 migration.c       | 391 ++++++++++++++++++++++++++++++++++++++++--------------
 migration.h       |   4 +-
 qemu-file.h       |   5 -
 savevm.c          |  37 +++++-
 sysemu.h          |   1 +
 vmstate.h         |   1 +
 21 files changed, 522 insertions(+), 532 deletions(-)
 delete mode 100644 buffered_file.c
 delete mode 100644 buffered_file.h

-- 
1.7.11.7

[Qemu-devel] [PATCH 01/30] split MRU ram list

[Juan Quintela]

From: Paolo Bonzini <pbonzini@redhat.com>

Outside the execution threads the normal, non-MRU-ized order of
the RAM blocks should always be enough.  So manage two separate
lists, which will have separate locking rules.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c |  1 +
 cpu-all.h   |  4 +++-
 exec.c      | 18 +++++++++++++-----
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index e6effe8..4293557 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -48,6 +48,7 @@
 #include "qemu/page_cache.h"
 #include "qmp-commands.h"
 #include "trace.h"
+#include "cpu-all.h"

 #ifdef DEBUG_ARCH_INIT
 #define DPRINTF(fmt, ...) \
diff --git a/cpu-all.h b/cpu-all.h
index 6aa7e58..6558a6f 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -490,8 +490,9 @@ typedef struct RAMBlock {
     ram_addr_t offset;
     ram_addr_t length;
     uint32_t flags;
-    char idstr[256];
+    QLIST_ENTRY(RAMBlock) next_mru;
     QLIST_ENTRY(RAMBlock) next;
+    char idstr[256];
 #if defined(__linux__) && !defined(TARGET_S390X)
     int fd;
 #endif
@@ -499,6 +500,7 @@ typedef struct RAMBlock {

 typedef struct RAMList {
     uint8_t *phys_dirty;
+    QLIST_HEAD(, RAMBlock) blocks_mru;
     QLIST_HEAD(, RAMBlock) blocks;
 } RAMList;
 extern RAMList ram_list;
diff --git a/exec.c b/exec.c
index e63ad0d..718bbc2 100644
--- a/exec.c
+++ b/exec.c
@@ -56,6 +56,7 @@
 #include "xen-mapcache.h"
 #include "trace.h"
 #endif
+#include "cpu-all.h"

 #include "cputlb.h"

@@ -112,7 +113,10 @@ static uint8_t *code_gen_ptr;
 int phys_ram_fd;
 static int in_migration;

-RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
+RAMList ram_list = {
+    .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks),
+    .blocks_mru = QLIST_HEAD_INITIALIZER(ram_list.blocks_mru)
+};

 static MemoryRegion *system_memory;
 static MemoryRegion *system_io;
@@ -633,6 +637,7 @@ bool tcg_enabled(void)
 void cpu_exec_init_all(void)
 {
 #if !defined(CONFIG_USER_ONLY)
+    qemu_mutex_init(&ram_list.mutex);
     memory_map_init();
     io_mem_init();
 #endif
@@ -2568,6 +2573,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
     new_block->length = size;

     QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
+    QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);

     ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);
@@ -2595,6 +2601,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
+            QLIST_REMOVE(block, next_mru);
             g_free(block);
             return;
         }
@@ -2608,6 +2615,7 @@ void qemu_ram_free(ram_addr_t addr)
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
+            QLIST_REMOVE(block, next_mru);
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
             } else if (mem_path) {
@@ -2713,12 +2721,12 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
 {
     RAMBlock *block;

-    QLIST_FOREACH(block, &ram_list.blocks, next) {
+    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
         if (addr - block->offset < block->length) {
             /* Move this entry to to start of the list.  */
             if (block != QLIST_FIRST(&ram_list.blocks)) {
-                QLIST_REMOVE(block, next);
-                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
+                QLIST_REMOVE(block, next_mru);
+                QLIST_INSERT_HEAD(&ram_list.blocks_mru, block, next_mru);
             }
             if (xen_enabled()) {
                 /* We need to check if the requested address is in the RAM
@@ -2813,7 +2821,7 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
         return 0;
     }

-    QLIST_FOREACH(block, &ram_list.blocks, next) {
+    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
         /* This case append when the block is not mapped. */
         if (block->host == NULL) {
             continue;
-- 
1.7.11.7

[Qemu-devel] [PATCH 02/30] add a version number to ram_list

[Juan Quintela]

From: Umesh Deshpande <udeshpan@redhat.com>

This will be used to detect if last_block might have become invalid
across different calls to ram_save_live.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 7 ++++++-
 cpu-all.h   | 1 +
 exec.c      | 5 ++++-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 4293557..b47313d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -336,6 +336,7 @@ static RAMBlock *last_block;
 static ram_addr_t last_offset;
 static unsigned long *migration_bitmap;
 static uint64_t migration_dirty_pages;
+static uint32_t last_version;

 static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
                                                          ram_addr_t offset)
@@ -406,7 +407,6 @@ static void migration_bitmap_sync(void)
     }
 }

-
 /*
  * ram_save_block: Writes a page of memory to the stream f
  *
@@ -558,6 +558,7 @@ static void reset_ram_globals(void)
 {
     last_block = NULL;
     last_offset = 0;
+    last_version = ram_list.version;
     sort_ram_list();
 }

@@ -613,6 +614,10 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     uint64_t expected_downtime;
     MigrationState *s = migrate_get_current();

+    if (ram_list.version != last_version) {
+        reset_ram_globals();
+    }
+
     bytes_transferred_last = bytes_transferred;
     bwidth = qemu_get_clock_ns(rt_clock);

diff --git a/cpu-all.h b/cpu-all.h
index 6558a6f..e07c91c 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -500,6 +500,7 @@ typedef struct RAMBlock {

 typedef struct RAMList {
     uint8_t *phys_dirty;
+    uint32_t version;
     QLIST_HEAD(, RAMBlock) blocks_mru;
     QLIST_HEAD(, RAMBlock) blocks;
 } RAMList;
diff --git a/exec.c b/exec.c
index 718bbc2..1e04711 100644
--- a/exec.c
+++ b/exec.c
@@ -637,7 +637,6 @@ bool tcg_enabled(void)
 void cpu_exec_init_all(void)
 {
 #if !defined(CONFIG_USER_ONLY)
-    qemu_mutex_init(&ram_list.mutex);
     memory_map_init();
     io_mem_init();
 #endif
@@ -2575,6 +2574,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
     QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
     QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);

+    ram_list.version++;
+
     ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);
     memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
@@ -2602,6 +2603,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
             QLIST_REMOVE(block, next_mru);
+            ram_list.version++;
             g_free(block);
             return;
         }
@@ -2616,6 +2618,7 @@ void qemu_ram_free(ram_addr_t addr)
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
             QLIST_REMOVE(block, next_mru);
+            ram_list.version++;
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
             } else if (mem_path) {
-- 
1.7.11.7

[Qemu-devel] [PATCH 03/30] protect the ramlist with a separate mutex

[Juan Quintela]

From: Umesh Deshpande <udeshpan@redhat.com>

Add the new mutex that protects shared state between ram_save_live
and the iothread.  If the iothread mutex has to be taken together
with the ramlist mutex, the iothread shall always be _outside_.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c |  9 ++++++++-
 cpu-all.h   |  8 ++++++++
 exec.c      | 23 +++++++++++++++++++++--
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index b47313d..2d29828 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -553,7 +553,6 @@ static void ram_migration_cancel(void *opaque)
     migration_end();
 }

-
 static void reset_ram_globals(void)
 {
     last_block = NULL;
@@ -573,6 +572,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     bitmap_set(migration_bitmap, 1, ram_pages);
     migration_dirty_pages = ram_pages;

+    qemu_mutex_lock_ramlist();
     bytes_transferred = 0;
     reset_ram_globals();

@@ -600,6 +600,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
         qemu_put_be64(f, block->length);
     }

+    qemu_mutex_unlock_ramlist();
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

     return 0;
@@ -614,6 +615,8 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     uint64_t expected_downtime;
     MigrationState *s = migrate_get_current();

+    qemu_mutex_lock_ramlist();
+
     if (ram_list.version != last_version) {
         reset_ram_globals();
     }
@@ -662,6 +665,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         bwidth = 0.000001;
     }

+    qemu_mutex_unlock_ramlist();
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

     expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
@@ -682,6 +686,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 {
     migration_bitmap_sync();

+    qemu_mutex_lock_ramlist();
+
     /* try transferring iterative blocks of memory */

     /* flush all remaining blocks regardless of rate limiting */
@@ -697,6 +703,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     }
     memory_global_dirty_log_stop();

+    qemu_mutex_unlock_ramlist();
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

     g_free(migration_bitmap);
diff --git a/cpu-all.h b/cpu-all.h
index e07c91c..2bafcdd 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -22,6 +22,7 @@
 #include "qemu-common.h"
 #include "qemu-tls.h"
 #include "cpu-common.h"
+#include "qemu-thread.h"

 /* some important defines:
  *
@@ -490,7 +491,9 @@ typedef struct RAMBlock {
     ram_addr_t offset;
     ram_addr_t length;
     uint32_t flags;
+    /* Protected by the iothread lock.  */
     QLIST_ENTRY(RAMBlock) next_mru;
+    /* Protected by the ramlist lock.  */
     QLIST_ENTRY(RAMBlock) next;
     char idstr[256];
 #if defined(__linux__) && !defined(TARGET_S390X)
@@ -499,9 +502,12 @@ typedef struct RAMBlock {
 } RAMBlock;

 typedef struct RAMList {
+    QemuMutex mutex;
+    /* Protected by the iothread lock.  */
     uint8_t *phys_dirty;
     uint32_t version;
     QLIST_HEAD(, RAMBlock) blocks_mru;
+    /* Protected by the ramlist lock.  */
     QLIST_HEAD(, RAMBlock) blocks;
 } RAMList;
 extern RAMList ram_list;
@@ -521,6 +527,8 @@ extern int mem_prealloc;

 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 ram_addr_t last_ram_offset(void);
+void qemu_mutex_lock_ramlist(void);
+void qemu_mutex_unlock_ramlist(void);
 #endif /* !CONFIG_USER_ONLY */

 int cpu_memory_rw_debug(CPUArchState *env, target_ulong addr,
diff --git a/exec.c b/exec.c
index 1e04711..cf9de92 100644
--- a/exec.c
+++ b/exec.c
@@ -637,6 +637,7 @@ bool tcg_enabled(void)
 void cpu_exec_init_all(void)
 {
 #if !defined(CONFIG_USER_ONLY)
+    qemu_mutex_init(&ram_list.mutex);
     memory_map_init();
     io_mem_init();
 #endif
@@ -2329,6 +2330,16 @@ void qemu_flush_coalesced_mmio_buffer(void)
         kvm_flush_coalesced_mmio_buffer();
 }

+void qemu_mutex_lock_ramlist(void)
+{
+    qemu_mutex_lock(&ram_list.mutex);
+}
+
+void qemu_mutex_unlock_ramlist(void)
+{
+    qemu_mutex_unlock(&ram_list.mutex);
+}
+
 #if defined(__linux__) && !defined(TARGET_S390X)

 #include <sys/vfs.h>
@@ -2510,6 +2521,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
     }
     pstrcat(new_block->idstr, sizeof(new_block->idstr), name);

+    qemu_mutex_lock_ramlist();
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (block != new_block && !strcmp(block->idstr, new_block->idstr)) {
             fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n",
@@ -2517,6 +2529,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
             abort();
         }
     }
+    qemu_mutex_unlock_ramlist();
 }

 static int memory_try_enable_merging(void *addr, size_t len)
@@ -2540,6 +2553,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
     size = TARGET_PAGE_ALIGN(size);
     new_block = g_malloc0(sizeof(*new_block));

+    qemu_mutex_lock_ramlist();
     new_block->mr = mr;
     new_block->offset = find_ram_offset(size);
     if (host) {
@@ -2575,6 +2589,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
     QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);

     ram_list.version++;
+    qemu_mutex_unlock_ramlist();

     ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);
@@ -2599,21 +2614,24 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
 {
     RAMBlock *block;

+    qemu_mutex_lock_ramlist();
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
             QLIST_REMOVE(block, next_mru);
             ram_list.version++;
             g_free(block);
-            return;
+            break;
         }
     }
+    qemu_mutex_unlock_ramlist();
 }

 void qemu_ram_free(ram_addr_t addr)
 {
     RAMBlock *block;

+    qemu_mutex_lock_ramlist();
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
@@ -2644,9 +2662,10 @@ void qemu_ram_free(ram_addr_t addr)
 #endif
             }
             g_free(block);
-            return;
+            break;
         }
     }
+    qemu_mutex_unlock_ramlist();

 }

-- 
1.7.11.7

[Qemu-devel] [PATCH 04/30] buffered_file: Move from using a timer to use a thread

[Juan Quintela]

We still protect everything except the wait with the iothread lock.
But we moved from a timer to a thread.  Steps one by one.

We also need to detect when we have finished with a variable "complete".

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 58 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index ed92df1..4b90d54 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -18,6 +18,7 @@
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "buffered_file.h"
+#include "qemu-thread.h"

 //#define DEBUG_BUFFERED_FILE

@@ -31,7 +32,8 @@ typedef struct QEMUFileBuffered
     uint8_t *buffer;
     size_t buffer_size;
     size_t buffer_capacity;
-    QEMUTimer *timer;
+    QemuThread thread;
+    bool complete;
 } QEMUFileBuffered;

 #ifdef DEBUG_BUFFERED_FILE
@@ -160,11 +162,8 @@ static int buffered_close(void *opaque)
     if (ret >= 0) {
         ret = ret2;
     }
-    qemu_del_timer(s->timer);
-    qemu_free_timer(s->timer);
-    g_free(s->buffer);
-    g_free(s);
-
+    ret = migrate_fd_close(s->migration_state);
+    s->complete = true;
     return ret;
 }

@@ -215,23 +214,38 @@ static int64_t buffered_get_rate_limit(void *opaque)
     return s->xfer_limit;
 }

-static void buffered_rate_tick(void *opaque)
+/* 10ms  xfer_limit is the limit that we should write each 10ms */
+#define BUFFER_DELAY 100
+
+static void *buffered_file_thread(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
+    int64_t expire_time = qemu_get_clock_ms(rt_clock) + BUFFER_DELAY;

-    if (qemu_file_get_error(s->file)) {
-        buffered_close(s);
-        return;
-    }
-
-    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
-
-    if (s->freeze_output)
-        return;
-
-    s->bytes_xfer = 0;
+    while (true) {
+        int64_t current_time = qemu_get_clock_ms(rt_clock);

-    buffered_put_buffer(s, NULL, 0, 0);
+        if (s->complete) {
+            break;
+        }
+        if (s->freeze_output) {
+            continue;
+        }
+        if (current_time >= expire_time) {
+            s->bytes_xfer = 0;
+            expire_time = current_time + BUFFER_DELAY;
+        }
+        if (s->bytes_xfer >= s->xfer_limit) {
+            /* usleep expects microseconds */
+            usleep((expire_time - current_time)*1000);
+        }
+        qemu_mutex_lock_iothread();
+        buffered_put_buffer(s, NULL, 0, 0);
+        qemu_mutex_unlock_iothread();
+    }
+    g_free(s->buffer);
+    g_free(s);
+    return NULL;
 }

 QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
@@ -242,15 +256,15 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)

     s->migration_state = migration_state;
     s->xfer_limit = migration_state->bandwidth_limit / 10;
+    s->complete = false;

     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
                              buffered_close, buffered_rate_limit,
                              buffered_set_rate_limit,
 			     buffered_get_rate_limit);

-    s->timer = qemu_new_timer_ms(rt_clock, buffered_rate_tick, s);
-
-    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
+    qemu_thread_create(&s->thread, buffered_file_thread, s,
+                       QEMU_THREAD_DETACHED);

     return s->file;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 05/30] migration: make qemu_fopen_ops_buffered() return void

[Juan Quintela]

We want the file assignment to happen before the thread is created to
avoid locking, so we just do it before creating the thread.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 13 ++++++-------
 buffered_file.h |  2 +-
 migration.c     |  2 +-
 migration.h     |  1 +
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 4b90d54..6395b37 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -33,7 +33,6 @@ typedef struct QEMUFileBuffered
     size_t buffer_size;
     size_t buffer_capacity;
     QemuThread thread;
-    bool complete;
 } QEMUFileBuffered;

 #ifdef DEBUG_BUFFERED_FILE
@@ -163,7 +162,7 @@ static int buffered_close(void *opaque)
         ret = ret2;
     }
     ret = migrate_fd_close(s->migration_state);
-    s->complete = true;
+    s->migration_state->complete = true;
     return ret;
 }

@@ -225,7 +224,7 @@ static void *buffered_file_thread(void *opaque)
     while (true) {
         int64_t current_time = qemu_get_clock_ms(rt_clock);

-        if (s->complete) {
+        if (s->migration_state->complete) {
             break;
         }
         if (s->freeze_output) {
@@ -248,7 +247,7 @@ static void *buffered_file_thread(void *opaque)
     return NULL;
 }

-QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
+void qemu_fopen_ops_buffered(MigrationState *migration_state)
 {
     QEMUFileBuffered *s;

@@ -256,15 +255,15 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)

     s->migration_state = migration_state;
     s->xfer_limit = migration_state->bandwidth_limit / 10;
-    s->complete = false;
+    s->migration_state->complete = false;

     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
                              buffered_close, buffered_rate_limit,
                              buffered_set_rate_limit,
 			     buffered_get_rate_limit);

+    migration_state->file = s->file;
+
     qemu_thread_create(&s->thread, buffered_file_thread, s,
                        QEMU_THREAD_DETACHED);
-
-    return s->file;
 }
diff --git a/buffered_file.h b/buffered_file.h
index ef010fe..8a246fd 100644
--- a/buffered_file.h
+++ b/buffered_file.h
@@ -17,6 +17,6 @@
 #include "hw/hw.h"
 #include "migration.h"

-QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state);
+void qemu_fopen_ops_buffered(MigrationState *migration_state);

 #endif
diff --git a/migration.c b/migration.c
index 62e0304..02f4ffa 100644
--- a/migration.c
+++ b/migration.c
@@ -431,7 +431,7 @@ void migrate_fd_connect(MigrationState *s)
     int ret;

     s->state = MIG_STATE_ACTIVE;
-    s->file = qemu_fopen_ops_buffered(s);
+    qemu_fopen_ops_buffered(s);

     DPRINTF("beginning savevm\n");
     ret = qemu_savevm_state_begin(s->file, &s->params);
diff --git a/migration.h b/migration.h
index 1c3e9b7..a63c5d5 100644
--- a/migration.h
+++ b/migration.h
@@ -45,6 +45,7 @@ struct MigrationState
     int64_t dirty_pages_rate;
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size;
+    bool complete;
 };

 void process_incoming_migration(QEMUFile *f);
-- 
1.7.11.7

[Qemu-devel] [PATCH 06/30] migration: stop all cpus correctly

[Juan Quintela]

You can only stop all cpus from the iothread or an vcpu.  As we want
to do it from the migration_thread, we need to do this dance with the
botton handlers.

This patch is a request for ideas.  I can move this function to cpus.c, but
wondered if there is an easy way of doing this?

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/migration.c b/migration.c
index 02f4ffa..05ef1a3 100644
--- a/migration.c
+++ b/migration.c
@@ -20,6 +20,7 @@
 #include "sysemu.h"
 #include "block.h"
 #include "qemu_socket.h"
+#include "qemu-thread.h"
 #include "block-migration.h"
 #include "qmp-commands.h"

@@ -322,11 +323,22 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
 void migrate_fd_put_ready(MigrationState *s)
 {
     int ret;
+    static bool first_time = true;

     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
         return;
     }
+    if (first_time) {
+        first_time = false;
+        DPRINTF("beginning savevm\n");
+        ret = qemu_savevm_state_begin(s->file, &s->params);
+        if (ret < 0) {
+            DPRINTF("failed, %d\n", ret);
+            migrate_fd_error(s);
+            return;
+        }
+    }

     DPRINTF("iterate\n");
     ret = qemu_savevm_state_iterate(s->file);
@@ -339,7 +351,11 @@ void migrate_fd_put_ready(MigrationState *s)
         DPRINTF("done iterating\n");
         start_time = qemu_get_clock_ms(rt_clock);
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
-        vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+        if (old_vm_running) {
+            vm_stop(RUN_STATE_FINISH_MIGRATE);
+        } else {
+            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+        }

         if (qemu_savevm_state_complete(s->file) < 0) {
             migrate_fd_error(s);
@@ -428,19 +444,8 @@ bool migration_has_failed(MigrationState *s)

 void migrate_fd_connect(MigrationState *s)
 {
-    int ret;
-
     s->state = MIG_STATE_ACTIVE;
     qemu_fopen_ops_buffered(s);
-
-    DPRINTF("beginning savevm\n");
-    ret = qemu_savevm_state_begin(s->file, &s->params);
-    if (ret < 0) {
-        DPRINTF("failed, %d\n", ret);
-        migrate_fd_error(s);
-        return;
-    }
-    migrate_fd_put_ready(s);
 }

 static MigrationState *migrate_init(const MigrationParams *params)
-- 
1.7.11.7

[Qemu-devel] [PATCH 07/30] migration: make writes blocking

[Juan Quintela]

Move all the writes to the migration_thread, and make writings
blocking.  Notice that are still using the iothread for everything
that we do.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c |  2 --
 migration-fd.c   |  6 ------
 migration-tcp.c  | 19 ++-----------------
 migration-unix.c |  2 --
 migration.c      | 21 ---------------------
 qemu-file.h      |  5 -----
 savevm.c         |  5 -----
 7 files changed, 2 insertions(+), 58 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 6c97db9..908f22e 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -76,8 +76,6 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
         goto err_after_open;
     }

-    socket_set_nonblock(s->fd);
-
     s->opaque = qemu_popen(f, "w");

     s->close = exec_close;
diff --git a/migration-fd.c b/migration-fd.c
index 7335167..aba1be7 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -81,11 +81,6 @@ int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
         goto err_after_get_fd;
     }

-    if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
-        DPRINTF("Unable to set nonblocking mode on file descriptor\n");
-        goto err_after_open;
-    }
-
     s->get_error = fd_errno;
     s->write = fd_write;
     s->close = fd_close;
@@ -93,7 +88,6 @@ int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
     migrate_fd_connect(s);
     return 0;

-err_after_open:
     close(s->fd);
 err_after_get_fd:
     return -1;
diff --git a/migration-tcp.c b/migration-tcp.c
index a15c2b8..812ae22 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,21 +53,6 @@ static int tcp_close(MigrationState *s)
     return r;
 }

-static void tcp_wait_for_connect(int fd, void *opaque)
-{
-    MigrationState *s = opaque;
-
-    if (fd < 0) {
-        DPRINTF("migrate connect error\n");
-        s->fd = -1;
-        migrate_fd_error(s);
-    } else {
-        DPRINTF("migrate connect success\n");
-        s->fd = fd;
-        migrate_fd_connect(s);
-    }
-}
-
 int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
                                  Error **errp)
 {
@@ -75,12 +60,12 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
     s->write = socket_write;
     s->close = tcp_close;

-    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
-                                     errp);
+    s->fd = inet_connect(host_port, errp);
     if (error_is_set(errp)) {
         migrate_fd_error(s);
         return -1;
     }
+    migrate_fd_connect(s);

     return 0;
 }
diff --git a/migration-unix.c b/migration-unix.c
index 169de88..bb41bac 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -96,8 +96,6 @@ int unix_start_outgoing_migration(MigrationState *s, const char *path)
         return -errno;
     }

-    socket_set_nonblock(s->fd);
-
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
         if (ret == -1) {
diff --git a/migration.c b/migration.c
index 05ef1a3..a8b2f4a 100644
--- a/migration.c
+++ b/migration.c
@@ -247,10 +247,6 @@ static int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;

-    if (s->fd != -1) {
-        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    }
-
     if (s->file) {
         DPRINTF("closing file\n");
         ret = qemu_fclose(s->file);
@@ -285,18 +281,6 @@ static void migrate_fd_completed(MigrationState *s)
     notifier_list_notify(&migration_state_notifiers, s);
 }

-static void migrate_fd_put_notify(void *opaque)
-{
-    MigrationState *s = opaque;
-    int ret;
-
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-    ret = qemu_file_put_notify(s->file);
-    if (ret) {
-        migrate_fd_error(s);
-    }
-}
-
 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size)
 {
@@ -313,10 +297,6 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
     if (ret == -1)
         ret = -(s->get_error(s));

-    if (ret == -EAGAIN) {
-        qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
-    }
-
     return ret;
 }

@@ -412,7 +392,6 @@ int migrate_fd_wait_for_unfreeze(MigrationState *s)

 int migrate_fd_close(MigrationState *s)
 {
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     return s->close(s);
 }

diff --git a/qemu-file.h b/qemu-file.h
index 9c8985b..e88892c 100644
--- a/qemu-file.h
+++ b/qemu-file.h
@@ -104,11 +104,6 @@ int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
 int64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_get_error(QEMUFile *f);

-/* Try to send any outstanding data.  This function is useful when output is
- * halted due to rate limiting or EAGAIN errors occur as it can be used to
- * resume output. */
-int qemu_file_put_notify(QEMUFile *f);
-
 static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
 {
     qemu_put_be64(f, *pv);
diff --git a/savevm.c b/savevm.c
index b080d37..69f1768 100644
--- a/savevm.c
+++ b/savevm.c
@@ -523,11 +523,6 @@ int qemu_fclose(QEMUFile *f)
     return ret;
 }

-int qemu_file_put_notify(QEMUFile *f)
-{
-    return f->put_buffer(f->opaque, NULL, 0, 0);
-}
-
 void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
 {
     int l;
-- 
1.7.11.7

[Qemu-devel] [PATCH 08/30] migration: remove unfreeze logic

[Juan Quintela]

Now that we have a thread, and blocking writes, we don't need it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 24 +-----------------------
 migration.c     | 23 -----------------------
 migration.h     |  1 -
 3 files changed, 1 insertion(+), 47 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 6395b37..876cc8c 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -26,7 +26,6 @@ typedef struct QEMUFileBuffered
 {
     MigrationState *migration_state;
     QEMUFile *file;
-    int freeze_output;
     size_t bytes_xfer;
     size_t xfer_limit;
     uint8_t *buffer;
@@ -70,13 +69,6 @@ static ssize_t buffered_flush(QEMUFileBuffered *s)

         ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
                                     s->buffer_size - offset);
-        if (ret == -EAGAIN) {
-            DPRINTF("backend not ready, freezing\n");
-            ret = 0;
-            s->freeze_output = 1;
-            break;
-        }
-
         if (ret <= 0) {
             DPRINTF("error flushing data, %zd\n", ret);
             break;
@@ -110,9 +102,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         return error;
     }

-    DPRINTF("unfreezing output\n");
-    s->freeze_output = 0;
-
     if (size > 0) {
         DPRINTF("buffering %d bytes\n", size - offset);
         buffered_append(s, buf, size);
@@ -126,7 +115,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in

     if (pos == 0 && size == 0) {
         DPRINTF("file is ready\n");
-        if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
+        if (s->bytes_xfer < s->xfer_limit) {
             DPRINTF("notifying client\n");
             migrate_fd_put_ready(s->migration_state);
         }
@@ -149,12 +138,6 @@ static int buffered_close(void *opaque)
         if (ret < 0) {
             break;
         }
-        if (s->freeze_output) {
-            ret = migrate_fd_wait_for_unfreeze(s->migration_state);
-            if (ret < 0) {
-                break;
-            }
-        }
     }

     ret2 = migrate_fd_close(s->migration_state);
@@ -181,8 +164,6 @@ static int buffered_rate_limit(void *opaque)
     if (ret) {
         return ret;
     }
-    if (s->freeze_output)
-        return 1;

     if (s->bytes_xfer > s->xfer_limit)
         return 1;
@@ -227,9 +208,6 @@ static void *buffered_file_thread(void *opaque)
         if (s->migration_state->complete) {
             break;
         }
-        if (s->freeze_output) {
-            continue;
-        }
         if (current_time >= expire_time) {
             s->bytes_xfer = 0;
             expire_time = current_time + BUFFER_DELAY;
diff --git a/migration.c b/migration.c
index a8b2f4a..29ee710 100644
--- a/migration.c
+++ b/migration.c
@@ -367,29 +367,6 @@ static void migrate_fd_cancel(MigrationState *s)
     migrate_fd_cleanup(s);
 }

-int migrate_fd_wait_for_unfreeze(MigrationState *s)
-{
-    int ret;
-
-    DPRINTF("wait for unfreeze\n");
-    if (s->state != MIG_STATE_ACTIVE)
-        return -EINVAL;
-
-    do {
-        fd_set wfds;
-
-        FD_ZERO(&wfds);
-        FD_SET(s->fd, &wfds);
-
-        ret = select(s->fd + 1, NULL, &wfds, NULL, NULL);
-    } while (ret == -1 && (s->get_error(s)) == EINTR);
-
-    if (ret == -1) {
-        return -s->get_error(s);
-    }
-    return 0;
-}
-
 int migrate_fd_close(MigrationState *s)
 {
     return s->close(s);
diff --git a/migration.h b/migration.h
index a63c5d5..505f792 100644
--- a/migration.h
+++ b/migration.h
@@ -82,7 +82,6 @@ void migrate_fd_connect(MigrationState *s);
 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
 void migrate_fd_put_ready(MigrationState *s);
-int migrate_fd_wait_for_unfreeze(MigrationState *s);
 int migrate_fd_close(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
-- 
1.7.11.7

[Qemu-devel] [PATCH 09/30] migration: take finer locking

[Juan Quintela]

Instead of locking the whole migration_thread inside loop, just lock
migration_fd_put_notify, that is what interacts with the rest of the
world.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 2 --
 migration.c     | 5 +++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 876cc8c..a3aba2a 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -216,9 +216,7 @@ static void *buffered_file_thread(void *opaque)
             /* usleep expects microseconds */
             usleep((expire_time - current_time)*1000);
         }
-        qemu_mutex_lock_iothread();
         buffered_put_buffer(s, NULL, 0, 0);
-        qemu_mutex_unlock_iothread();
     }
     g_free(s->buffer);
     g_free(s);
diff --git a/migration.c b/migration.c
index 29ee710..82c2663 100644
--- a/migration.c
+++ b/migration.c
@@ -305,8 +305,10 @@ void migrate_fd_put_ready(MigrationState *s)
     int ret;
     static bool first_time = true;

+    qemu_mutex_lock_iothread();
     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
+        qemu_mutex_unlock_iothread();
         return;
     }
     if (first_time) {
@@ -316,6 +318,7 @@ void migrate_fd_put_ready(MigrationState *s)
         if (ret < 0) {
             DPRINTF("failed, %d\n", ret);
             migrate_fd_error(s);
+            qemu_mutex_unlock_iothread();
             return;
         }
     }
@@ -351,6 +354,8 @@ void migrate_fd_put_ready(MigrationState *s)
             }
         }
     }
+    qemu_mutex_unlock_iothread();
+
 }

 static void migrate_fd_cancel(MigrationState *s)
-- 
1.7.11.7

[Qemu-devel] [PATCH 10/30] buffered_file: Unfold the trick to restart generating migration data

[Juan Quintela]

This was needed before due to the way that the callbacks worked.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index a3aba2a..7692950 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -113,14 +113,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         return error;
     }

-    if (pos == 0 && size == 0) {
-        DPRINTF("file is ready\n");
-        if (s->bytes_xfer < s->xfer_limit) {
-            DPRINTF("notifying client\n");
-            migrate_fd_put_ready(s->migration_state);
-        }
-    }
-
     return size;
 }

@@ -216,8 +208,15 @@ static void *buffered_file_thread(void *opaque)
             /* usleep expects microseconds */
             usleep((expire_time - current_time)*1000);
         }
-        buffered_put_buffer(s, NULL, 0, 0);
+        buffered_flush(s);
+
+        DPRINTF("file is ready\n");
+        if (s->bytes_xfer < s->xfer_limit) {
+            DPRINTF("notifying client\n");
+            migrate_fd_put_ready(s->migration_state);
+        }
     }
+
     g_free(s->buffer);
     g_free(s);
     return NULL;
-- 
1.7.11.7

[Qemu-devel] [PATCH 11/30] buffered_file: don't flush on put buffer

[Juan Quintela]

We call buffered_put_buffer with iothread held, and buffered_flush() does
synchronous writes.  We only want to do the synchronous writes outside.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 7692950..f508026 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -107,12 +107,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         buffered_append(s, buf, size);
     }

-    error = buffered_flush(s);
-    if (error < 0) {
-        DPRINTF("buffered flush error. bailing: %s\n", strerror(-error));
-        return error;
-    }
-
     return size;
 }

-- 
1.7.11.7

[Qemu-devel] [PATCH 12/30] buffered_file: unfold buffered_append in buffered_put_buffer

[Juan Quintela]

It was the only user, and now buffered_put_buffer just do the append

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index f508026..fcb3907 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -42,22 +42,6 @@ typedef struct QEMUFileBuffered
     do { } while (0)
 #endif

-static void buffered_append(QEMUFileBuffered *s,
-                            const uint8_t *buf, size_t size)
-{
-    if (size > (s->buffer_capacity - s->buffer_size)) {
-        DPRINTF("increasing buffer capacity from %zu by %zu\n",
-                s->buffer_capacity, size + 1024);
-
-        s->buffer_capacity += size + 1024;
-
-        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
-    }
-
-    memcpy(s->buffer + s->buffer_size, buf, size);
-    s->buffer_size += size;
-}
-
 static ssize_t buffered_flush(QEMUFileBuffered *s)
 {
     size_t offset = 0;
@@ -102,11 +86,22 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         return error;
     }

-    if (size > 0) {
-        DPRINTF("buffering %d bytes\n", size - offset);
-        buffered_append(s, buf, size);
+    if (size <= 0) {
+        return size;
     }

+    if (size > (s->buffer_capacity - s->buffer_size)) {
+        DPRINTF("increasing buffer capacity from %zu by %zu\n",
+                s->buffer_capacity, size + 1024);
+
+        s->buffer_capacity += size + 1024;
+
+        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
+    }
+
+    memcpy(s->buffer + s->buffer_size, buf, size);
+    s->buffer_size += size;
+
     return size;
 }

-- 
1.7.11.7

[Qemu-devel] [PATCH 13/30] savevm: New save live migration method: pending

[Juan Quintela]

Code just now does (simplified for clarity)

    if (qemu_savevm_state_iterate(s->file) == 1) {
       vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
       qemu_savevm_state_complete(s->file);
    }

Problem here is that qemu_savevm_state_iterate() returns 1 when it
knows that remaining memory to sent takes less than max downtime.

But this means that we could end spending 2x max_downtime, one
downtime in qemu_savevm_iterate, and the other in
qemu_savevm_state_complete.

Changed code to:

    pending_size = qemu_savevm_state_pending(s->file, max_size);
    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
    if (pending_size >= max_size) {
        ret = qemu_savevm_state_iterate(s->file);
     } else {
        vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
        qemu_savevm_state_complete(s->file);
     }

So what we do is: at current network speed, we calculate the maximum
number of bytes we can sent: max_size.

Then we ask every save_live section how much they have pending.  If
they are less than max_size, we move to complete phase, otherwise we
do an iterate one.

This makes things much simpler, because now individual sections don't
have to caluclate the bandwidth (it was implossible to do right from
there).

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c       | 48 ++++++++++++++++++------------------------------
 block-migration.c | 49 ++++++++++---------------------------------------
 buffered_file.c   | 25 ++++++++++++++++++-------
 migration.c       | 22 +++++++++++++++-------
 migration.h       |  2 +-
 savevm.c          | 19 +++++++++++++++++++
 sysemu.h          |  1 +
 vmstate.h         |  1 +
 8 files changed, 83 insertions(+), 84 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 2d29828..1eefef8 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -608,12 +608,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)

 static int ram_save_iterate(QEMUFile *f, void *opaque)
 {
-    uint64_t bytes_transferred_last;
-    double bwidth = 0;
     int ret;
     int i;
-    uint64_t expected_downtime;
-    MigrationState *s = migrate_get_current();
+    int64_t t0;

     qemu_mutex_lock_ramlist();

@@ -621,9 +618,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         reset_ram_globals();
     }

-    bytes_transferred_last = bytes_transferred;
-    bwidth = qemu_get_clock_ns(rt_clock);
-
+    t0 = qemu_get_clock_ns(rt_clock);
     i = 0;
     while ((ret = qemu_file_rate_limit(f)) == 0) {
         int bytes_sent;
@@ -641,7 +636,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
            iterations
         */
         if ((i & 63) == 0) {
-            uint64_t t1 = (qemu_get_clock_ns(rt_clock) - bwidth) / 1000000;
+            uint64_t t1 = (qemu_get_clock_ns(rt_clock) - t0) / 1000000;
             if (t1 > MAX_WAIT) {
                 DPRINTF("big wait: %" PRIu64 " milliseconds, %d iterations\n",
                         t1, i);
@@ -655,31 +650,10 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         return ret;
     }

-    bwidth = qemu_get_clock_ns(rt_clock) - bwidth;
-    bwidth = (bytes_transferred - bytes_transferred_last) / bwidth;
-
-    /* if we haven't transferred anything this round, force
-     * expected_downtime to a very high value, but without
-     * crashing */
-    if (bwidth == 0) {
-        bwidth = 0.000001;
-    }
-
     qemu_mutex_unlock_ramlist();
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);

-    expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
-    DPRINTF("ram_save_live: expected(%" PRIu64 ") <= max(" PRIu64 ")?\n",
-            expected_downtime, migrate_max_downtime());
-
-    if (expected_downtime <= migrate_max_downtime()) {
-        migration_bitmap_sync();
-        expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
-        s->expected_downtime = expected_downtime / 1000000; /* ns -> ms */
-
-        return expected_downtime <= migrate_max_downtime();
-    }
-    return 0;
+    return i;
 }

 static int ram_save_complete(QEMUFile *f, void *opaque)
@@ -712,6 +686,19 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     return 0;
 }

+static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size)
+{
+    uint64_t remaining_size;
+
+    remaining_size = ram_save_remaining() * TARGET_PAGE_SIZE;
+
+    if (remaining_size < max_size) {
+        migration_bitmap_sync();
+        remaining_size = ram_save_remaining() * TARGET_PAGE_SIZE;
+    }
+    return remaining_size;
+}
+
 static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
 {
     int ret, rc = 0;
@@ -897,6 +884,7 @@ SaveVMHandlers savevm_ram_handlers = {
     .save_live_setup = ram_save_setup,
     .save_live_iterate = ram_save_iterate,
     .save_live_complete = ram_save_complete,
+    .save_live_pending = ram_save_pending,
     .load_state = ram_load,
     .cancel = ram_migration_cancel,
 };
diff --git a/block-migration.c b/block-migration.c
index 71b9601..5db01fe 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -77,9 +77,7 @@ typedef struct BlkMigState {
     int64_t total_sector_sum;
     int prev_progress;
     int bulk_completed;
-    long double total_time;
     long double prev_time_offset;
-    int reads;
 } BlkMigState;

 static BlkMigState block_mig_state;
@@ -132,12 +130,6 @@ uint64_t blk_mig_bytes_total(void)
     return sum << BDRV_SECTOR_BITS;
 }

-static inline long double compute_read_bwidth(void)
-{
-    assert(block_mig_state.total_time != 0);
-    return (block_mig_state.reads / block_mig_state.total_time) * BLOCK_SIZE;
-}
-
 static int bmds_aio_inflight(BlkMigDevState *bmds, int64_t sector)
 {
     int64_t chunk = sector / (int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK;
@@ -191,8 +183,6 @@ static void blk_mig_read_cb(void *opaque, int ret)

     blk->ret = ret;

-    block_mig_state.reads++;
-    block_mig_state.total_time += (curr_time - block_mig_state.prev_time_offset);
     block_mig_state.prev_time_offset = curr_time;

     QSIMPLEQ_INSERT_TAIL(&block_mig_state.blk_list, blk, entry);
@@ -310,8 +300,6 @@ static void init_blk_migration(QEMUFile *f)
     block_mig_state.total_sector_sum = 0;
     block_mig_state.prev_progress = -1;
     block_mig_state.bulk_completed = 0;
-    block_mig_state.total_time = 0;
-    block_mig_state.reads = 0;

     bdrv_iterate(init_blk_migration_it, NULL);
 }
@@ -493,32 +481,6 @@ static int64_t get_remaining_dirty(void)
     return dirty * BLOCK_SIZE;
 }

-static int is_stage2_completed(void)
-{
-    int64_t remaining_dirty;
-    long double bwidth;
-
-    if (block_mig_state.bulk_completed == 1) {
-
-        remaining_dirty = get_remaining_dirty();
-        if (remaining_dirty == 0) {
-            return 1;
-        }
-
-        bwidth = compute_read_bwidth();
-
-        if ((remaining_dirty / bwidth) <=
-            migrate_max_downtime()) {
-            /* finish stage2 because we think that we can finish remaining work
-               below max_downtime */
-
-            return 1;
-        }
-    }
-
-    return 0;
-}
-
 static void blk_mig_cleanup(void)
 {
     BlkMigDevState *bmds;
@@ -619,7 +581,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)

     qemu_put_be64(f, BLK_MIG_FLAG_EOS);

-    return is_stage2_completed();
+    return 0;
 }

 static int block_save_complete(QEMUFile *f, void *opaque)
@@ -659,6 +621,14 @@ static int block_save_complete(QEMUFile *f, void *opaque)
     return 0;
 }

+static uint64_t block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size)
+{
+
+    DPRINTF("Enter save live pending  %ld\n", get_remaining_dirty());
+
+    return get_remaining_dirty();
+}
+
 static int block_load(QEMUFile *f, void *opaque, int version_id)
 {
     static int banner_printed;
@@ -755,6 +725,7 @@ SaveVMHandlers savevm_block_handlers = {
     .save_live_setup = block_save_setup,
     .save_live_iterate = block_save_iterate,
     .save_live_complete = block_save_complete,
+    .save_live_pending = block_save_pending,
     .load_state = block_load,
     .cancel = block_migration_cancel,
     .is_active = block_is_active,
diff --git a/buffered_file.c b/buffered_file.c
index fcb3907..c21f847 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -175,13 +175,15 @@ static int64_t buffered_get_rate_limit(void *opaque)
     return s->xfer_limit;
 }

-/* 10ms  xfer_limit is the limit that we should write each 10ms */
+/* 100ms  xfer_limit is the limit that we should write each 100ms */
 #define BUFFER_DELAY 100

 static void *buffered_file_thread(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
-    int64_t expire_time = qemu_get_clock_ms(rt_clock) + BUFFER_DELAY;
+    int64_t initial_time = qemu_get_clock_ms(rt_clock);
+    int64_t max_size = 0;
+    bool last_round = false;

     while (true) {
         int64_t current_time = qemu_get_clock_ms(rt_clock);
@@ -189,20 +191,29 @@ static void *buffered_file_thread(void *opaque)
         if (s->migration_state->complete) {
             break;
         }
-        if (current_time >= expire_time) {
+        if (current_time >= initial_time + BUFFER_DELAY) {
+            uint64_t transferred_bytes = s->bytes_xfer;
+            uint64_t time_spent = current_time - initial_time;
+            double bandwidth = transferred_bytes / time_spent;
+            max_size = bandwidth * migrate_max_downtime() / 1000000;
+
+            DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
+                    " bandwidth %g max_size %" PRId64 "\n",
+                    transferred_bytes, time_spent, bandwidth, max_size);
+
             s->bytes_xfer = 0;
-            expire_time = current_time + BUFFER_DELAY;
+            initial_time = current_time;
         }
-        if (s->bytes_xfer >= s->xfer_limit) {
+        if (!last_round && (s->bytes_xfer >= s->xfer_limit)) {
             /* usleep expects microseconds */
-            usleep((expire_time - current_time)*1000);
+            usleep((initial_time + BUFFER_DELAY - current_time)*1000);
         }
         buffered_flush(s);

         DPRINTF("file is ready\n");
         if (s->bytes_xfer < s->xfer_limit) {
             DPRINTF("notifying client\n");
-            migrate_fd_put_ready(s->migration_state);
+            last_round = migrate_fd_put_ready(s->migration_state, max_size);
         }
     }

diff --git a/migration.c b/migration.c
index 82c2663..8054a77 100644
--- a/migration.c
+++ b/migration.c
@@ -300,16 +300,18 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
     return ret;
 }

-void migrate_fd_put_ready(MigrationState *s)
+bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
 {
     int ret;
     static bool first_time = true;
+    uint64_t pending_size;
+    bool last_round = false;

     qemu_mutex_lock_iothread();
     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
         qemu_mutex_unlock_iothread();
-        return;
+        return false;
     }
     if (first_time) {
         first_time = false;
@@ -319,15 +321,19 @@ void migrate_fd_put_ready(MigrationState *s)
             DPRINTF("failed, %d\n", ret);
             migrate_fd_error(s);
             qemu_mutex_unlock_iothread();
-            return;
+            return false;
         }
     }

     DPRINTF("iterate\n");
-    ret = qemu_savevm_state_iterate(s->file);
-    if (ret < 0) {
-        migrate_fd_error(s);
-    } else if (ret == 1) {
+    pending_size = qemu_savevm_state_pending(s->file, max_size);
+    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
+    if (pending_size >= max_size) {
+        ret = qemu_savevm_state_iterate(s->file);
+        if (ret < 0) {
+            migrate_fd_error(s);
+        }
+    } else {
         int old_vm_running = runstate_is_running();
         int64_t start_time, end_time;

@@ -353,9 +359,11 @@ void migrate_fd_put_ready(MigrationState *s)
                 vm_start();
             }
         }
+        last_round = true;
     }
     qemu_mutex_unlock_iothread();

+    return last_round;
 }

 static void migrate_fd_cancel(MigrationState *s)
diff --git a/migration.h b/migration.h
index 505f792..1f2baed 100644
--- a/migration.h
+++ b/migration.h
@@ -81,7 +81,7 @@ void migrate_fd_connect(MigrationState *s);

 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
-void migrate_fd_put_ready(MigrationState *s);
+bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size);
 int migrate_fd_close(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
diff --git a/savevm.c b/savevm.c
index 69f1768..a68c37b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1681,6 +1681,25 @@ int qemu_savevm_state_complete(QEMUFile *f)
     return qemu_file_get_error(f);
 }

+uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size)
+{
+    SaveStateEntry *se;
+    uint64_t ret = 0;
+
+    QTAILQ_FOREACH(se, &savevm_handlers, entry) {
+        if (!se->ops || !se->ops->save_live_pending) {
+            continue;
+        }
+        if (se->ops && se->ops->is_active) {
+            if (!se->ops->is_active(se->opaque)) {
+                continue;
+            }
+        }
+        ret += se->ops->save_live_pending(f, se->opaque, max_size);
+    }
+    return ret;
+}
+
 void qemu_savevm_state_cancel(QEMUFile *f)
 {
     SaveStateEntry *se;
diff --git a/sysemu.h b/sysemu.h
index 0c39a3a..0276205 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -83,6 +83,7 @@ int qemu_savevm_state_begin(QEMUFile *f,
 int qemu_savevm_state_iterate(QEMUFile *f);
 int qemu_savevm_state_complete(QEMUFile *f);
 void qemu_savevm_state_cancel(QEMUFile *f);
+uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
 int qemu_loadvm_state(QEMUFile *f);

 /* SLIRP */
diff --git a/vmstate.h b/vmstate.h
index c9c320e..241d962 100644
--- a/vmstate.h
+++ b/vmstate.h
@@ -35,6 +35,7 @@ typedef struct SaveVMHandlers {
     int (*save_live_setup)(QEMUFile *f, void *opaque);
     int (*save_live_iterate)(QEMUFile *f, void *opaque);
     int (*save_live_complete)(QEMUFile *f, void *opaque);
+    uint64_t (*save_live_pending)(QEMUFile *f, void *opaque, uint64_t max_size);
     void (*cancel)(void *opaque);
     LoadStateHandler *load_state;
     bool (*is_active)(void *opaque);
-- 
1.7.11.7

[Qemu-devel] [PATCH 14/30] migration: include qemu-file.h

[Juan Quintela]

They don't use/know anything about buffered-file.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c | 2 +-
 migration-fd.c   | 2 +-
 migration-tcp.c  | 2 +-
 migration-unix.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 908f22e..c451545 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -19,7 +19,7 @@
 #include "qemu_socket.h"
 #include "migration.h"
 #include "qemu-char.h"
-#include "buffered_file.h"
+#include "qemu-file.h"
 #include "block.h"
 #include <sys/types.h>
 #include <sys/wait.h>
diff --git a/migration-fd.c b/migration-fd.c
index aba1be7..25f0245 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -18,7 +18,7 @@
 #include "migration.h"
 #include "monitor.h"
 #include "qemu-char.h"
-#include "buffered_file.h"
+#include "qemu-file.h"
 #include "block.h"
 #include "qemu_socket.h"

diff --git a/migration-tcp.c b/migration-tcp.c
index 812ae22..fdf8b0f 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -17,7 +17,7 @@
 #include "qemu_socket.h"
 #include "migration.h"
 #include "qemu-char.h"
-#include "buffered_file.h"
+#include "qemu-file.h"
 #include "block.h"

 //#define DEBUG_MIGRATION_TCP
diff --git a/migration-unix.c b/migration-unix.c
index bb41bac..64bc0c3 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -17,7 +17,7 @@
 #include "qemu_socket.h"
 #include "migration.h"
 #include "qemu-char.h"
-#include "buffered_file.h"
+#include "qemu-file.h"
 #include "block.h"

 //#define DEBUG_MIGRATION_UNIX
-- 
1.7.11.7

[Qemu-devel] [PATCH 15/30] migration-fd: remove duplicate include

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-fd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration-fd.c b/migration-fd.c
index 25f0245..8f6b55f 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -20,7 +20,6 @@
 #include "qemu-char.h"
 #include "qemu-file.h"
 #include "block.h"
-#include "qemu_socket.h"

 //#define DEBUG_MIGRATION_FD

-- 
1.7.11.7

[Qemu-devel] [PATCH 16/30] migration: move buffered_file.c code into migration.c

[Juan Quintela]

This only moves the code (also from buffered_file.h to migration.h).
Fix whitespace until checkpatch is happy.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 Makefile.objs   |   2 +-
 buffered_file.c | 244 --------------------------------------------------------
 buffered_file.h |  22 -----
 migration.c     | 218 +++++++++++++++++++++++++++++++++++++++++++++++++-
 migration.h     |   1 +
 5 files changed, 219 insertions(+), 268 deletions(-)
 delete mode 100644 buffered_file.c
 delete mode 100644 buffered_file.h

diff --git a/Makefile.objs b/Makefile.objs
index 74b3542..3de8f27 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -73,7 +73,7 @@ extra-obj-$(CONFIG_LINUX) += fsdev/

 common-obj-y += tcg-runtime.o host-utils.o main-loop.o
 common-obj-y += input.o
-common-obj-y += buffered_file.o migration.o migration-tcp.o
+common-obj-y += migration.o migration-tcp.o
 common-obj-y += qemu-char.o #aio.o
 common-obj-y += block-migration.o iohandler.o
 common-obj-y += pflib.o
diff --git a/buffered_file.c b/buffered_file.c
deleted file mode 100644
index c21f847..0000000
--- a/buffered_file.c
+++ /dev/null
@@ -1,244 +0,0 @@
-/*
- * QEMU buffered QEMUFile
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- * Contributions after 2012-01-13 are licensed under the terms of the
- * GNU GPL, version 2 or (at your option) any later version.
- */
-
-#include "qemu-common.h"
-#include "hw/hw.h"
-#include "qemu-timer.h"
-#include "qemu-char.h"
-#include "buffered_file.h"
-#include "qemu-thread.h"
-
-//#define DEBUG_BUFFERED_FILE
-
-typedef struct QEMUFileBuffered
-{
-    MigrationState *migration_state;
-    QEMUFile *file;
-    size_t bytes_xfer;
-    size_t xfer_limit;
-    uint8_t *buffer;
-    size_t buffer_size;
-    size_t buffer_capacity;
-    QemuThread thread;
-} QEMUFileBuffered;
-
-#ifdef DEBUG_BUFFERED_FILE
-#define DPRINTF(fmt, ...) \
-    do { printf("buffered-file: " fmt, ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-    do { } while (0)
-#endif
-
-static ssize_t buffered_flush(QEMUFileBuffered *s)
-{
-    size_t offset = 0;
-    ssize_t ret = 0;
-
-    DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
-
-    while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
-
-        ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
-                                    s->buffer_size - offset);
-        if (ret <= 0) {
-            DPRINTF("error flushing data, %zd\n", ret);
-            break;
-        } else {
-            DPRINTF("flushed %zd byte(s)\n", ret);
-            offset += ret;
-            s->bytes_xfer += ret;
-        }
-    }
-
-    DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
-    memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
-    s->buffer_size -= offset;
-
-    if (ret < 0) {
-        return ret;
-    }
-    return offset;
-}
-
-static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
-{
-    QEMUFileBuffered *s = opaque;
-    ssize_t error;
-
-    DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
-
-    error = qemu_file_get_error(s->file);
-    if (error) {
-        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
-        return error;
-    }
-
-    if (size <= 0) {
-        return size;
-    }
-
-    if (size > (s->buffer_capacity - s->buffer_size)) {
-        DPRINTF("increasing buffer capacity from %zu by %zu\n",
-                s->buffer_capacity, size + 1024);
-
-        s->buffer_capacity += size + 1024;
-
-        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
-    }
-
-    memcpy(s->buffer + s->buffer_size, buf, size);
-    s->buffer_size += size;
-
-    return size;
-}
-
-static int buffered_close(void *opaque)
-{
-    QEMUFileBuffered *s = opaque;
-    ssize_t ret = 0;
-    int ret2;
-
-    DPRINTF("closing\n");
-
-    s->xfer_limit = INT_MAX;
-    while (!qemu_file_get_error(s->file) && s->buffer_size) {
-        ret = buffered_flush(s);
-        if (ret < 0) {
-            break;
-        }
-    }
-
-    ret2 = migrate_fd_close(s->migration_state);
-    if (ret >= 0) {
-        ret = ret2;
-    }
-    ret = migrate_fd_close(s->migration_state);
-    s->migration_state->complete = true;
-    return ret;
-}
-
-/*
- * The meaning of the return values is:
- *   0: We can continue sending
- *   1: Time to stop
- *   negative: There has been an error
- */
-static int buffered_rate_limit(void *opaque)
-{
-    QEMUFileBuffered *s = opaque;
-    int ret;
-
-    ret = qemu_file_get_error(s->file);
-    if (ret) {
-        return ret;
-    }
-
-    if (s->bytes_xfer > s->xfer_limit)
-        return 1;
-
-    return 0;
-}
-
-static int64_t buffered_set_rate_limit(void *opaque, int64_t new_rate)
-{
-    QEMUFileBuffered *s = opaque;
-    if (qemu_file_get_error(s->file)) {
-        goto out;
-    }
-    if (new_rate > SIZE_MAX) {
-        new_rate = SIZE_MAX;
-    }
-
-    s->xfer_limit = new_rate / 10;
-    
-out:
-    return s->xfer_limit;
-}
-
-static int64_t buffered_get_rate_limit(void *opaque)
-{
-    QEMUFileBuffered *s = opaque;
-  
-    return s->xfer_limit;
-}
-
-/* 100ms  xfer_limit is the limit that we should write each 100ms */
-#define BUFFER_DELAY 100
-
-static void *buffered_file_thread(void *opaque)
-{
-    QEMUFileBuffered *s = opaque;
-    int64_t initial_time = qemu_get_clock_ms(rt_clock);
-    int64_t max_size = 0;
-    bool last_round = false;
-
-    while (true) {
-        int64_t current_time = qemu_get_clock_ms(rt_clock);
-
-        if (s->migration_state->complete) {
-            break;
-        }
-        if (current_time >= initial_time + BUFFER_DELAY) {
-            uint64_t transferred_bytes = s->bytes_xfer;
-            uint64_t time_spent = current_time - initial_time;
-            double bandwidth = transferred_bytes / time_spent;
-            max_size = bandwidth * migrate_max_downtime() / 1000000;
-
-            DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
-                    " bandwidth %g max_size %" PRId64 "\n",
-                    transferred_bytes, time_spent, bandwidth, max_size);
-
-            s->bytes_xfer = 0;
-            initial_time = current_time;
-        }
-        if (!last_round && (s->bytes_xfer >= s->xfer_limit)) {
-            /* usleep expects microseconds */
-            usleep((initial_time + BUFFER_DELAY - current_time)*1000);
-        }
-        buffered_flush(s);
-
-        DPRINTF("file is ready\n");
-        if (s->bytes_xfer < s->xfer_limit) {
-            DPRINTF("notifying client\n");
-            last_round = migrate_fd_put_ready(s->migration_state, max_size);
-        }
-    }
-
-    g_free(s->buffer);
-    g_free(s);
-    return NULL;
-}
-
-void qemu_fopen_ops_buffered(MigrationState *migration_state)
-{
-    QEMUFileBuffered *s;
-
-    s = g_malloc0(sizeof(*s));
-
-    s->migration_state = migration_state;
-    s->xfer_limit = migration_state->bandwidth_limit / 10;
-    s->migration_state->complete = false;
-
-    s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
-                             buffered_close, buffered_rate_limit,
-                             buffered_set_rate_limit,
-			     buffered_get_rate_limit);
-
-    migration_state->file = s->file;
-
-    qemu_thread_create(&s->thread, buffered_file_thread, s,
-                       QEMU_THREAD_DETACHED);
-}
diff --git a/buffered_file.h b/buffered_file.h
deleted file mode 100644
index 8a246fd..0000000
--- a/buffered_file.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * QEMU buffered QEMUFile
- *
- * Copyright IBM, Corp. 2008
- *
- * Authors:
- *  Anthony Liguori   <aliguori@us.ibm.com>
- *
- * This work is licensed under the terms of the GNU GPL, version 2.  See
- * the COPYING file in the top-level directory.
- *
- */
-
-#ifndef QEMU_BUFFERED_FILE_H
-#define QEMU_BUFFERED_FILE_H
-
-#include "hw/hw.h"
-#include "migration.h"
-
-void qemu_fopen_ops_buffered(MigrationState *migration_state);
-
-#endif
diff --git a/migration.c b/migration.c
index 8054a77..b1567f3 100644
--- a/migration.c
+++ b/migration.c
@@ -16,7 +16,7 @@
 #include "qemu-common.h"
 #include "migration.h"
 #include "monitor.h"
-#include "buffered_file.h"
+#include "qemu-file.h"
 #include "sysemu.h"
 #include "block.h"
 #include "qemu_socket.h"
@@ -569,3 +569,219 @@ int64_t migrate_xbzrle_cache_size(void)

     return s->xbzrle_cache_size;
 }
+
+/* migration thread support */
+
+typedef struct QEMUFileBuffered {
+    MigrationState *migration_state;
+    QEMUFile *file;
+    size_t bytes_xfer;
+    size_t xfer_limit;
+    uint8_t *buffer;
+    size_t buffer_size;
+    size_t buffer_capacity;
+    QemuThread thread;
+} QEMUFileBuffered;
+
+static ssize_t buffered_flush(QEMUFileBuffered *s)
+{
+    size_t offset = 0;
+    ssize_t ret = 0;
+
+    DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
+
+    while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
+
+        ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
+                                    s->buffer_size - offset);
+        if (ret <= 0) {
+            DPRINTF("error flushing data, %zd\n", ret);
+            break;
+        } else {
+            DPRINTF("flushed %zd byte(s)\n", ret);
+            offset += ret;
+            s->bytes_xfer += ret;
+        }
+    }
+
+    DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
+    memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
+    s->buffer_size -= offset;
+
+    if (ret < 0) {
+        return ret;
+    }
+    return offset;
+}
+
+static int buffered_put_buffer(void *opaque, const uint8_t *buf,
+                               int64_t pos, int size)
+{
+    QEMUFileBuffered *s = opaque;
+    ssize_t error;
+
+    DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
+
+    error = qemu_file_get_error(s->file);
+    if (error) {
+        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
+        return error;
+    }
+
+    if (size <= 0) {
+        return size;
+    }
+
+    if (size > (s->buffer_capacity - s->buffer_size)) {
+        DPRINTF("increasing buffer capacity from %zu by %zu\n",
+                s->buffer_capacity, size + 1024);
+
+        s->buffer_capacity += size + 1024;
+
+        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
+    }
+
+    memcpy(s->buffer + s->buffer_size, buf, size);
+    s->buffer_size += size;
+
+    return size;
+}
+
+static int buffered_close(void *opaque)
+{
+    QEMUFileBuffered *s = opaque;
+    ssize_t ret = 0;
+    int ret2;
+
+    DPRINTF("closing\n");
+
+    s->xfer_limit = INT_MAX;
+    while (!qemu_file_get_error(s->file) && s->buffer_size) {
+        ret = buffered_flush(s);
+        if (ret < 0) {
+            break;
+        }
+    }
+
+    ret2 = migrate_fd_close(s->migration_state);
+    if (ret >= 0) {
+        ret = ret2;
+    }
+    ret = migrate_fd_close(s->migration_state);
+    s->migration_state->complete = true;
+    return ret;
+}
+
+/*
+ * The meaning of the return values is:
+ *   0: We can continue sending
+ *   1: Time to stop
+ *   negative: There has been an error
+ */
+static int buffered_rate_limit(void *opaque)
+{
+    QEMUFileBuffered *s = opaque;
+    int ret;
+
+    ret = qemu_file_get_error(s->file);
+    if (ret) {
+        return ret;
+    }
+
+    if (s->bytes_xfer > s->xfer_limit) {
+        return 1;
+    }
+
+    return 0;
+}
+
+static int64_t buffered_set_rate_limit(void *opaque, int64_t new_rate)
+{
+    QEMUFileBuffered *s = opaque;
+    if (qemu_file_get_error(s->file)) {
+        goto out;
+    }
+    if (new_rate > SIZE_MAX) {
+        new_rate = SIZE_MAX;
+    }
+
+    s->xfer_limit = new_rate / 10;
+
+out:
+    return s->xfer_limit;
+}
+
+static int64_t buffered_get_rate_limit(void *opaque)
+{
+    QEMUFileBuffered *s = opaque;
+
+    return s->xfer_limit;
+}
+
+/* 100ms  xfer_limit is the limit that we should write each 100ms */
+#define BUFFER_DELAY 100
+
+static void *buffered_file_thread(void *opaque)
+{
+    QEMUFileBuffered *s = opaque;
+    int64_t initial_time = qemu_get_clock_ms(rt_clock);
+    int64_t max_size = 0;
+    bool last_round = false;
+
+    while (true) {
+        int64_t current_time = qemu_get_clock_ms(rt_clock);
+
+        if (s->migration_state->complete) {
+            break;
+        }
+        if (current_time >= initial_time + BUFFER_DELAY) {
+            uint64_t transferred_bytes = s->bytes_xfer;
+            uint64_t time_spent = current_time - initial_time;
+            double bandwidth = transferred_bytes / time_spent;
+            max_size = bandwidth * migrate_max_downtime() / 1000000;
+
+            DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
+                    " bandwidth %g max_size %" PRId64 "\n",
+                    transferred_bytes, time_spent, bandwidth, max_size);
+
+            s->bytes_xfer = 0;
+            initial_time = current_time;
+        }
+        if (!last_round && (s->bytes_xfer >= s->xfer_limit)) {
+            /* usleep expects microseconds */
+            usleep((initial_time + BUFFER_DELAY - current_time)*1000);
+        }
+        buffered_flush(s);
+
+        DPRINTF("file is ready\n");
+        if (s->bytes_xfer < s->xfer_limit) {
+            DPRINTF("notifying client\n");
+            last_round = migrate_fd_put_ready(s->migration_state, max_size);
+        }
+    }
+
+    g_free(s->buffer);
+    g_free(s);
+    return NULL;
+}
+
+void qemu_fopen_ops_buffered(MigrationState *migration_state)
+{
+    QEMUFileBuffered *s;
+
+    s = g_malloc0(sizeof(*s));
+
+    s->migration_state = migration_state;
+    s->xfer_limit = migration_state->bandwidth_limit / 10;
+    s->migration_state->complete = false;
+
+    s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
+                             buffered_close, buffered_rate_limit,
+                             buffered_set_rate_limit,
+                             buffered_get_rate_limit);
+
+    migration_state->file = s->file;
+
+    qemu_thread_create(&s->thread, buffered_file_thread, s,
+                       QEMU_THREAD_DETACHED);
+}
diff --git a/migration.h b/migration.h
index 1f2baed..40c8e9c 100644
--- a/migration.h
+++ b/migration.h
@@ -129,4 +129,5 @@ int64_t migrate_xbzrle_cache_size(void);

 int64_t xbzrle_cache_resize(int64_t new_size);

+void qemu_fopen_ops_buffered(MigrationState *migration_state);
 #endif
-- 
1.7.11.7

[Qemu-devel] [PATCH 17/30] migration: move migration_fd_put_ready()

[Juan Quintela]

Put it near its use and un-export it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 132 ++++++++++++++++++++++++++++++------------------------------
 migration.h |   1 -
 2 files changed, 66 insertions(+), 67 deletions(-)

diff --git a/migration.c b/migration.c
index b1567f3..a99653e 100644
--- a/migration.c
+++ b/migration.c
@@ -300,72 +300,6 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
     return ret;
 }

-bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
-{
-    int ret;
-    static bool first_time = true;
-    uint64_t pending_size;
-    bool last_round = false;
-
-    qemu_mutex_lock_iothread();
-    if (s->state != MIG_STATE_ACTIVE) {
-        DPRINTF("put_ready returning because of non-active state\n");
-        qemu_mutex_unlock_iothread();
-        return false;
-    }
-    if (first_time) {
-        first_time = false;
-        DPRINTF("beginning savevm\n");
-        ret = qemu_savevm_state_begin(s->file, &s->params);
-        if (ret < 0) {
-            DPRINTF("failed, %d\n", ret);
-            migrate_fd_error(s);
-            qemu_mutex_unlock_iothread();
-            return false;
-        }
-    }
-
-    DPRINTF("iterate\n");
-    pending_size = qemu_savevm_state_pending(s->file, max_size);
-    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
-    if (pending_size >= max_size) {
-        ret = qemu_savevm_state_iterate(s->file);
-        if (ret < 0) {
-            migrate_fd_error(s);
-        }
-    } else {
-        int old_vm_running = runstate_is_running();
-        int64_t start_time, end_time;
-
-        DPRINTF("done iterating\n");
-        start_time = qemu_get_clock_ms(rt_clock);
-        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
-        if (old_vm_running) {
-            vm_stop(RUN_STATE_FINISH_MIGRATE);
-        } else {
-            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-        }
-
-        if (qemu_savevm_state_complete(s->file) < 0) {
-            migrate_fd_error(s);
-        } else {
-            migrate_fd_completed(s);
-        }
-        end_time = qemu_get_clock_ms(rt_clock);
-        s->total_time = end_time - s->total_time;
-        s->downtime = end_time - start_time;
-        if (s->state != MIG_STATE_COMPLETED) {
-            if (old_vm_running) {
-                vm_start();
-            }
-        }
-        last_round = true;
-    }
-    qemu_mutex_unlock_iothread();
-
-    return last_round;
-}
-
 static void migrate_fd_cancel(MigrationState *s)
 {
     if (s->state != MIG_STATE_ACTIVE)
@@ -718,6 +652,72 @@ static int64_t buffered_get_rate_limit(void *opaque)
     return s->xfer_limit;
 }

+static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
+{
+    int ret;
+    static bool first_time = true;
+    uint64_t pending_size;
+    bool last_round = false;
+
+    qemu_mutex_lock_iothread();
+    if (s->state != MIG_STATE_ACTIVE) {
+        DPRINTF("put_ready returning because of non-active state\n");
+        qemu_mutex_unlock_iothread();
+        return false;
+    }
+    if (first_time) {
+        first_time = false;
+        DPRINTF("beginning savevm\n");
+        ret = qemu_savevm_state_begin(s->file, &s->params);
+        if (ret < 0) {
+            DPRINTF("failed, %d\n", ret);
+            migrate_fd_error(s);
+            qemu_mutex_unlock_iothread();
+            return false;
+        }
+    }
+
+    DPRINTF("iterate\n");
+    pending_size = qemu_savevm_state_pending(s->file, max_size);
+    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
+    if (pending_size >= max_size) {
+        ret = qemu_savevm_state_iterate(s->file);
+        if (ret < 0) {
+            migrate_fd_error(s);
+        }
+    } else {
+        int old_vm_running = runstate_is_running();
+        int64_t start_time, end_time;
+
+        DPRINTF("done iterating\n");
+        start_time = qemu_get_clock_ms(rt_clock);
+        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
+        if (old_vm_running) {
+            vm_stop(RUN_STATE_FINISH_MIGRATE);
+        } else {
+            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+        }
+
+        if (qemu_savevm_state_complete(s->file) < 0) {
+            migrate_fd_error(s);
+        } else {
+            migrate_fd_completed(s);
+        }
+        end_time = qemu_get_clock_ms(rt_clock);
+        s->total_time = end_time - s->total_time;
+        s->downtime = end_time - start_time;
+        if (s->state != MIG_STATE_COMPLETED) {
+            if (old_vm_running) {
+                vm_start();
+            }
+        }
+        last_round = true;
+    }
+    qemu_mutex_unlock_iothread();
+
+    return last_round;
+}
+
 /* 100ms  xfer_limit is the limit that we should write each 100ms */
 #define BUFFER_DELAY 100

diff --git a/migration.h b/migration.h
index 40c8e9c..f4c4d1e 100644
--- a/migration.h
+++ b/migration.h
@@ -81,7 +81,6 @@ void migrate_fd_connect(MigrationState *s);

 ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
                               size_t size);
-bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size);
 int migrate_fd_close(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
-- 
1.7.11.7

[Qemu-devel] [PATCH 18/30] migration: Inline qemu_fopen_ops_buffered into migrate_fd_connect

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 9 ++-------
 migration.h | 2 --
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/migration.c b/migration.c
index a99653e..1f783e1 100644
--- a/migration.c
+++ b/migration.c
@@ -345,12 +345,6 @@ bool migration_has_failed(MigrationState *s)
             s->state == MIG_STATE_ERROR);
 }

-void migrate_fd_connect(MigrationState *s)
-{
-    s->state = MIG_STATE_ACTIVE;
-    qemu_fopen_ops_buffered(s);
-}
-
 static MigrationState *migrate_init(const MigrationParams *params)
 {
     MigrationState *s = migrate_get_current();
@@ -765,10 +759,11 @@ static void *buffered_file_thread(void *opaque)
     return NULL;
 }

-void qemu_fopen_ops_buffered(MigrationState *migration_state)
+void migrate_fd_connect(MigrationState *migration_state)
 {
     QEMUFileBuffered *s;

+    migration_state->state = MIG_STATE_ACTIVE;
     s = g_malloc0(sizeof(*s));

     s->migration_state = migration_state;
diff --git a/migration.h b/migration.h
index f4c4d1e..60a2335 100644
--- a/migration.h
+++ b/migration.h
@@ -127,6 +127,4 @@ int migrate_use_xbzrle(void);
 int64_t migrate_xbzrle_cache_size(void);

 int64_t xbzrle_cache_resize(int64_t new_size);
-
-void qemu_fopen_ops_buffered(MigrationState *migration_state);
 #endif
-- 
1.7.11.7

[Qemu-devel] [PATCH 19/30] migration: move migration notifier

[Juan Quintela]

At this point, it is waranteed that state is ACTIVE.  Old position
didn't assured hat.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration.c b/migration.c
index 1f783e1..65f96b7 100644
--- a/migration.c
+++ b/migration.c
@@ -432,8 +432,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         }
         return;
     }
-
-    notifier_list_notify(&migration_state_notifiers, s);
 }

 void qmp_migrate_cancel(Error **errp)
@@ -779,4 +777,5 @@ void migrate_fd_connect(MigrationState *migration_state)

     qemu_thread_create(&s->thread, buffered_file_thread, s,
                        QEMU_THREAD_DETACHED);
+    notifier_list_notify(&migration_state_notifiers, s);
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 20/30] migration: move begining stage to the migration thread

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/migration.c b/migration.c
index 65f96b7..8cacbc3 100644
--- a/migration.c
+++ b/migration.c
@@ -647,7 +647,6 @@ static int64_t buffered_get_rate_limit(void *opaque)
 static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
 {
     int ret;
-    static bool first_time = true;
     uint64_t pending_size;
     bool last_round = false;

@@ -657,17 +656,6 @@ static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
         qemu_mutex_unlock_iothread();
         return false;
     }
-    if (first_time) {
-        first_time = false;
-        DPRINTF("beginning savevm\n");
-        ret = qemu_savevm_state_begin(s->file, &s->params);
-        if (ret < 0) {
-            DPRINTF("failed, %d\n", ret);
-            migrate_fd_error(s);
-            qemu_mutex_unlock_iothread();
-            return false;
-        }
-    }

     DPRINTF("iterate\n");
     pending_size = qemu_savevm_state_pending(s->file, max_size);
@@ -716,9 +704,21 @@ static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
 static void *buffered_file_thread(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
+    MigrationState *m = s->migration_state;
     int64_t initial_time = qemu_get_clock_ms(rt_clock);
     int64_t max_size = 0;
     bool last_round = false;
+    int ret;
+
+    qemu_mutex_lock_iothread();
+    DPRINTF("beginning savevm\n");
+    ret = qemu_savevm_state_begin(m->file, &m->params);
+    if (ret < 0) {
+        DPRINTF("failed, %d\n", ret);
+        qemu_mutex_unlock_iothread();
+        goto out;
+    }
+    qemu_mutex_unlock_iothread();

     while (true) {
         int64_t current_time = qemu_get_clock_ms(rt_clock);
@@ -748,10 +748,14 @@ static void *buffered_file_thread(void *opaque)
         DPRINTF("file is ready\n");
         if (s->bytes_xfer < s->xfer_limit) {
             DPRINTF("notifying client\n");
-            last_round = migrate_fd_put_ready(s->migration_state, max_size);
+            last_round = migrate_fd_put_ready(m, max_size);
         }
     }

+out:
+    if (ret < 0) {
+        migrate_fd_error(m);
+    }
     g_free(s->buffer);
     g_free(s);
     return NULL;
-- 
1.7.11.7

[Qemu-devel] [PATCH 21/30] migration: move exit condition to migration thread

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/migration.c b/migration.c
index 8cacbc3..7206866 100644
--- a/migration.c
+++ b/migration.c
@@ -651,12 +651,6 @@ static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
     bool last_round = false;

     qemu_mutex_lock_iothread();
-    if (s->state != MIG_STATE_ACTIVE) {
-        DPRINTF("put_ready returning because of non-active state\n");
-        qemu_mutex_unlock_iothread();
-        return false;
-    }
-
     DPRINTF("iterate\n");
     pending_size = qemu_savevm_state_pending(s->file, max_size);
     DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
@@ -723,9 +717,18 @@ static void *buffered_file_thread(void *opaque)
     while (true) {
         int64_t current_time = qemu_get_clock_ms(rt_clock);

-        if (s->migration_state->complete) {
+        qemu_mutex_lock_iothread();
+        if (m->state != MIG_STATE_ACTIVE) {
+            DPRINTF("put_ready returning because of non-active state\n");
+            qemu_mutex_unlock_iothread();
             break;
         }
+        if (m->complete) {
+            qemu_mutex_unlock_iothread();
+            break;
+        }
+        qemu_mutex_unlock_iothread();
+
         if (current_time >= initial_time + BUFFER_DELAY) {
             uint64_t transferred_bytes = s->bytes_xfer;
             uint64_t time_spent = current_time - initial_time;
-- 
1.7.11.7

[Qemu-devel] [PATCH 22/30] migration: unfold rest of migrate_fd_put_ready() into thread

[Juan Quintela]

This will allow us finer control in next patches.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 95 ++++++++++++++++++++++++++-----------------------------------
 1 file changed, 41 insertions(+), 54 deletions(-)

diff --git a/migration.c b/migration.c
index 7206866..e6ff1f1 100644
--- a/migration.c
+++ b/migration.c
@@ -644,54 +644,6 @@ static int64_t buffered_get_rate_limit(void *opaque)
     return s->xfer_limit;
 }

-static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
-{
-    int ret;
-    uint64_t pending_size;
-    bool last_round = false;
-
-    qemu_mutex_lock_iothread();
-    DPRINTF("iterate\n");
-    pending_size = qemu_savevm_state_pending(s->file, max_size);
-    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
-    if (pending_size >= max_size) {
-        ret = qemu_savevm_state_iterate(s->file);
-        if (ret < 0) {
-            migrate_fd_error(s);
-        }
-    } else {
-        int old_vm_running = runstate_is_running();
-        int64_t start_time, end_time;
-
-        DPRINTF("done iterating\n");
-        start_time = qemu_get_clock_ms(rt_clock);
-        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
-        if (old_vm_running) {
-            vm_stop(RUN_STATE_FINISH_MIGRATE);
-        } else {
-            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-        }
-
-        if (qemu_savevm_state_complete(s->file) < 0) {
-            migrate_fd_error(s);
-        } else {
-            migrate_fd_completed(s);
-        }
-        end_time = qemu_get_clock_ms(rt_clock);
-        s->total_time = end_time - s->total_time;
-        s->downtime = end_time - start_time;
-        if (s->state != MIG_STATE_COMPLETED) {
-            if (old_vm_running) {
-                vm_start();
-            }
-        }
-        last_round = true;
-    }
-    qemu_mutex_unlock_iothread();
-
-    return last_round;
-}
-
 /* 100ms  xfer_limit is the limit that we should write each 100ms */
 #define BUFFER_DELAY 100

@@ -716,6 +668,7 @@ static void *buffered_file_thread(void *opaque)

     while (true) {
         int64_t current_time = qemu_get_clock_ms(rt_clock);
+        uint64_t pending_size;

         qemu_mutex_lock_iothread();
         if (m->state != MIG_STATE_ACTIVE) {
@@ -727,6 +680,46 @@ static void *buffered_file_thread(void *opaque)
             qemu_mutex_unlock_iothread();
             break;
         }
+        if (s->bytes_xfer < s->xfer_limit) {
+            DPRINTF("iterate\n");
+            pending_size = qemu_savevm_state_pending(m->file, max_size);
+            DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
+            if (pending_size >= max_size) {
+                ret = qemu_savevm_state_iterate(m->file);
+                if (ret < 0) {
+                    qemu_mutex_unlock_iothread();
+                    break;
+                }
+            } else {
+                int old_vm_running = runstate_is_running();
+                int64_t start_time, end_time;
+
+                DPRINTF("done iterating\n");
+                start_time = qemu_get_clock_ms(rt_clock);
+                qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
+                if (old_vm_running) {
+                    vm_stop(RUN_STATE_FINISH_MIGRATE);
+                } else {
+                    vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+                }
+                ret = qemu_savevm_state_complete(m->file);
+                if (ret < 0) {
+                    qemu_mutex_unlock_iothread();
+                    break;
+                } else {
+                    migrate_fd_completed(m);
+                }
+                end_time = qemu_get_clock_ms(rt_clock);
+                m->total_time = end_time - m->total_time;
+                m->downtime = end_time - start_time;
+                if (m->state != MIG_STATE_COMPLETED) {
+                    if (old_vm_running) {
+                        vm_start();
+                    }
+                }
+                last_round = true;
+            }
+        }
         qemu_mutex_unlock_iothread();

         if (current_time >= initial_time + BUFFER_DELAY) {
@@ -747,12 +740,6 @@ static void *buffered_file_thread(void *opaque)
             usleep((initial_time + BUFFER_DELAY - current_time)*1000);
         }
         buffered_flush(s);
-
-        DPRINTF("file is ready\n");
-        if (s->bytes_xfer < s->xfer_limit) {
-            DPRINTF("notifying client\n");
-            last_round = migrate_fd_put_ready(m, max_size);
-        }
     }

 out:
-- 
1.7.11.7

[Qemu-devel] [PATCH 23/30] migration: print times for end phase

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 block.c     |  6 ++++++
 cpus.c      | 17 +++++++++++++++++
 migration.c |  9 +++++++++
 savevm.c    | 13 +++++++++++++
 4 files changed, 45 insertions(+)

diff --git a/block.c b/block.c
index e95f613..9cd6ffe 100644
--- a/block.c
+++ b/block.c
@@ -2648,9 +2648,15 @@ int bdrv_get_flags(BlockDriverState *bs)
 void bdrv_flush_all(void)
 {
     BlockDriverState *bs;
+    int64_t start_time, end_time;
+
+    start_time = qemu_get_clock_ms(rt_clock);

     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         bdrv_flush(bs);
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time flush device %s: %ld\n", bs->filename,
+               end_time - start_time);
     }
 }

diff --git a/cpus.c b/cpus.c
index 191cbf5..63e51f1 100644
--- a/cpus.c
+++ b/cpus.c
@@ -435,14 +435,31 @@ int cpu_is_stopped(CPUArchState *env)

 static void do_vm_stop(RunState state)
 {
+    int64_t start_time, end_time;
+
     if (runstate_is_running()) {
+        start_time = qemu_get_clock_ms(rt_clock);
         cpu_disable_ticks();
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time cpu_disable_ticks %ld\n", end_time - start_time);
         pause_all_vcpus();
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time pause_all_vcpus %ld\n", end_time - start_time);
         runstate_set(state);
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time runstate_set %ld\n", end_time - start_time);
         vm_state_notify(0, state);
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time vmstate_notify %ld\n", end_time - start_time);
         bdrv_drain_all();
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time bdrv_drain_all %ld\n", end_time - start_time);
         bdrv_flush_all();
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time bdrv_flush_all %ld\n", end_time - start_time);
         monitor_protocol_event(QEVENT_STOP, NULL);
+        end_time = qemu_get_clock_ms(rt_clock);
+        printf("time monitor_protocol_event %ld\n", end_time - start_time);
     }
 }

diff --git a/migration.c b/migration.c
index e6ff1f1..3856a99 100644
--- a/migration.c
+++ b/migration.c
@@ -697,19 +697,26 @@ static void *buffered_file_thread(void *opaque)
                 DPRINTF("done iterating\n");
                 start_time = qemu_get_clock_ms(rt_clock);
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
+                end_time = qemu_get_clock_ms(rt_clock);
+                printf("wakeup_request %ld\n", end_time - start_time);
                 if (old_vm_running) {
                     vm_stop(RUN_STATE_FINISH_MIGRATE);
                 } else {
                     vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
                 }
+                end_time = qemu_get_clock_ms(rt_clock);
+                printf("vm_stop 2 %ld\n", end_time - start_time);
                 ret = qemu_savevm_state_complete(m->file);
                 if (ret < 0) {
                     qemu_mutex_unlock_iothread();
                     break;
                 } else {
+                    end_time = qemu_get_clock_ms(rt_clock);
+                    printf("complete without error 3a %ld\n", end_time - start_time);
                     migrate_fd_completed(m);
                 }
                 end_time = qemu_get_clock_ms(rt_clock);
+                printf("completed %ld\n", end_time - start_time);
                 m->total_time = end_time - m->total_time;
                 m->downtime = end_time - start_time;
                 if (m->state != MIG_STATE_COMPLETED) {
@@ -717,6 +724,8 @@ static void *buffered_file_thread(void *opaque)
                         vm_start();
                     }
                 }
+                end_time = qemu_get_clock_ms(rt_clock);
+                printf("end completed stage %ld\n", end_time - start_time);
                 last_round = true;
             }
         }
diff --git a/savevm.c b/savevm.c
index a68c37b..3ebde5c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1629,9 +1629,14 @@ int qemu_savevm_state_complete(QEMUFile *f)
 {
     SaveStateEntry *se;
     int ret;
+    int64_t t1;
+    int64_t t0 = qemu_get_clock_ms(rt_clock);

     cpu_synchronize_all_states();
+    t1 = qemu_get_clock_ms(rt_clock);
+    printf("synchronize_all_states %ld\n", t1 - t0);

+    t0 = t1;
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         if (!se->ops || !se->ops->save_live_complete) {
             continue;
@@ -1652,6 +1657,11 @@ int qemu_savevm_state_complete(QEMUFile *f)
             return ret;
         }
     }
+    t1 = qemu_get_clock_ms(rt_clock);
+
+    printf("migrate RAM %ld\n", t1 - t0);
+
+    t0 = t1;

     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         int len;
@@ -1676,6 +1686,9 @@ int qemu_savevm_state_complete(QEMUFile *f)
         trace_savevm_section_end(se->section_id);
     }

+    t1 = qemu_get_clock_ms(rt_clock);
+
+    printf("migrate rest devices %ld\n", t1 - t0);
     qemu_put_byte(f, QEMU_VM_EOF);

     return qemu_file_get_error(f);
-- 
1.7.11.7

[Qemu-devel] [PATCH 24/30] ram: rename last_block to last_seen_block

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 1eefef8..8ac4c94 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -332,7 +332,10 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
     return bytes_sent;
 }

-static RAMBlock *last_block;
+
+/* This is the last block that we have visited serching for dirty pages
+ */
+static RAMBlock *last_seen_block;
 static ram_addr_t last_offset;
 static unsigned long *migration_bitmap;
 static uint64_t migration_dirty_pages;
@@ -417,7 +420,7 @@ static void migration_bitmap_sync(void)

 static int ram_save_block(QEMUFile *f, bool last_stage)
 {
-    RAMBlock *block = last_block;
+    RAMBlock *block = last_seen_block;
     ram_addr_t offset = last_offset;
     int bytes_sent = -1;
     MemoryRegion *mr;
@@ -430,7 +433,8 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
         mr = block->mr;
         if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
             uint8_t *p;
-            int cont = (block == last_block) ? RAM_SAVE_FLAG_CONTINUE : 0;
+            int cont = (block == last_seen_block) ?
+                RAM_SAVE_FLAG_CONTINUE : 0;

             p = memory_region_get_ram_ptr(mr) + offset;

@@ -469,9 +473,9 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
             if (!block)
                 block = QLIST_FIRST(&ram_list.blocks);
         }
-    } while (block != last_block || offset != last_offset);
+    } while (block != last_seen_block || offset != last_offset);

-    last_block = block;
+    last_seen_block = block;
     last_offset = offset;

     return bytes_sent;
@@ -555,7 +559,7 @@ static void ram_migration_cancel(void *opaque)

 static void reset_ram_globals(void)
 {
-    last_block = NULL;
+    last_seen_block = NULL;
     last_offset = 0;
     last_version = ram_list.version;
     sort_ram_list();
-- 
1.7.11.7

[Qemu-devel] [PATCH 25/30] ram: Add last_sent_block

[Juan Quintela]

This is the last block from where we have sent data.

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index 8ac4c94..6f39ebd 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -336,6 +336,8 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
 /* This is the last block that we have visited serching for dirty pages
  */
 static RAMBlock *last_seen_block;
+/* This is the last block from where we have sent data */
+static RAMBlock *last_sent_block;
 static ram_addr_t last_offset;
 static unsigned long *migration_bitmap;
 static uint64_t migration_dirty_pages;
@@ -433,7 +435,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
         mr = block->mr;
         if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
             uint8_t *p;
-            int cont = (block == last_seen_block) ?
+            int cont = (block == last_sent_block) ?
                 RAM_SAVE_FLAG_CONTINUE : 0;

             p = memory_region_get_ram_ptr(mr) + offset;
@@ -462,6 +464,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)

             /* if page is unmodified, continue to the next */
             if (bytes_sent != 0) {
+                last_sent_block = block;
                 break;
             }
         }
@@ -560,6 +563,7 @@ static void ram_migration_cancel(void *opaque)
 static void reset_ram_globals(void)
 {
     last_seen_block = NULL;
+    last_sent_block = NULL;
     last_offset = 0;
     last_version = ram_list.version;
     sort_ram_list();
-- 
1.7.11.7

[Qemu-devel] [PATCH 26/30] memory: introduce memory_region_test_and_clear_dirty

[Juan Quintela]

This function avoids having to do two calls, one to test the dirty bit, and
other to reset it.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 memory.c | 15 +++++++++++++++
 memory.h | 17 +++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/memory.c b/memory.c
index 4f3ade0..126fb63 100644
--- a/memory.c
+++ b/memory.c
@@ -1080,6 +1080,21 @@ void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
     return cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size, -1);
 }

+bool memory_region_test_and_clear_dirty(MemoryRegion *mr,
+                                        target_phys_addr_t addr,
+                                        target_phys_addr_t size, unsigned client)
+{
+    bool ret;
+    assert(mr->terminates);
+    ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size,
+                                        1 << client);
+    if (ret) {
+        cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size, -1);
+    }
+    return ret;
+}
+
+
 void memory_region_sync_dirty_bitmap(MemoryRegion *mr)
 {
     FlatRange *fr;
diff --git a/memory.h b/memory.h
index 37ce151..08af012 100644
--- a/memory.h
+++ b/memory.h
@@ -434,6 +434,23 @@ void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
                              target_phys_addr_t size);

 /**
+ * memory_region_test_and_clear_dirty: Check whether a range of bytes is dirty
+ *                                     for a specified client. It clears them.
+ *
+ * Checks whether a range of bytes has been written to since the last
+ * call to memory_region_reset_dirty() with the same @client.  Dirty logging
+ * must be enabled.
+ *
+ * @mr: the memory region being queried.
+ * @addr: the address (relative to the start of the region) being queried.
+ * @size: the size of the range being queried.
+ * @client: the user of the logging information; %DIRTY_MEMORY_MIGRATION or
+ *          %DIRTY_MEMORY_VGA.
+ */
+bool memory_region_test_and_clear_dirty(MemoryRegion *mr,
+                                        target_phys_addr_t addr,
+                                        target_phys_addr_t size, unsigned client)
+/**
  * memory_region_sync_dirty_bitmap: Synchronize a region's dirty bitmap with
  *                                  any external TLBs (e.g. kvm)
  *
-- 
1.7.11.7

[Qemu-devel] [PATCH 27/30] ram: Use memory_region_test_and_clear_dirty

[Juan Quintela]

This avoids having to do two walks over the dirty bitmap, once reading
the dirty bits, and anthoer cleaning them.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 6f39ebd..8391375 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -390,13 +390,12 @@ static void migration_bitmap_sync(void)

     QLIST_FOREACH(block, &ram_list.blocks, next) {
         for (addr = 0; addr < block->length; addr += TARGET_PAGE_SIZE) {
-            if (memory_region_get_dirty(block->mr, addr, TARGET_PAGE_SIZE,
-                                        DIRTY_MEMORY_MIGRATION)) {
+            if (memory_region_test_and_clear_dirty(block->mr,
+                                                   addr, TARGET_PAGE_SIZE,
+                                                   DIRTY_MEMORY_MIGRATION)) {
                 migration_bitmap_set_dirty(block->mr, addr);
             }
         }
-        memory_region_reset_dirty(block->mr, 0, block->length,
-                                  DIRTY_MEMORY_MIGRATION);
     }
     trace_migration_bitmap_sync_end(migration_dirty_pages
                                     - num_dirty_pages_init);
-- 
1.7.11.7

[Qemu-devel] [PATCH 28/30] fix memory.c

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 memory.c | 4 +++-
 memory.h | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 126fb63..4d0fa96 100644
--- a/memory.c
+++ b/memory.c
@@ -1089,7 +1089,9 @@ bool memory_region_test_and_clear_dirty(MemoryRegion *mr,
     ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size,
                                         1 << client);
     if (ret) {
-        cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size, -1);
+        cpu_physical_memory_reset_dirty(mr->ram_addr + addr,
+                                        mr->ram_addr + addr + size,
+                                        1 << client);
     }
     return ret;
 }
diff --git a/memory.h b/memory.h
index 08af012..0dcc0f4 100644
--- a/memory.h
+++ b/memory.h
@@ -449,7 +449,8 @@ void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
  */
 bool memory_region_test_and_clear_dirty(MemoryRegion *mr,
                                         target_phys_addr_t addr,
-                                        target_phys_addr_t size, unsigned client)
+                                        target_phys_addr_t size,
+                                        unsigned client);
 /**
  * memory_region_sync_dirty_bitmap: Synchronize a region's dirty bitmap with
  *                                  any external TLBs (e.g. kvm)
-- 
1.7.11.7

[Qemu-devel] [PATCH 29/30] migration: Only go to the iterate stage if there is anything to send

[Juan Quintela]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration.c b/migration.c
index 3856a99..554d79a 100644
--- a/migration.c
+++ b/migration.c
@@ -684,7 +684,7 @@ static void *buffered_file_thread(void *opaque)
             DPRINTF("iterate\n");
             pending_size = qemu_savevm_state_pending(m->file, max_size);
             DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
-            if (pending_size >= max_size) {
+            if (pending_size && pending_size >= max_size) {
                 ret = qemu_savevm_state_iterate(m->file);
                 if (ret < 0) {
                     qemu_mutex_unlock_iothread();
-- 
1.7.11.7

[Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Juan Quintela]

Instead of testing each page individually, we search what is the next
dirty page with a bitmap operation.  We have to reorganize the code to
move from a "for" loop, to a while(dirty) loop.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 8391375..cd6ebef 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -343,18 +343,21 @@ static unsigned long *migration_bitmap;
 static uint64_t migration_dirty_pages;
 static uint32_t last_version;

-static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
-                                                         ram_addr_t offset)
+static inline
+ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
+                                                 ram_addr_t start)
 {
-    bool ret;
-    int nr = (mr->ram_addr + offset) >> TARGET_PAGE_BITS;
+    unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
+    unsigned long nr = base + (start >> TARGET_PAGE_BITS);
+    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);

-    ret = test_and_clear_bit(nr, migration_bitmap);
+    unsigned long next = find_next_bit(migration_bitmap, size, nr);

-    if (ret) {
+    if (next < size) {
+        clear_bit(next, migration_bitmap);
         migration_dirty_pages--;
     }
-    return ret;
+    return (next - base) << TARGET_PAGE_BITS;
 }

 static inline bool migration_bitmap_set_dirty(MemoryRegion *mr,
@@ -423,6 +426,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
 {
     RAMBlock *block = last_seen_block;
     ram_addr_t offset = last_offset;
+    bool complete_round = false;
     int bytes_sent = -1;
     MemoryRegion *mr;
     ram_addr_t current_addr;
@@ -430,9 +434,21 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
     if (!block)
         block = QLIST_FIRST(&ram_list.blocks);

-    do {
+    while(true) {
         mr = block->mr;
-        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
+        offset = migration_bitmap_find_and_reset_dirty(mr, offset);
+        if (complete_round && block == last_seen_block &&
+            offset >= last_offset) {
+            break;
+        }
+        if (offset >= block->length) {
+            offset = 0;
+            block = QLIST_NEXT(block, next);
+            if (!block) {
+                block = QLIST_FIRST(&ram_list.blocks);
+                complete_round = true;
+            }
+        } else {
             uint8_t *p;
             int cont = (block == last_sent_block) ?
                 RAM_SAVE_FLAG_CONTINUE : 0;
@@ -467,16 +483,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
                 break;
             }
         }
-
-        offset += TARGET_PAGE_SIZE;
-        if (offset >= block->length) {
-            offset = 0;
-            block = QLIST_NEXT(block, next);
-            if (!block)
-                block = QLIST_FIRST(&ram_list.blocks);
-        }
-    } while (block != last_seen_block || offset != last_offset);
-
+    }
     last_seen_block = block;
     last_offset = offset;

-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 21/30] migration: move exit condition to migration thread

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> -        if (s->migration_state->complete) {
> +        qemu_mutex_lock_iothread();

So, was it a bug that we were accessing ->complete without the BQL?

> +        if (m->state != MIG_STATE_ACTIVE) {
> +            DPRINTF("put_ready returning because of non-active state\n");

The contents of the debug message obsolete.  Besides, I would just put
the two branches in the same "if (m->state != MIG_STATE_ACTIVE ||
m->complete)".

> +            qemu_mutex_unlock_iothread();
>              break;
>          }
> +        if (m->complete) {
> +            qemu_mutex_unlock_iothread();
> +            break;
> +        }
> +        qemu_mutex_unlock_iothread();
> +

Paolo

Re: [Qemu-devel] [PATCH 22/30] migration: unfold rest of migrate_fd_put_ready() into thread

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> This will allow us finer control in next patches.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c | 95 ++++++++++++++++++++++++++-----------------------------------
>  1 file changed, 41 insertions(+), 54 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 7206866..e6ff1f1 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -644,54 +644,6 @@ static int64_t buffered_get_rate_limit(void *opaque)
>      return s->xfer_limit;
>  }
> 
> -static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
> -{
> -    int ret;
> -    uint64_t pending_size;
> -    bool last_round = false;
> -
> -    qemu_mutex_lock_iothread();
> -    DPRINTF("iterate\n");
> -    pending_size = qemu_savevm_state_pending(s->file, max_size);
> -    DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> -    if (pending_size >= max_size) {
> -        ret = qemu_savevm_state_iterate(s->file);
> -        if (ret < 0) {
> -            migrate_fd_error(s);
> -        }
> -    } else {
> -        int old_vm_running = runstate_is_running();
> -        int64_t start_time, end_time;
> -
> -        DPRINTF("done iterating\n");
> -        start_time = qemu_get_clock_ms(rt_clock);
> -        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> -        if (old_vm_running) {
> -            vm_stop(RUN_STATE_FINISH_MIGRATE);
> -        } else {
> -            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> -        }
> -
> -        if (qemu_savevm_state_complete(s->file) < 0) {
> -            migrate_fd_error(s);
> -        } else {
> -            migrate_fd_completed(s);
> -        }
> -        end_time = qemu_get_clock_ms(rt_clock);
> -        s->total_time = end_time - s->total_time;
> -        s->downtime = end_time - start_time;
> -        if (s->state != MIG_STATE_COMPLETED) {
> -            if (old_vm_running) {
> -                vm_start();
> -            }
> -        }
> -        last_round = true;
> -    }
> -    qemu_mutex_unlock_iothread();
> -
> -    return last_round;
> -}
> -
>  /* 100ms  xfer_limit is the limit that we should write each 100ms */
>  #define BUFFER_DELAY 100
> 
> @@ -716,6 +668,7 @@ static void *buffered_file_thread(void *opaque)
> 
>      while (true) {
>          int64_t current_time = qemu_get_clock_ms(rt_clock);
> +        uint64_t pending_size;
> 
>          qemu_mutex_lock_iothread();
>          if (m->state != MIG_STATE_ACTIVE) {
> @@ -727,6 +680,46 @@ static void *buffered_file_thread(void *opaque)
>              qemu_mutex_unlock_iothread();
>              break;
>          }
> +        if (s->bytes_xfer < s->xfer_limit) {
> +            DPRINTF("iterate\n");
> +            pending_size = qemu_savevm_state_pending(m->file, max_size);
> +            DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> +            if (pending_size >= max_size) {
> +                ret = qemu_savevm_state_iterate(m->file);

So RAM migration is still being run inside the BQL, isn't it?

> +                if (ret < 0) {
> +                    qemu_mutex_unlock_iothread();
> +                    break;

There's a lot of

                    qemu_mutex_unlock_iothread();
                    break;

in this function.  Perhaps it is better if you make an invariant that
the loop is entered and exited with the BQL taken, and it is only
unlocked in the middle.  It makes sense once you fold everything in
migration.c.

It can be a separate patch though.

> +                }
> +            } else {
> +                int old_vm_running = runstate_is_running();
> +                int64_t start_time, end_time;
> +
> +                DPRINTF("done iterating\n");
> +                start_time = qemu_get_clock_ms(rt_clock);
> +                qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> +                if (old_vm_running) {
> +                    vm_stop(RUN_STATE_FINISH_MIGRATE);
> +                } else {
> +                    vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +                }
> +                ret = qemu_savevm_state_complete(m->file);
> +                if (ret < 0) {
> +                    qemu_mutex_unlock_iothread();
> +                    break;
> +                } else {
> +                    migrate_fd_completed(m);
> +                }
> +                end_time = qemu_get_clock_ms(rt_clock);
> +                m->total_time = end_time - m->total_time;
> +                m->downtime = end_time - start_time;
> +                if (m->state != MIG_STATE_COMPLETED) {
> +                    if (old_vm_running) {
> +                        vm_start();
> +                    }
> +                }
> +                last_round = true;
> +            }
> +        }
>          qemu_mutex_unlock_iothread();
> 
>          if (current_time >= initial_time + BUFFER_DELAY) {
> @@ -747,12 +740,6 @@ static void *buffered_file_thread(void *opaque)
>              usleep((initial_time + BUFFER_DELAY - current_time)*1000);
>          }
>          buffered_flush(s);
> -
> -        DPRINTF("file is ready\n");
> -        if (s->bytes_xfer < s->xfer_limit) {
> -            DPRINTF("notifying client\n");
> -            last_round = migrate_fd_put_ready(m, max_size);
> -        }
>      }
> 
>  out:
> 

Re: [Qemu-devel] [PATCH 28/30] fix memory.c

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  memory.c | 4 +++-
>  memory.h | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 126fb63..4d0fa96 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1089,7 +1089,9 @@ bool memory_region_test_and_clear_dirty(MemoryRegion *mr,
>      ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size,
>                                          1 << client);
>      if (ret) {
> -        cpu_physical_memory_set_dirty_range(mr->ram_addr + addr, size, -1);
> +        cpu_physical_memory_reset_dirty(mr->ram_addr + addr,
> +                                        mr->ram_addr + addr + size,
> +                                        1 << client);
>      }
>      return ret;
>  }
> diff --git a/memory.h b/memory.h
> index 08af012..0dcc0f4 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -449,7 +449,8 @@ void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr,
>   */
>  bool memory_region_test_and_clear_dirty(MemoryRegion *mr,
>                                          target_phys_addr_t addr,
> -                                        target_phys_addr_t size, unsigned client)
> +                                        target_phys_addr_t size,
> +                                        unsigned client);
>  /**
>   * memory_region_sync_dirty_bitmap: Synchronize a region's dirty bitmap with
>   *                                  any external TLBs (e.g. kvm)
> 

This should be squashed in patch 26.

Paolo

Re: [Qemu-devel] [PATCH 22/30] migration: unfold rest of migrate_fd_put_ready() into thread

[Paolo Bonzini]

Il 18/10/2012 10:39, Paolo Bonzini ha scritto:
> in this function.  Perhaps it is better if you make an invariant that
> the loop is entered and exited with the BQL taken, and it is only
> unlocked in the middle.  It makes sense once you fold everything in
> migration.c.

i.e. this:

diff --git a/migration.c b/migration.c
index 554d79a..bd9fe2d 100644
--- a/migration.c
+++ b/migration.c
@@ -661,23 +661,18 @@ static void *buffered_file_thread(void *opaque)
     ret = qemu_savevm_state_begin(m->file, &m->params);
     if (ret < 0) {
         DPRINTF("failed, %d\n", ret);
-        qemu_mutex_unlock_iothread();
         goto out;
     }
-    qemu_mutex_unlock_iothread();
 
     while (true) {
         int64_t current_time = qemu_get_clock_ms(rt_clock);
         uint64_t pending_size;
 
-        qemu_mutex_lock_iothread();
         if (m->state != MIG_STATE_ACTIVE) {
             DPRINTF("put_ready returning because of non-active state\n");
-            qemu_mutex_unlock_iothread();
             break;
         }
         if (m->complete) {
-            qemu_mutex_unlock_iothread();
             break;
         }
         if (s->bytes_xfer < s->xfer_limit) {
@@ -687,7 +682,6 @@ static void *buffered_file_thread(void *opaque)
             if (pending_size && pending_size >= max_size) {
                 ret = qemu_savevm_state_iterate(m->file);
                 if (ret < 0) {
-                    qemu_mutex_unlock_iothread();
                     break;
                 }
             } else {
@@ -708,7 +702,6 @@ static void *buffered_file_thread(void *opaque)
                 printf("vm_stop 2 %ld\n", end_time - start_time);
                 ret = qemu_savevm_state_complete(m->file);
                 if (ret < 0) {
-                    qemu_mutex_unlock_iothread();
                     break;
                 } else {
                     end_time = qemu_get_clock_ms(rt_clock);
@@ -729,8 +722,8 @@ static void *buffered_file_thread(void *opaque)
                 last_round = true;
             }
         }
-        qemu_mutex_unlock_iothread();
 
+        qemu_mutex_unlock_iothread();
         if (current_time >= initial_time + BUFFER_DELAY) {
             uint64_t transferred_bytes = s->bytes_xfer;
             uint64_t time_spent = current_time - initial_time;
@@ -749,9 +742,11 @@ static void *buffered_file_thread(void *opaque)
             usleep((initial_time + BUFFER_DELAY - current_time)*1000);
         }
         buffered_flush(s);
+        qemu_mutex_lock_iothread();
     }
 
 out:
+    qemu_mutex_unlock_iothread();
     if (ret < 0) {
         migrate_fd_error(m);
     }


BTW the completion (basically the "else" starting at "int old_vm_running =
runstate_is_running()" looks much nicer in a separate function, like:

            if (pending_size && pending_size >= max_size) {
                ret = qemu_savevm_state_iterate(m->file);
            } else {
                ret = migration_complete(m);
                last_round = true;
            }
            if (ret < 0) {
                break;
            }

Paolo

Re: [Qemu-devel] [PATCH 04/30] buffered_file: Move from using a timer to use a thread

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> +        }
> +        if (s->bytes_xfer >= s->xfer_limit) {
> +            /* usleep expects microseconds */
> +            usleep((expire_time - current_time)*1000);
> +        }

g_usleep please.

Paolo

Re: [Qemu-devel] [PATCH 16/30] migration: move buffered_file.c code into migration.c

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> This only moves the code (also from buffered_file.h to migration.h).
> Fix whitespace until checkpatch is happy.

While I agree with this patch, it is also a conflict magnet.  My
migration-in-a-coroutine cleanups will touch buffered_file.c, you're
warned...

Paolo

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  Makefile.objs   |   2 +-
>  buffered_file.c | 244 --------------------------------------------------------
>  buffered_file.h |  22 -----
>  migration.c     | 218 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  migration.h     |   1 +
>  5 files changed, 219 insertions(+), 268 deletions(-)
>  delete mode 100644 buffered_file.c
>  delete mode 100644 buffered_file.h
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 74b3542..3de8f27 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -73,7 +73,7 @@ extra-obj-$(CONFIG_LINUX) += fsdev/
> 
>  common-obj-y += tcg-runtime.o host-utils.o main-loop.o
>  common-obj-y += input.o
> -common-obj-y += buffered_file.o migration.o migration-tcp.o
> +common-obj-y += migration.o migration-tcp.o
>  common-obj-y += qemu-char.o #aio.o
>  common-obj-y += block-migration.o iohandler.o
>  common-obj-y += pflib.o
> diff --git a/buffered_file.c b/buffered_file.c
> deleted file mode 100644
> index c21f847..0000000
> --- a/buffered_file.c
> +++ /dev/null
> @@ -1,244 +0,0 @@
> -/*
> - * QEMU buffered QEMUFile
> - *
> - * Copyright IBM, Corp. 2008
> - *
> - * Authors:
> - *  Anthony Liguori   <aliguori@us.ibm.com>
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2.  See
> - * the COPYING file in the top-level directory.
> - *
> - * Contributions after 2012-01-13 are licensed under the terms of the
> - * GNU GPL, version 2 or (at your option) any later version.
> - */
> -
> -#include "qemu-common.h"
> -#include "hw/hw.h"
> -#include "qemu-timer.h"
> -#include "qemu-char.h"
> -#include "buffered_file.h"
> -#include "qemu-thread.h"
> -
> -//#define DEBUG_BUFFERED_FILE
> -
> -typedef struct QEMUFileBuffered
> -{
> -    MigrationState *migration_state;
> -    QEMUFile *file;
> -    size_t bytes_xfer;
> -    size_t xfer_limit;
> -    uint8_t *buffer;
> -    size_t buffer_size;
> -    size_t buffer_capacity;
> -    QemuThread thread;
> -} QEMUFileBuffered;
> -
> -#ifdef DEBUG_BUFFERED_FILE
> -#define DPRINTF(fmt, ...) \
> -    do { printf("buffered-file: " fmt, ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) \
> -    do { } while (0)
> -#endif
> -
> -static ssize_t buffered_flush(QEMUFileBuffered *s)
> -{
> -    size_t offset = 0;
> -    ssize_t ret = 0;
> -
> -    DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
> -
> -    while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
> -
> -        ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
> -                                    s->buffer_size - offset);
> -        if (ret <= 0) {
> -            DPRINTF("error flushing data, %zd\n", ret);
> -            break;
> -        } else {
> -            DPRINTF("flushed %zd byte(s)\n", ret);
> -            offset += ret;
> -            s->bytes_xfer += ret;
> -        }
> -    }
> -
> -    DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
> -    memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
> -    s->buffer_size -= offset;
> -
> -    if (ret < 0) {
> -        return ret;
> -    }
> -    return offset;
> -}
> -
> -static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size)
> -{
> -    QEMUFileBuffered *s = opaque;
> -    ssize_t error;
> -
> -    DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
> -
> -    error = qemu_file_get_error(s->file);
> -    if (error) {
> -        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
> -        return error;
> -    }
> -
> -    if (size <= 0) {
> -        return size;
> -    }
> -
> -    if (size > (s->buffer_capacity - s->buffer_size)) {
> -        DPRINTF("increasing buffer capacity from %zu by %zu\n",
> -                s->buffer_capacity, size + 1024);
> -
> -        s->buffer_capacity += size + 1024;
> -
> -        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
> -    }
> -
> -    memcpy(s->buffer + s->buffer_size, buf, size);
> -    s->buffer_size += size;
> -
> -    return size;
> -}
> -
> -static int buffered_close(void *opaque)
> -{
> -    QEMUFileBuffered *s = opaque;
> -    ssize_t ret = 0;
> -    int ret2;
> -
> -    DPRINTF("closing\n");
> -
> -    s->xfer_limit = INT_MAX;
> -    while (!qemu_file_get_error(s->file) && s->buffer_size) {
> -        ret = buffered_flush(s);
> -        if (ret < 0) {
> -            break;
> -        }
> -    }
> -
> -    ret2 = migrate_fd_close(s->migration_state);
> -    if (ret >= 0) {
> -        ret = ret2;
> -    }
> -    ret = migrate_fd_close(s->migration_state);
> -    s->migration_state->complete = true;
> -    return ret;
> -}
> -
> -/*
> - * The meaning of the return values is:
> - *   0: We can continue sending
> - *   1: Time to stop
> - *   negative: There has been an error
> - */
> -static int buffered_rate_limit(void *opaque)
> -{
> -    QEMUFileBuffered *s = opaque;
> -    int ret;
> -
> -    ret = qemu_file_get_error(s->file);
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    if (s->bytes_xfer > s->xfer_limit)
> -        return 1;
> -
> -    return 0;
> -}
> -
> -static int64_t buffered_set_rate_limit(void *opaque, int64_t new_rate)
> -{
> -    QEMUFileBuffered *s = opaque;
> -    if (qemu_file_get_error(s->file)) {
> -        goto out;
> -    }
> -    if (new_rate > SIZE_MAX) {
> -        new_rate = SIZE_MAX;
> -    }
> -
> -    s->xfer_limit = new_rate / 10;
> -    
> -out:
> -    return s->xfer_limit;
> -}
> -
> -static int64_t buffered_get_rate_limit(void *opaque)
> -{
> -    QEMUFileBuffered *s = opaque;
> -  
> -    return s->xfer_limit;
> -}
> -
> -/* 100ms  xfer_limit is the limit that we should write each 100ms */
> -#define BUFFER_DELAY 100
> -
> -static void *buffered_file_thread(void *opaque)
> -{
> -    QEMUFileBuffered *s = opaque;
> -    int64_t initial_time = qemu_get_clock_ms(rt_clock);
> -    int64_t max_size = 0;
> -    bool last_round = false;
> -
> -    while (true) {
> -        int64_t current_time = qemu_get_clock_ms(rt_clock);
> -
> -        if (s->migration_state->complete) {
> -            break;
> -        }
> -        if (current_time >= initial_time + BUFFER_DELAY) {
> -            uint64_t transferred_bytes = s->bytes_xfer;
> -            uint64_t time_spent = current_time - initial_time;
> -            double bandwidth = transferred_bytes / time_spent;
> -            max_size = bandwidth * migrate_max_downtime() / 1000000;
> -
> -            DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
> -                    " bandwidth %g max_size %" PRId64 "\n",
> -                    transferred_bytes, time_spent, bandwidth, max_size);
> -
> -            s->bytes_xfer = 0;
> -            initial_time = current_time;
> -        }
> -        if (!last_round && (s->bytes_xfer >= s->xfer_limit)) {
> -            /* usleep expects microseconds */
> -            usleep((initial_time + BUFFER_DELAY - current_time)*1000);
> -        }
> -        buffered_flush(s);
> -
> -        DPRINTF("file is ready\n");
> -        if (s->bytes_xfer < s->xfer_limit) {
> -            DPRINTF("notifying client\n");
> -            last_round = migrate_fd_put_ready(s->migration_state, max_size);
> -        }
> -    }
> -
> -    g_free(s->buffer);
> -    g_free(s);
> -    return NULL;
> -}
> -
> -void qemu_fopen_ops_buffered(MigrationState *migration_state)
> -{
> -    QEMUFileBuffered *s;
> -
> -    s = g_malloc0(sizeof(*s));
> -
> -    s->migration_state = migration_state;
> -    s->xfer_limit = migration_state->bandwidth_limit / 10;
> -    s->migration_state->complete = false;
> -
> -    s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
> -                             buffered_close, buffered_rate_limit,
> -                             buffered_set_rate_limit,
> -			     buffered_get_rate_limit);
> -
> -    migration_state->file = s->file;
> -
> -    qemu_thread_create(&s->thread, buffered_file_thread, s,
> -                       QEMU_THREAD_DETACHED);
> -}
> diff --git a/buffered_file.h b/buffered_file.h
> deleted file mode 100644
> index 8a246fd..0000000
> --- a/buffered_file.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -/*
> - * QEMU buffered QEMUFile
> - *
> - * Copyright IBM, Corp. 2008
> - *
> - * Authors:
> - *  Anthony Liguori   <aliguori@us.ibm.com>
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2.  See
> - * the COPYING file in the top-level directory.
> - *
> - */
> -
> -#ifndef QEMU_BUFFERED_FILE_H
> -#define QEMU_BUFFERED_FILE_H
> -
> -#include "hw/hw.h"
> -#include "migration.h"
> -
> -void qemu_fopen_ops_buffered(MigrationState *migration_state);
> -
> -#endif
> diff --git a/migration.c b/migration.c
> index 8054a77..b1567f3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -16,7 +16,7 @@
>  #include "qemu-common.h"
>  #include "migration.h"
>  #include "monitor.h"
> -#include "buffered_file.h"
> +#include "qemu-file.h"
>  #include "sysemu.h"
>  #include "block.h"
>  #include "qemu_socket.h"
> @@ -569,3 +569,219 @@ int64_t migrate_xbzrle_cache_size(void)
> 
>      return s->xbzrle_cache_size;
>  }
> +
> +/* migration thread support */
> +
> +typedef struct QEMUFileBuffered {
> +    MigrationState *migration_state;
> +    QEMUFile *file;
> +    size_t bytes_xfer;
> +    size_t xfer_limit;
> +    uint8_t *buffer;
> +    size_t buffer_size;
> +    size_t buffer_capacity;
> +    QemuThread thread;
> +} QEMUFileBuffered;
> +
> +static ssize_t buffered_flush(QEMUFileBuffered *s)
> +{
> +    size_t offset = 0;
> +    ssize_t ret = 0;
> +
> +    DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
> +
> +    while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
> +
> +        ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
> +                                    s->buffer_size - offset);
> +        if (ret <= 0) {
> +            DPRINTF("error flushing data, %zd\n", ret);
> +            break;
> +        } else {
> +            DPRINTF("flushed %zd byte(s)\n", ret);
> +            offset += ret;
> +            s->bytes_xfer += ret;
> +        }
> +    }
> +
> +    DPRINTF("flushed %zu of %zu byte(s)\n", offset, s->buffer_size);
> +    memmove(s->buffer, s->buffer + offset, s->buffer_size - offset);
> +    s->buffer_size -= offset;
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return offset;
> +}
> +
> +static int buffered_put_buffer(void *opaque, const uint8_t *buf,
> +                               int64_t pos, int size)
> +{
> +    QEMUFileBuffered *s = opaque;
> +    ssize_t error;
> +
> +    DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
> +
> +    error = qemu_file_get_error(s->file);
> +    if (error) {
> +        DPRINTF("flush when error, bailing: %s\n", strerror(-error));
> +        return error;
> +    }
> +
> +    if (size <= 0) {
> +        return size;
> +    }
> +
> +    if (size > (s->buffer_capacity - s->buffer_size)) {
> +        DPRINTF("increasing buffer capacity from %zu by %zu\n",
> +                s->buffer_capacity, size + 1024);
> +
> +        s->buffer_capacity += size + 1024;
> +
> +        s->buffer = g_realloc(s->buffer, s->buffer_capacity);
> +    }
> +
> +    memcpy(s->buffer + s->buffer_size, buf, size);
> +    s->buffer_size += size;
> +
> +    return size;
> +}
> +
> +static int buffered_close(void *opaque)
> +{
> +    QEMUFileBuffered *s = opaque;
> +    ssize_t ret = 0;
> +    int ret2;
> +
> +    DPRINTF("closing\n");
> +
> +    s->xfer_limit = INT_MAX;
> +    while (!qemu_file_get_error(s->file) && s->buffer_size) {
> +        ret = buffered_flush(s);
> +        if (ret < 0) {
> +            break;
> +        }
> +    }
> +
> +    ret2 = migrate_fd_close(s->migration_state);
> +    if (ret >= 0) {
> +        ret = ret2;
> +    }
> +    ret = migrate_fd_close(s->migration_state);
> +    s->migration_state->complete = true;
> +    return ret;
> +}
> +
> +/*
> + * The meaning of the return values is:
> + *   0: We can continue sending
> + *   1: Time to stop
> + *   negative: There has been an error
> + */
> +static int buffered_rate_limit(void *opaque)
> +{
> +    QEMUFileBuffered *s = opaque;
> +    int ret;
> +
> +    ret = qemu_file_get_error(s->file);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    if (s->bytes_xfer > s->xfer_limit) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int64_t buffered_set_rate_limit(void *opaque, int64_t new_rate)
> +{
> +    QEMUFileBuffered *s = opaque;
> +    if (qemu_file_get_error(s->file)) {
> +        goto out;
> +    }
> +    if (new_rate > SIZE_MAX) {
> +        new_rate = SIZE_MAX;
> +    }
> +
> +    s->xfer_limit = new_rate / 10;
> +
> +out:
> +    return s->xfer_limit;
> +}
> +
> +static int64_t buffered_get_rate_limit(void *opaque)
> +{
> +    QEMUFileBuffered *s = opaque;
> +
> +    return s->xfer_limit;
> +}
> +
> +/* 100ms  xfer_limit is the limit that we should write each 100ms */
> +#define BUFFER_DELAY 100
> +
> +static void *buffered_file_thread(void *opaque)
> +{
> +    QEMUFileBuffered *s = opaque;
> +    int64_t initial_time = qemu_get_clock_ms(rt_clock);
> +    int64_t max_size = 0;
> +    bool last_round = false;
> +
> +    while (true) {
> +        int64_t current_time = qemu_get_clock_ms(rt_clock);
> +
> +        if (s->migration_state->complete) {
> +            break;
> +        }
> +        if (current_time >= initial_time + BUFFER_DELAY) {
> +            uint64_t transferred_bytes = s->bytes_xfer;
> +            uint64_t time_spent = current_time - initial_time;
> +            double bandwidth = transferred_bytes / time_spent;
> +            max_size = bandwidth * migrate_max_downtime() / 1000000;
> +
> +            DPRINTF("transferred %" PRIu64 " time_spent %" PRIu64
> +                    " bandwidth %g max_size %" PRId64 "\n",
> +                    transferred_bytes, time_spent, bandwidth, max_size);
> +
> +            s->bytes_xfer = 0;
> +            initial_time = current_time;
> +        }
> +        if (!last_round && (s->bytes_xfer >= s->xfer_limit)) {
> +            /* usleep expects microseconds */
> +            usleep((initial_time + BUFFER_DELAY - current_time)*1000);
> +        }
> +        buffered_flush(s);
> +
> +        DPRINTF("file is ready\n");
> +        if (s->bytes_xfer < s->xfer_limit) {
> +            DPRINTF("notifying client\n");
> +            last_round = migrate_fd_put_ready(s->migration_state, max_size);
> +        }
> +    }
> +
> +    g_free(s->buffer);
> +    g_free(s);
> +    return NULL;
> +}
> +
> +void qemu_fopen_ops_buffered(MigrationState *migration_state)
> +{
> +    QEMUFileBuffered *s;
> +
> +    s = g_malloc0(sizeof(*s));
> +
> +    s->migration_state = migration_state;
> +    s->xfer_limit = migration_state->bandwidth_limit / 10;
> +    s->migration_state->complete = false;
> +
> +    s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
> +                             buffered_close, buffered_rate_limit,
> +                             buffered_set_rate_limit,
> +                             buffered_get_rate_limit);
> +
> +    migration_state->file = s->file;
> +
> +    qemu_thread_create(&s->thread, buffered_file_thread, s,
> +                       QEMU_THREAD_DETACHED);
> +}
> diff --git a/migration.h b/migration.h
> index 1f2baed..40c8e9c 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -129,4 +129,5 @@ int64_t migrate_xbzrle_cache_size(void);
> 
>  int64_t xbzrle_cache_resize(int64_t new_size);
> 
> +void qemu_fopen_ops_buffered(MigrationState *migration_state);
>  #endif
> 

Re: [Qemu-devel] [PATCH 00/30] Migration thread 20121017 edition

[Paolo Bonzini]

Il 18/10/2012 09:29, Juan Quintela ha scritto:
> v3:
> 
> This is work in progress on top of the previous migration series just sent.
> 
> - Introduces a thread for migration instead of using a timer and callback
> - remove the writting to the fd from the iothread lock
> - make the writes synchronous
> - Introduce a new pending method that returns how many bytes are pending for
>   one save live section
> - last patch just shows printfs to see where the time is being spent
>   on the migration complete phase.
>   (yes it pollutes all uses of stop on the monitor)
> 
> So far I have found that we spent a lot of time on bdrv_flush_all() It
> can take from 1ms to 600ms (yes, it is not a typo).  That dwarfs the
> migration default downtime time (30ms).
> 
> Stop all vcpus:
> 
> - it works now (after the changes on qemu_cpu_is_vcpu on the previous
>   series) caveat is that the time that brdv_flush_all() takes is
>   "unpredictable".  Any silver bullets?

You could reuse the "block" live migration item.  In block_save_pending,
start a bdrv_aio_flush() on all block devices that have already
completed the previous one.

But that's not a regression in the migration thread, isn't it?

Paolo

Re: [Qemu-devel] [PATCH 27/30] ram: Use memory_region_test_and_clear_dirty

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

On 10/18/2012 01:30 AM, Juan Quintela wrote:
> This avoids having to do two walks over the dirty bitmap, once reading
> the dirty bits, and anthoer cleaning them.

s/anthoer/another/

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH 01/30] split MRU ram list

[Orit Wasserman]

On 10/18/2012 09:29 AM, Juan Quintela wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Outside the execution threads the normal, non-MRU-ized order of
> the RAM blocks should always be enough.  So manage two separate
> lists, which will have separate locking rules.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c |  1 +
>  cpu-all.h   |  4 +++-
>  exec.c      | 18 +++++++++++++-----
>  3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index e6effe8..4293557 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -48,6 +48,7 @@
>  #include "qemu/page_cache.h"
>  #include "qmp-commands.h"
>  #include "trace.h"
> +#include "cpu-all.h"
> 
>  #ifdef DEBUG_ARCH_INIT
>  #define DPRINTF(fmt, ...) \
> diff --git a/cpu-all.h b/cpu-all.h
> index 6aa7e58..6558a6f 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -490,8 +490,9 @@ typedef struct RAMBlock {
>      ram_addr_t offset;
>      ram_addr_t length;
>      uint32_t flags;
> -    char idstr[256];
> +    QLIST_ENTRY(RAMBlock) next_mru;
>      QLIST_ENTRY(RAMBlock) next;
> +    char idstr[256];
>  #if defined(__linux__) && !defined(TARGET_S390X)
>      int fd;
>  #endif
> @@ -499,6 +500,7 @@ typedef struct RAMBlock {
> 
>  typedef struct RAMList {
>      uint8_t *phys_dirty;
> +    QLIST_HEAD(, RAMBlock) blocks_mru;
>      QLIST_HEAD(, RAMBlock) blocks;
>  } RAMList;
>  extern RAMList ram_list;
> diff --git a/exec.c b/exec.c
> index e63ad0d..718bbc2 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -56,6 +56,7 @@
>  #include "xen-mapcache.h"
>  #include "trace.h"
>  #endif
> +#include "cpu-all.h"
> 
>  #include "cputlb.h"
> 
> @@ -112,7 +113,10 @@ static uint8_t *code_gen_ptr;
>  int phys_ram_fd;
>  static int in_migration;
> 
> -RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
> +RAMList ram_list = {
> +    .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks),
> +    .blocks_mru = QLIST_HEAD_INITIALIZER(ram_list.blocks_mru)
> +};
> 
>  static MemoryRegion *system_memory;
>  static MemoryRegion *system_io;
> @@ -633,6 +637,7 @@ bool tcg_enabled(void)
>  void cpu_exec_init_all(void)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> +    qemu_mutex_init(&ram_list.mutex);
>      memory_map_init();
>      io_mem_init();
>  #endif
> @@ -2568,6 +2573,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>      new_block->length = size;
> 
>      QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
> +    QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);
> 
>      ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>                                         last_ram_offset() >> TARGET_PAGE_BITS);
> @@ -2595,6 +2601,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
> +            QLIST_REMOVE(block, next_mru);
>              g_free(block);
>              return;
>          }
> @@ -2608,6 +2615,7 @@ void qemu_ram_free(ram_addr_t addr)
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
> +            QLIST_REMOVE(block, next_mru);
>              if (block->flags & RAM_PREALLOC_MASK) {
>                  ;
>              } else if (mem_path) {
> @@ -2713,12 +2721,12 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
>  {
>      RAMBlock *block;
> 
> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
> +    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
>          if (addr - block->offset < block->length) {
>              /* Move this entry to to start of the list.  */
>              if (block != QLIST_FIRST(&ram_list.blocks)) {
> -                QLIST_REMOVE(block, next);
> -                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
> +                QLIST_REMOVE(block, next_mru);
> +                QLIST_INSERT_HEAD(&ram_list.blocks_mru, block, next_mru);
>              }
>              if (xen_enabled()) {
>                  /* We need to check if the requested address is in the RAM
> @@ -2813,7 +2821,7 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
>          return 0;
>      }
> 
> -    QLIST_FOREACH(block, &ram_list.blocks, next) {
> +    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
>          /* This case append when the block is not mapped. */
>          if (block->host == NULL) {
>              continue;
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Re: [Qemu-devel] [PATCH 02/30] add a version number to ram_list

[Orit Wasserman]

On 10/18/2012 09:29 AM, Juan Quintela wrote:
> From: Umesh Deshpande <udeshpan@redhat.com>
> 
> This will be used to detect if last_block might have become invalid
> across different calls to ram_save_live.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 7 ++++++-
>  cpu-all.h   | 1 +
>  exec.c      | 5 ++++-
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 4293557..b47313d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -336,6 +336,7 @@ static RAMBlock *last_block;
>  static ram_addr_t last_offset;
>  static unsigned long *migration_bitmap;
>  static uint64_t migration_dirty_pages;
> +static uint32_t last_version;
> 
>  static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
>                                                           ram_addr_t offset)
> @@ -406,7 +407,6 @@ static void migration_bitmap_sync(void)
>      }
>  }
> 
> -
>  /*
>   * ram_save_block: Writes a page of memory to the stream f
>   *
> @@ -558,6 +558,7 @@ static void reset_ram_globals(void)
>  {
>      last_block = NULL;
>      last_offset = 0;
> +    last_version = ram_list.version;
>      sort_ram_list();
>  }
> 
> @@ -613,6 +614,10 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      uint64_t expected_downtime;
>      MigrationState *s = migrate_get_current();
> 
> +    if (ram_list.version != last_version) {
> +        reset_ram_globals();
> +    }
> +
>      bytes_transferred_last = bytes_transferred;
>      bwidth = qemu_get_clock_ns(rt_clock);
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 6558a6f..e07c91c 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -500,6 +500,7 @@ typedef struct RAMBlock {
> 
>  typedef struct RAMList {
>      uint8_t *phys_dirty;
> +    uint32_t version;
>      QLIST_HEAD(, RAMBlock) blocks_mru;
>      QLIST_HEAD(, RAMBlock) blocks;
>  } RAMList;
> diff --git a/exec.c b/exec.c
> index 718bbc2..1e04711 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -637,7 +637,6 @@ bool tcg_enabled(void)
>  void cpu_exec_init_all(void)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> -    qemu_mutex_init(&ram_list.mutex);
>      memory_map_init();
>      io_mem_init();
>  #endif
> @@ -2575,6 +2574,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>      QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
>      QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);
> 
> +    ram_list.version++;
> +
>      ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>                                         last_ram_offset() >> TARGET_PAGE_BITS);
>      memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
> @@ -2602,6 +2603,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
>              QLIST_REMOVE(block, next_mru);
> +            ram_list.version++;
>              g_free(block);
>              return;
>          }
> @@ -2616,6 +2618,7 @@ void qemu_ram_free(ram_addr_t addr)
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
>              QLIST_REMOVE(block, next_mru);
> +            ram_list.version++;
>              if (block->flags & RAM_PREALLOC_MASK) {
>                  ;
>              } else if (mem_path) {
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Re: [Qemu-devel] [PATCH 03/30] protect the ramlist with a separate mutex

[Orit Wasserman]

On 10/18/2012 09:29 AM, Juan Quintela wrote:
> From: Umesh Deshpande <udeshpan@redhat.com>
> 
> Add the new mutex that protects shared state between ram_save_live
> and the iothread.  If the iothread mutex has to be taken together
> with the ramlist mutex, the iothread shall always be _outside_.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c |  9 ++++++++-
>  cpu-all.h   |  8 ++++++++
>  exec.c      | 23 +++++++++++++++++++++--
>  3 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index b47313d..2d29828 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -553,7 +553,6 @@ static void ram_migration_cancel(void *opaque)
>      migration_end();
>  }
> 
> -
>  static void reset_ram_globals(void)
>  {
>      last_block = NULL;
> @@ -573,6 +572,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      bitmap_set(migration_bitmap, 1, ram_pages);
>      migration_dirty_pages = ram_pages;
> 
> +    qemu_mutex_lock_ramlist();
>      bytes_transferred = 0;
>      reset_ram_globals();
> 
> @@ -600,6 +600,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>          qemu_put_be64(f, block->length);
>      }
> 
> +    qemu_mutex_unlock_ramlist();
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> 
>      return 0;
> @@ -614,6 +615,8 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      uint64_t expected_downtime;
>      MigrationState *s = migrate_get_current();
> 
> +    qemu_mutex_lock_ramlist();
> +
>      if (ram_list.version != last_version) {
>          reset_ram_globals();
>      }
> @@ -662,6 +665,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          bwidth = 0.000001;
>      }
> 
> +    qemu_mutex_unlock_ramlist();
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> 
>      expected_downtime = ram_save_remaining() * TARGET_PAGE_SIZE / bwidth;
> @@ -682,6 +686,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  {
>      migration_bitmap_sync();
> 
> +    qemu_mutex_lock_ramlist();
> +
>      /* try transferring iterative blocks of memory */
> 
>      /* flush all remaining blocks regardless of rate limiting */
> @@ -697,6 +703,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>      }
>      memory_global_dirty_log_stop();
> 
> +    qemu_mutex_unlock_ramlist();
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> 
>      g_free(migration_bitmap);
> diff --git a/cpu-all.h b/cpu-all.h
> index e07c91c..2bafcdd 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -22,6 +22,7 @@
>  #include "qemu-common.h"
>  #include "qemu-tls.h"
>  #include "cpu-common.h"
> +#include "qemu-thread.h"
> 
>  /* some important defines:
>   *
> @@ -490,7 +491,9 @@ typedef struct RAMBlock {
>      ram_addr_t offset;
>      ram_addr_t length;
>      uint32_t flags;
> +    /* Protected by the iothread lock.  */
>      QLIST_ENTRY(RAMBlock) next_mru;
> +    /* Protected by the ramlist lock.  */
>      QLIST_ENTRY(RAMBlock) next;
>      char idstr[256];
>  #if defined(__linux__) && !defined(TARGET_S390X)
> @@ -499,9 +502,12 @@ typedef struct RAMBlock {
>  } RAMBlock;
> 
>  typedef struct RAMList {
> +    QemuMutex mutex;
> +    /* Protected by the iothread lock.  */
>      uint8_t *phys_dirty;
>      uint32_t version;
>      QLIST_HEAD(, RAMBlock) blocks_mru;
> +    /* Protected by the ramlist lock.  */
>      QLIST_HEAD(, RAMBlock) blocks;
>  } RAMList;
>  extern RAMList ram_list;
> @@ -521,6 +527,8 @@ extern int mem_prealloc;
> 
>  void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
>  ram_addr_t last_ram_offset(void);
> +void qemu_mutex_lock_ramlist(void);
> +void qemu_mutex_unlock_ramlist(void);
>  #endif /* !CONFIG_USER_ONLY */
> 
>  int cpu_memory_rw_debug(CPUArchState *env, target_ulong addr,
> diff --git a/exec.c b/exec.c
> index 1e04711..cf9de92 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -637,6 +637,7 @@ bool tcg_enabled(void)
>  void cpu_exec_init_all(void)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> +    qemu_mutex_init(&ram_list.mutex);
>      memory_map_init();
>      io_mem_init();
>  #endif
> @@ -2329,6 +2330,16 @@ void qemu_flush_coalesced_mmio_buffer(void)
>          kvm_flush_coalesced_mmio_buffer();
>  }
> 
> +void qemu_mutex_lock_ramlist(void)
> +{
> +    qemu_mutex_lock(&ram_list.mutex);
> +}
> +
> +void qemu_mutex_unlock_ramlist(void)
> +{
> +    qemu_mutex_unlock(&ram_list.mutex);
> +}
> +
>  #if defined(__linux__) && !defined(TARGET_S390X)
> 
>  #include <sys/vfs.h>
> @@ -2510,6 +2521,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
>      }
>      pstrcat(new_block->idstr, sizeof(new_block->idstr), name);
> 
> +    qemu_mutex_lock_ramlist();
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (block != new_block && !strcmp(block->idstr, new_block->idstr)) {
>              fprintf(stderr, "RAMBlock \"%s\" already registered, abort!\n",
> @@ -2517,6 +2529,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
>              abort();
>          }
>      }
> +    qemu_mutex_unlock_ramlist();
>  }
> 
>  static int memory_try_enable_merging(void *addr, size_t len)
> @@ -2540,6 +2553,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>      size = TARGET_PAGE_ALIGN(size);
>      new_block = g_malloc0(sizeof(*new_block));
> 
> +    qemu_mutex_lock_ramlist();
>      new_block->mr = mr;
>      new_block->offset = find_ram_offset(size);
>      if (host) {
> @@ -2575,6 +2589,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>      QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);
> 
>      ram_list.version++;
> +    qemu_mutex_unlock_ramlist();
> 
>      ram_list.phys_dirty = g_realloc(ram_list.phys_dirty,
>                                         last_ram_offset() >> TARGET_PAGE_BITS);
> @@ -2599,21 +2614,24 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
>  {
>      RAMBlock *block;
> 
> +    qemu_mutex_lock_ramlist();
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
>              QLIST_REMOVE(block, next_mru);
>              ram_list.version++;
>              g_free(block);
> -            return;
> +            break;
>          }
>      }
> +    qemu_mutex_unlock_ramlist();
>  }
> 
>  void qemu_ram_free(ram_addr_t addr)
>  {
>      RAMBlock *block;
> 
> +    qemu_mutex_lock_ramlist();
>      QLIST_FOREACH(block, &ram_list.blocks, next) {
>          if (addr == block->offset) {
>              QLIST_REMOVE(block, next);
> @@ -2644,9 +2662,10 @@ void qemu_ram_free(ram_addr_t addr)
>  #endif
>              }
>              g_free(block);
> -            return;
> +            break;
>          }
>      }
> +    qemu_mutex_unlock_ramlist();
> 
>  }
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Re: [Qemu-devel] [PATCH 04/30] buffered_file: Move from using a timer to use a thread

[Orit Wasserman]

On 10/18/2012 09:30 AM, Juan Quintela wrote:
> We still protect everything except the wait with the iothread lock.
> But we moved from a timer to a thread.  Steps one by one.
> 
> We also need to detect when we have finished with a variable "complete".
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 58 +++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index ed92df1..4b90d54 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -18,6 +18,7 @@
>  #include "qemu-timer.h"
>  #include "qemu-char.h"
>  #include "buffered_file.h"
> +#include "qemu-thread.h"
> 
>  //#define DEBUG_BUFFERED_FILE
> 
> @@ -31,7 +32,8 @@ typedef struct QEMUFileBuffered
>      uint8_t *buffer;
>      size_t buffer_size;
>      size_t buffer_capacity;
> -    QEMUTimer *timer;
> +    QemuThread thread;
> +    bool complete;
>  } QEMUFileBuffered;
> 
>  #ifdef DEBUG_BUFFERED_FILE
> @@ -160,11 +162,8 @@ static int buffered_close(void *opaque)
>      if (ret >= 0) {
>          ret = ret2;
>      }
> -    qemu_del_timer(s->timer);
> -    qemu_free_timer(s->timer);
> -    g_free(s->buffer);
> -    g_free(s);
> -
> +    ret = migrate_fd_close(s->migration_state);
> +    s->complete = true;
>      return ret;
>  }
> 
> @@ -215,23 +214,38 @@ static int64_t buffered_get_rate_limit(void *opaque)
>      return s->xfer_limit;
>  }
> 
> -static void buffered_rate_tick(void *opaque)
> +/* 10ms  xfer_limit is the limit that we should write each 10ms */
maybe 100, please fix the comment

Orit
> +#define BUFFER_DELAY 100
> +
> +static void *buffered_file_thread(void *opaque)
>  {
>      QEMUFileBuffered *s = opaque;
> +    int64_t expire_time = qemu_get_clock_ms(rt_clock) + BUFFER_DELAY;
> 
> -    if (qemu_file_get_error(s->file)) {
> -        buffered_close(s);
> -        return;
> -    }
> -
> -    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
> -
> -    if (s->freeze_output)
> -        return;
> -
> -    s->bytes_xfer = 0;
> +    while (true) {
> +        int64_t current_time = qemu_get_clock_ms(rt_clock);
> 
> -    buffered_put_buffer(s, NULL, 0, 0);
> +        if (s->complete) {
> +            break;
> +        }
> +        if (s->freeze_output) {
> +            continue;
> +        }
> +        if (current_time >= expire_time) {
> +            s->bytes_xfer = 0;
> +            expire_time = current_time + BUFFER_DELAY;
> +        }
> +        if (s->bytes_xfer >= s->xfer_limit) {
> +            /* usleep expects microseconds */
> +            usleep((expire_time - current_time)*1000);
> +        }
> +        qemu_mutex_lock_iothread();
> +        buffered_put_buffer(s, NULL, 0, 0);
> +        qemu_mutex_unlock_iothread();
> +    }
> +    g_free(s->buffer);
> +    g_free(s);
> +    return NULL;
>  }
> 
>  QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
> @@ -242,15 +256,15 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
> 
>      s->migration_state = migration_state;
>      s->xfer_limit = migration_state->bandwidth_limit / 10;
> +    s->complete = false;
> 
>      s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
>                               buffered_close, buffered_rate_limit,
>                               buffered_set_rate_limit,
>  			     buffered_get_rate_limit);
> 
> -    s->timer = qemu_new_timer_ms(rt_clock, buffered_rate_tick, s);
> -
> -    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
> +    qemu_thread_create(&s->thread, buffered_file_thread, s,
> +                       QEMU_THREAD_DETACHED);
> 
>      return s->file;
>  }
> 

Re: [Qemu-devel] [PATCH 05/30] migration: make qemu_fopen_ops_buffered() return void

[Orit Wasserman]

On 10/18/2012 09:30 AM, Juan Quintela wrote:
> We want the file assignment to happen before the thread is created to
> avoid locking, so we just do it before creating the thread.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 13 ++++++-------
>  buffered_file.h |  2 +-
>  migration.c     |  2 +-
>  migration.h     |  1 +
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 4b90d54..6395b37 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -33,7 +33,6 @@ typedef struct QEMUFileBuffered
>      size_t buffer_size;
>      size_t buffer_capacity;
>      QemuThread thread;
> -    bool complete;
>  } QEMUFileBuffered;
> 
>  #ifdef DEBUG_BUFFERED_FILE
> @@ -163,7 +162,7 @@ static int buffered_close(void *opaque)
>          ret = ret2;
>      }
>      ret = migrate_fd_close(s->migration_state);
> -    s->complete = true;
> +    s->migration_state->complete = true;
>      return ret;
>  }
> 
> @@ -225,7 +224,7 @@ static void *buffered_file_thread(void *opaque)
>      while (true) {
>          int64_t current_time = qemu_get_clock_ms(rt_clock);
> 
> -        if (s->complete) {
> +        if (s->migration_state->complete) {
>              break;
>          }
>          if (s->freeze_output) {
> @@ -248,7 +247,7 @@ static void *buffered_file_thread(void *opaque)
>      return NULL;
>  }
> 
> -QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
> +void qemu_fopen_ops_buffered(MigrationState *migration_state)
>  {
>      QEMUFileBuffered *s;
> 
> @@ -256,15 +255,15 @@ QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state)
> 
>      s->migration_state = migration_state;
>      s->xfer_limit = migration_state->bandwidth_limit / 10;
> -    s->complete = false;
> +    s->migration_state->complete = false;
> 
>      s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
>                               buffered_close, buffered_rate_limit,
>                               buffered_set_rate_limit,
>  			     buffered_get_rate_limit);
> 
> +    migration_state->file = s->file;
> +
>      qemu_thread_create(&s->thread, buffered_file_thread, s,
>                         QEMU_THREAD_DETACHED);
> -
> -    return s->file;
>  }
> diff --git a/buffered_file.h b/buffered_file.h
> index ef010fe..8a246fd 100644
> --- a/buffered_file.h
> +++ b/buffered_file.h
> @@ -17,6 +17,6 @@
>  #include "hw/hw.h"
>  #include "migration.h"
> 
> -QEMUFile *qemu_fopen_ops_buffered(MigrationState *migration_state);
> +void qemu_fopen_ops_buffered(MigrationState *migration_state);
> 
>  #endif
> diff --git a/migration.c b/migration.c
> index 62e0304..02f4ffa 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -431,7 +431,7 @@ void migrate_fd_connect(MigrationState *s)
>      int ret;
> 
>      s->state = MIG_STATE_ACTIVE;
> -    s->file = qemu_fopen_ops_buffered(s);
> +    qemu_fopen_ops_buffered(s);
> 
>      DPRINTF("beginning savevm\n");
>      ret = qemu_savevm_state_begin(s->file, &s->params);
> diff --git a/migration.h b/migration.h
> index 1c3e9b7..a63c5d5 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -45,6 +45,7 @@ struct MigrationState
>      int64_t dirty_pages_rate;
>      bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
>      int64_t xbzrle_cache_size;
> +    bool complete;
>  };
> 
>  void process_incoming_migration(QEMUFile *f);
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Re: [Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Orit Wasserman]

On 10/18/2012 09:30 AM, Juan Quintela wrote:
> Instead of testing each page individually, we search what is the next
> dirty page with a bitmap operation.  We have to reorganize the code to
> move from a "for" loop, to a while(dirty) loop.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 45 ++++++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 8391375..cd6ebef 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -343,18 +343,21 @@ static unsigned long *migration_bitmap;
>  static uint64_t migration_dirty_pages;
>  static uint32_t last_version;
> 
> -static inline bool migration_bitmap_test_and_reset_dirty(MemoryRegion *mr,
> -                                                         ram_addr_t offset)
> +static inline
> +ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
> +                                                 ram_addr_t start)
>  {
> -    bool ret;
> -    int nr = (mr->ram_addr + offset) >> TARGET_PAGE_BITS;
> +    unsigned long base = mr->ram_addr >> TARGET_PAGE_BITS;
> +    unsigned long nr = base + (start >> TARGET_PAGE_BITS);
> +    unsigned long size = base + (int128_get64(mr->size) >> TARGET_PAGE_BITS);
> 
> -    ret = test_and_clear_bit(nr, migration_bitmap);
> +    unsigned long next = find_next_bit(migration_bitmap, size, nr);
> 
> -    if (ret) {
> +    if (next < size) {
> +        clear_bit(next, migration_bitmap);
>          migration_dirty_pages--;
>      }
> -    return ret;
> +    return (next - base) << TARGET_PAGE_BITS;
>  }
> 
>  static inline bool migration_bitmap_set_dirty(MemoryRegion *mr,
> @@ -423,6 +426,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>  {
>      RAMBlock *block = last_seen_block;
>      ram_addr_t offset = last_offset;
> +    bool complete_round = false;
>      int bytes_sent = -1;
>      MemoryRegion *mr;
>      ram_addr_t current_addr;
> @@ -430,9 +434,21 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>      if (!block)
>          block = QLIST_FIRST(&ram_list.blocks);
> 
> -    do {
> +    while(true) {
>          mr = block->mr;
> -        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
> +        offset = migration_bitmap_find_and_reset_dirty(mr, offset);
> +        if (complete_round && block == last_seen_block &&
> +            offset >= last_offset) {
> +            break;
> +        }
Juan,
You need to exchange those line, first check to see you did a full round than
calculate and reset the offset, in the way it is written now you may reset a bit and than break of the loop
without sending it.

Orit
> +        if (offset >= block->length) {
> +            offset = 0;
> +            block = QLIST_NEXT(block, next);
> +            if (!block) {
> +                block = QLIST_FIRST(&ram_list.blocks);
> +                complete_round = true;
> +            }
> +        } else {
>              uint8_t *p;
>              int cont = (block == last_sent_block) ?
>                  RAM_SAVE_FLAG_CONTINUE : 0;
> @@ -467,16 +483,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>                  break;
>              }
>          }
> -
> -        offset += TARGET_PAGE_SIZE;
> -        if (offset >= block->length) {
> -            offset = 0;
> -            block = QLIST_NEXT(block, next);
> -            if (!block)
> -                block = QLIST_FIRST(&ram_list.blocks);
> -        }
> -    } while (block != last_seen_block || offset != last_offset);
> -
> +    }
>      last_seen_block = block;
>      last_offset = offset;
> 

Re: [Qemu-devel] [PATCH 01/30] split MRU ram list

[Peter Maydell]

On 18 October 2012 08:29, Juan Quintela <quintela@redhat.com> wrote:
> -RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks) };
> +RAMList ram_list = {
> +    .blocks = QLIST_HEAD_INITIALIZER(ram_list.blocks),
> +    .blocks_mru = QLIST_HEAD_INITIALIZER(ram_list.blocks_mru)
> +};

Maybe worth adding a comment explaining why we have the two lists,
to save people having to ferret around in the revision history later?

-- PMM

Re: [Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
> On 10/18/2012 09:30 AM, Juan Quintela wrote:
>> Instead of testing each page individually, we search what is the next
>> dirty page with a bitmap operation.  We have to reorganize the code to
>> move from a "for" loop, to a while(dirty) loop.
>> 

>> 
>> -    do {
>> +    while(true) {
>>          mr = block->mr;
>> -        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
>> +        offset = migration_bitmap_find_and_reset_dirty(mr, offset);
>> +        if (complete_round && block == last_seen_block &&
>> +            offset >= last_offset) {
>> +            break;
>> +        }
> Juan,
> You need to exchange those line, first check to see you did a full round than
> calculate and reset the offset, in the way it is written now you may
> reset a bit and than break of the loop
> without sending it.

How?

if complete_round == true, it means that we are in the second round.

block == last_seen_block means that we are back at the 1st block that we
have looked.

if offset >= last_offset, there are two options:
a- == last_offset: that was the 1st one that we checked, so it can't be
   true.
b- >= last_offset: it means tat we have already passed that bit, it
   _has_ to be zero, otherwise somebody has changed the bitmap under or
   foot.

Or have I missed something?
Notice that at some point we should allow for concurrent dirty of the
bitmap, but we need to do yet more things.

Later, Juan.

Re: [Qemu-devel] [PATCH 21/30] migration: move exit condition to migration thread

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 18/10/2012 09:30, Juan Quintela ha scritto:
>> -        if (s->migration_state->complete) {
>> +        qemu_mutex_lock_iothread();
>
> So, was it a bug that we were accessing ->complete without the BQL?
>
>> +        if (m->state != MIG_STATE_ACTIVE) {
>> +            DPRINTF("put_ready returning because of non-active state\n");
>
> The contents of the debug message obsolete.  Besides, I would just put
> the two branches in the same "if (m->state != MIG_STATE_ACTIVE ||
> m->complete)".

BQL is not needed.  Complete is only used by the migration thread.  but
maintaing it outside of the iothread lock, makes locking even more complicated.

>
>> +            qemu_mutex_unlock_iothread();
>>              break;
>>          }
>> +        if (m->complete) {
>> +            qemu_mutex_unlock_iothread();
>> +            break;
>> +        }
>> +        qemu_mutex_unlock_iothread();
>> +
>
> Paolo

Re: [Qemu-devel] [PATCH 00/30] Migration thread 20121017 edition

[Paolo Bonzini]

Il 18/10/2012 09:29, Juan Quintela ha scritto:
> Hi
> 
> This series apply on top of the refactoring that I sent yesterday.
> Changes from the last version include:
> 
> - buffered_file.c is gone, its functionality is merged in migration.c
>   special attention to the megre of buffered_file_thread() &
>   migration_file_put_notify().
> 
> - Some more bitmap handling optimizations (thanks to Orit & Paolo for
>   suggestions and code and Vinod for testing)
> 
> Please review.  Included is the pointer to the full tree.

Anthony, I think patches 1-13 are ready to go (but only if Juan reviews
my incoming-migration-in-a-coroutine first ;)).

Paolo

Re: [Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Orit Wasserman]

On 10/26/2012 01:39 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> On 10/18/2012 09:30 AM, Juan Quintela wrote:
>>> Instead of testing each page individually, we search what is the next
>>> dirty page with a bitmap operation.  We have to reorganize the code to
>>> move from a "for" loop, to a while(dirty) loop.
>>>
> 
>>>
>>> -    do {
>>> +    while(true) {
>>>          mr = block->mr;
>>> -        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
>>> +        offset = migration_bitmap_find_and_reset_dirty(mr, offset);
>>> +        if (complete_round && block == last_seen_block &&
>>> +            offset >= last_offset) {
>>> +            break;
>>> +        }
>> Juan,
>> You need to exchange those line, first check to see you did a full round than
>> calculate and reset the offset, in the way it is written now you may
>> reset a bit and than break of the loop
>> without sending it.
> 
> How?
> 
> if complete_round == true, it means that we are in the second round.
> 
> block == last_seen_block means that we are back at the 1st block that we
> have looked.
> 
> if offset >= last_offset, there are two options:
> a- == last_offset: that was the 1st one that we checked, so it can't be
>    true.
> b- >= last_offset: it means tat we have already passed that bit, it
>    _has_ to be zero, otherwise somebody has changed the bitmap under or
>    foot.
> 
> Or have I missed something?
If we are in iterate state this means the bitmap is changing all the time,
even when we didn't finish a complete cycle (for example we get to the bandwidth limit, exit ram_save_iterate and sync the bitmap in pending).
This means that bits in part of the bitmap we already walked can get dirty again. 
In that case the if condition can be true for a dirty bit,
in the current code we reset it and than break for the loop, this means it is set clean without sending it, which is a problem.
Changing the line order will fix it (the way it was before).

Regards,
Orit

> Notice that at some point we should allow for concurrent dirty of the
> bitmap, but we need to do yet more things.
> 
> Later, Juan.
> 

Re: [Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Orit Wasserman]

On 10/28/2012 10:35 AM, Orit Wasserman wrote:
> On 10/26/2012 01:39 PM, Juan Quintela wrote:
>> Orit Wasserman <owasserm@redhat.com> wrote:
>>> On 10/18/2012 09:30 AM, Juan Quintela wrote:
>>>> Instead of testing each page individually, we search what is the next
>>>> dirty page with a bitmap operation.  We have to reorganize the code to
>>>> move from a "for" loop, to a while(dirty) loop.
>>>>
>>
>>>>
>>>> -    do {
>>>> +    while(true) {
>>>>          mr = block->mr;
>>>> -        if (migration_bitmap_test_and_reset_dirty(mr, offset)) {
>>>> +        offset = migration_bitmap_find_and_reset_dirty(mr, offset);
>>>> +        if (complete_round && block == last_seen_block &&
>>>> +            offset >= last_offset) {
>>>> +            break;
>>>> +        }
>>> Juan,
>>> You need to exchange those line, first check to see you did a full round than
>>> calculate and reset the offset, in the way it is written now you may
>>> reset a bit and than break of the loop
>>> without sending it.
>>
>> How?
>>
>> if complete_round == true, it means that we are in the second round.
>>
>> block == last_seen_block means that we are back at the 1st block that we
>> have looked.
>>
>> if offset >= last_offset, there are two options:
>> a- == last_offset: that was the 1st one that we checked, so it can't be
>>    true.
>> b- >= last_offset: it means tat we have already passed that bit, it
>>    _has_ to be zero, otherwise somebody has changed the bitmap under or
>>    foot.
>>
>> Or have I missed something?
> If we are in iterate state this means the bitmap is changing all the time,
> even when we didn't finish a complete cycle (for example we get to the bandwidth limit, exit ram_save_iterate and sync the bitmap in pending).
> This means that bits in part of the bitmap we already walked can get dirty again. 
> In that case the if condition can be true for a dirty bit,
> in the current code we reset it and than break for the loop, this means it is set clean without sending it, which is a problem.
> Changing the line order will fix it (the way it was before).
> 
OK read the code more thoroughly, there is no way that the bitmap will if we do a full cycle in ram_save_block,
We can actually simplify and  the function by checking the number of dirty pages. If it is zero we can return 
immediately.
If it is not zero it means we will exit the loop when we will find the dirty page.

Cheers,
Orit 
> Regards,
> Orit
> 
>> Notice that at some point we should allow for concurrent dirty of the
>> bitmap, but we need to do yet more things.
>>
>> Later, Juan.
>>
> 
> 

Re: [Qemu-devel] [PATCH 30/30] ram: optimize migration bitmap walking

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
>> If we are in iterate state this means the bitmap is changing all the time,
>> even when we didn't finish a complete cycle (for example we get to
>> the bandwidth limit, exit ram_save_iterate and sync the bitmap in
>> pending).
>> This means that bits in part of the bitmap we already walked can get
>> dirty again.
>> In that case the if condition can be true for a dirty bit,
>> in the current code we reset it and than break for the loop, this
>> means it is set clean without sending it, which is a problem.
>> Changing the line order will fix it (the way it was before).
>> 
> OK read the code more thoroughly, there is no way that the bitmap will
> if we do a full cycle in ram_save_block,
> We can actually simplify and the function by checking the number of
> dirty pages. If it is zero we can return
> immediately.
> If it is not zero it means we will exit the loop when we will find the
> dirty page.

Very good idea.  Thanks.

Later, Juan.

Re: [Qemu-devel] [PATCH 04/30] buffered_file: Move from using a timer to use a thread

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> @@ -160,11 +162,8 @@ static int buffered_close(void *opaque)
>      if (ret >= 0) {
>          ret = ret2;
>      }
> -    qemu_del_timer(s->timer);
> -    qemu_free_timer(s->timer);
> -    g_free(s->buffer);
> -    g_free(s);
> -
> +    ret = migrate_fd_close(s->migration_state);
> +    s->complete = true;
>      return ret;

Just above these lines there is

    ret2 = migrate_fd_close(s->migration_state);

I believe the second call to migrate_fd_close should be removed.

Paolo

Re: [Qemu-devel] [PATCH 06/30] migration: stop all cpus correctly

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> You can only stop all cpus from the iothread or an vcpu.  As we want
> to do it from the migration_thread, we need to do this dance with the
> botton handlers.
> 
> This patch is a request for ideas.  I can move this function to cpus.c, but
> wondered if there is an easy way of doing this?

The commit message here is stale (I think I had pointed this out
already).  Also, first_time should be a member of MigrationState,
initialized in migrate_fd_connect.

Paolo

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 02f4ffa..05ef1a3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -20,6 +20,7 @@
>  #include "sysemu.h"
>  #include "block.h"
>  #include "qemu_socket.h"
> +#include "qemu-thread.h"
>  #include "block-migration.h"
>  #include "qmp-commands.h"
> 
> @@ -322,11 +323,22 @@ ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
>  void migrate_fd_put_ready(MigrationState *s)
>  {
>      int ret;
> +    static bool first_time = true;
> 
>      if (s->state != MIG_STATE_ACTIVE) {
>          DPRINTF("put_ready returning because of non-active state\n");
>          return;
>      }
> +    if (first_time) {
> +        first_time = false;
> +        DPRINTF("beginning savevm\n");
> +        ret = qemu_savevm_state_begin(s->file, &s->params);
> +        if (ret < 0) {
> +            DPRINTF("failed, %d\n", ret);
> +            migrate_fd_error(s);
> +            return;
> +        }
> +    }
> 
>      DPRINTF("iterate\n");
>      ret = qemu_savevm_state_iterate(s->file);
> @@ -339,7 +351,11 @@ void migrate_fd_put_ready(MigrationState *s)
>          DPRINTF("done iterating\n");
>          start_time = qemu_get_clock_ms(rt_clock);
>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> -        vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +        if (old_vm_running) {
> +            vm_stop(RUN_STATE_FINISH_MIGRATE);
> +        } else {
> +            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +        }
> 
>          if (qemu_savevm_state_complete(s->file) < 0) {
>              migrate_fd_error(s);
> @@ -428,19 +444,8 @@ bool migration_has_failed(MigrationState *s)
> 
>  void migrate_fd_connect(MigrationState *s)
>  {
> -    int ret;
> -
>      s->state = MIG_STATE_ACTIVE;
>      qemu_fopen_ops_buffered(s);
> -
> -    DPRINTF("beginning savevm\n");
> -    ret = qemu_savevm_state_begin(s->file, &s->params);
> -    if (ret < 0) {
> -        DPRINTF("failed, %d\n", ret);
> -        migrate_fd_error(s);
> -        return;
> -    }
> -    migrate_fd_put_ready(s);
>  }
> 
>  static MigrationState *migrate_init(const MigrationParams *params)
> 

Re: [Qemu-devel] [PATCH 07/30] migration: make writes blocking

[Paolo Bonzini]

Il 18/10/2012 09:30, Juan Quintela ha scritto:
> @@ -247,10 +247,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>  {
>      int ret = 0;
> 
> -    if (s->fd != -1) {
> -        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -    }
> -
>      if (s->file) {
>          DPRINTF("closing file\n");
>          ret = qemu_fclose(s->file);

While reviewing this patch, I noticed that migrate_fd_cleanup calls
migrate_fd_close twice.  The first via qemu_fclose's own call to
buffered_close, the second directly.

Perhaps you can include something like

diff --git a/migration.c b/migration.c
--- a/migration.c
+++ b/migration.c
@@ -274,5 +274,5 @@ static int migrate_fd_cleanup(MigrationState *s)
     }

-    migrate_fd_close(s);
+    assert(s->fd == -1);
     return ret;
 }


somewhere in the series?

Paolo

Re: [Qemu-devel] [PATCH 06/30] migration: stop all cpus correctly

[Paolo Bonzini]

Il 12/11/2012 12:44, Paolo Bonzini ha scritto:
>> @@ -339,7 +351,11 @@ void migrate_fd_put_ready(MigrationState *s)
>>          DPRINTF("done iterating\n");
>>          start_time = qemu_get_clock_ms(rt_clock);
>>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>> -        vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>> +        if (old_vm_running) {
>> +            vm_stop(RUN_STATE_FINISH_MIGRATE);
>> +        } else {
>> +            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>> +        }
>> 
>>          if (qemu_savevm_state_complete(s->file) < 0) {
>>              migrate_fd_error(s);

This hunk also seems to be useless nowadays.

Paolo

Re: [Qemu-devel] [PATCH 06/30] migration: stop all cpus correctly

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 12/11/2012 12:44, Paolo Bonzini ha scritto:
>>> @@ -339,7 +351,11 @@ void migrate_fd_put_ready(MigrationState *s)
>>>          DPRINTF("done iterating\n");
>>>          start_time = qemu_get_clock_ms(rt_clock);
>>>          qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>>> -        vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>>> +        if (old_vm_running) {
>>> +            vm_stop(RUN_STATE_FINISH_MIGRATE);
>>> +        } else {
>>> +            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>>> +        }
>>> 
>>>          if (qemu_savevm_state_complete(s->file) < 0) {
>>>              migrate_fd_error(s);
>
> This hunk also seems to be useless nowadays.

it is needed for when we are migrating in suspended state.  We need to
fix it some other way, but it needs to remaing for now.

Re: [Qemu-devel] [PATCH 06/30] migration: stop all cpus correctly

[Paolo Bonzini]

Il 14/12/2012 13:36, Juan Quintela ha scritto:
>>>> >>> -        vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>>>> >>> +        if (old_vm_running) {
>>>> >>> +            vm_stop(RUN_STATE_FINISH_MIGRATE);
>>>> >>> +        } else {
>>>> >>> +            vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>>>> >>> +        }
>>>> >>> 
>>>> >>>          if (qemu_savevm_state_complete(s->file) < 0) {
>>>> >>>              migrate_fd_error(s);
>> >
>> > This hunk also seems to be useless nowadays.
> it is needed for when we are migrating in suspended state.

That was the case when the line read

    vm_stop(RUN_STATE_FINISH_MIGRATE);

but it reads vm_stop_force_state.  So you're being kind and not forcing
the change if running, but that's not necessary.  Just always using
vm_stop_force_state works now, and would work with the migration thread too.

Paolo