[Qemu-devel] [PULL 0/6] Migration fixes

[Qemu-devel] [PULL 0/6] Migration fixes

[Juan Quintela]

Hi

This are this week fixes for Migration:
- fix calculation of dirty-pages-rate (it was being too optimistic)
- Make sure we can remove tls params for migration (Dan)
- Disable postcopy with shared memory (Dave)
- migration-block: Don't call blk_drain too much (Lidong)
- Fix failed iotests cases (QingFeng)

Please apply, Juan.

The following changes since commit 1883ff34b540daacae948f493b0ba525edf5f642:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2017-03-15 18:44:05 +0000)

are available in the git repository at:

  git://github.com/juanquintela/qemu.git tags/migration/20170316

for you to fetch changes up to 8679638b0e231f97ad3456c681bf569ca7f7f6d5:

  postcopy: Check for shared memory (2017-03-16 09:02:26 +0100)

----------------------------------------------------------------
migration/next for 20170316

----------------------------------------------------------------
Chao Fan (1):
      Change the method to calculate dirty-pages-rate

Daniel P. Berrange (1):
      migration: use "" as the default for tls-creds/hostname

Dr. David Alan Gilbert (2):
      RAMBlocks: qemu_ram_is_shared
      postcopy: Check for shared memory

Lidong Chen (1):
      migration/block: Avoid invoking blk_drain too frequently

QingFeng Hao (1):
      vmstate: fix failed iotests case 68 and 91

 exec.c                    |  5 +++++
 include/exec/cpu-common.h |  1 +
 include/exec/ram_addr.h   |  5 ++++-
 migration/block.c         |  3 +++
 migration/migration.c     |  4 ++++
 migration/postcopy-ram.c  | 18 ++++++++++++++++++
 migration/ram.c           | 12 +++++-------
 migration/tls.c           |  2 +-
 migration/vmstate.c       |  8 ++++----
 qapi-schema.json          |  4 ++++
 10 files changed, 49 insertions(+), 13 deletions(-)

[Qemu-devel] [PULL 1/6] Change the method to calculate dirty-pages-rate

[Juan Quintela]

From: Chao Fan <fanc.fnst@cn.fujitsu.com>

In function cpu_physical_memory_sync_dirty_bitmap, file
include/exec/ram_addr.h:

if (src[idx][offset]) {
    unsigned long bits = atomic_xchg(&src[idx][offset], 0);
    unsigned long new_dirty;
    new_dirty = ~dest[k];
    dest[k] |= bits;
    new_dirty &= bits;
    num_dirty += ctpopl(new_dirty);
}

After these codes executed, only the pages not dirtied in bitmap(dest),
but dirtied in dirty_memory[DIRTY_MEMORY_MIGRATION] will be calculated.
For example:
When ram_list.dirty_memory[DIRTY_MEMORY_MIGRATION] = 0b00001111,
and atomic_rcu_read(&migration_bitmap_rcu)->bmap = 0b00000011,
the new_dirty will be 0b00001100, and this function will return 2 but not
4 which is expected.
the dirty pages in dirty_memory[DIRTY_MEMORY_MIGRATION] are all new,
so these should be calculated also.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/exec/ram_addr.h |  5 ++++-
 migration/ram.c         | 12 +++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index cd432e7..b05dc84 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -355,7 +355,8 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
 static inline
 uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
                                                ram_addr_t start,
-                                               ram_addr_t length)
+                                               ram_addr_t length,
+                                               int64_t *real_dirty_pages)
 {
     ram_addr_t addr;
     unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
@@ -379,6 +380,7 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
             if (src[idx][offset]) {
                 unsigned long bits = atomic_xchg(&src[idx][offset], 0);
                 unsigned long new_dirty;
+                *real_dirty_pages += ctpopl(bits);
                 new_dirty = ~dest[k];
                 dest[k] |= bits;
                 new_dirty &= bits;
@@ -398,6 +400,7 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
                         start + addr,
                         TARGET_PAGE_SIZE,
                         DIRTY_MEMORY_MIGRATION)) {
+                *real_dirty_pages += 1;
                 long k = (start + addr) >> TARGET_PAGE_BITS;
                 if (!test_and_set_bit(k, dest)) {
                     num_dirty++;
diff --git a/migration/ram.c b/migration/ram.c
index 719425b..de1e0a3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -576,18 +576,18 @@ static inline bool migration_bitmap_clear_dirty(ram_addr_t addr)
     return ret;
 }
 
+static int64_t num_dirty_pages_period;
 static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
 {
     unsigned long *bitmap;
     bitmap = atomic_rcu_read(&migration_bitmap_rcu)->bmap;
-    migration_dirty_pages +=
-        cpu_physical_memory_sync_dirty_bitmap(bitmap, start, length);
+    migration_dirty_pages += cpu_physical_memory_sync_dirty_bitmap(bitmap,
+                             start, length, &num_dirty_pages_period);
 }
 
 /* Fix me: there are too many global variables used in migration process. */
 static int64_t start_time;
 static int64_t bytes_xfer_prev;
-static int64_t num_dirty_pages_period;
 static uint64_t xbzrle_cache_miss_prev;
 static uint64_t iterations_prev;
 
@@ -620,7 +620,6 @@ uint64_t ram_pagesize_summary(void)
 static void migration_bitmap_sync(void)
 {
     RAMBlock *block;
-    uint64_t num_dirty_pages_init = migration_dirty_pages;
     MigrationState *s = migrate_get_current();
     int64_t end_time;
     int64_t bytes_xfer_now;
@@ -646,9 +645,8 @@ static void migration_bitmap_sync(void)
     rcu_read_unlock();
     qemu_mutex_unlock(&migration_bitmap_mutex);
 
-    trace_migration_bitmap_sync_end(migration_dirty_pages
-                                    - num_dirty_pages_init);
-    num_dirty_pages_period += migration_dirty_pages - num_dirty_pages_init;
+    trace_migration_bitmap_sync_end(num_dirty_pages_period);
+
     end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
 
     /* more than 1 second = 1000 millisecons */
-- 
2.9.3

[Qemu-devel] [PULL 2/6] migration: use "" as the default for tls-creds/hostname

[Juan Quintela]

From: "Daniel P. Berrange" <berrange@redhat.com>

The tls-creds parameter has a default value of NULL indicating
that TLS should not be used. Setting it to non-NULL enables
use of TLS. Once tls-creds are set to a non-NULL value via the
monitor, it isn't possible to set them back to NULL again, due
to current implementation limitations. The empty string is not
a valid QObject identifier, so this switches to use "" as the
default, indicating that TLS will not be used

The tls-hostname parameter has a default value of NULL indicating
the the hostname from the migrate connection URI should be used.
Again, once tls-hostname is set non-NULL, to override the default
hostname for x509 cert validation, it isn't possible to reset it
back to NULL via the monitor. The empty string is not a valid
hostname, so this switches to use "" as the default, indicating
that the migrate URI hostname should be used.

Using "" as the default for both, also means that the monitor
commands "info migrate_parameters" / "query-migrate-parameters"
will report existance of tls-creds/tls-parameters even when set
to their default values.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 4 ++++
 migration/tls.c       | 2 +-
 qapi-schema.json      | 4 ++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3dab684..54060f7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,6 +110,8 @@ MigrationState *migrate_get_current(void)
 
     if (!once) {
         qemu_mutex_init(&current_migration.src_page_req_mutex);
+        current_migration.parameters.tls_creds = g_strdup("");
+        current_migration.parameters.tls_hostname = g_strdup("");
         once = true;
     }
     return &current_migration;
@@ -458,6 +460,7 @@ void migration_channel_process_incoming(MigrationState *s,
         ioc, object_get_typename(OBJECT(ioc)));
 
     if (s->parameters.tls_creds &&
+        *s->parameters.tls_creds &&
         !object_dynamic_cast(OBJECT(ioc),
                              TYPE_QIO_CHANNEL_TLS)) {
         Error *local_err = NULL;
@@ -480,6 +483,7 @@ void migration_channel_connect(MigrationState *s,
         ioc, object_get_typename(OBJECT(ioc)), hostname);
 
     if (s->parameters.tls_creds &&
+        *s->parameters.tls_creds &&
         !object_dynamic_cast(OBJECT(ioc),
                              TYPE_QIO_CHANNEL_TLS)) {
         Error *local_err = NULL;
diff --git a/migration/tls.c b/migration/tls.c
index 203c11d..45bec44 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -141,7 +141,7 @@ void migration_tls_channel_connect(MigrationState *s,
         return;
     }
 
-    if (s->parameters.tls_hostname) {
+    if (s->parameters.tls_hostname && *s->parameters.tls_hostname) {
         hostname = s->parameters.tls_hostname;
     }
     if (!hostname) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 32b4a4b..eb9bf67 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1036,6 +1036,8 @@
 #             credentials must be for a 'server' endpoint. Setting this
 #             will enable TLS for all migrations. The default is unset,
 #             resulting in unsecured migration at the QEMU level. (Since 2.7)
+#             An empty string means that QEMU will use plain text mode for
+#             migration, rather than TLS (Since 2.9)
 #
 # @tls-hostname: #optional hostname of the target host for the migration. This
 #                is required when using x509 based TLS credentials and the
@@ -1043,6 +1045,8 @@
 #                example if using fd: or exec: based migration, the
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
+#                An empty string means that QEMU will use the hostname
+#                associated with the migration URI, if any. (Since 2.9)
 #
 # @max-bandwidth: to set maximum speed for migration. maximum speed in
 #                 bytes per second. (Since 2.8)
-- 
2.9.3

[Qemu-devel] [PULL 3/6] migration/block: Avoid invoking blk_drain too frequently

[Juan Quintela]

From: Lidong Chen <jemmy858585@gmail.com>

Increase bmds->cur_dirty after submit io, so reduce the frequency
involve into blk_drain, and improve the performance obviously
when block migration.

The performance test result of this patch:

During the block dirty save phase, this patch improve guest os IOPS
from 4.0K to 9.5K. and improve the migration speed from
505856 rsec/s to 855756 rsec/s.

Signed-off-by: Lidong Chen <jemmy858585@gmail.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/block.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/block.c b/migration/block.c
index 6741228..7734ff7 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -576,6 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
             }
 
             bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
+            sector += nr_sectors;
+            bmds->cur_dirty = sector;
+
             break;
         }
         sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
-- 
2.9.3

[Qemu-devel] [PULL 4/6] vmstate: fix failed iotests case 68 and 91

[Juan Quintela]

From: QingFeng Hao <haoqf@linux.vnet.ibm.com>

This problem affects s390x only if we are running without KVM.
Basically, S390CPU.irqstate is unused if we do not use KVM,
and thus no buffer is allocated.
This causes size=0, first_elem=NULL and n_elems=1 in
vmstate_load_state and vmstate_save_state. And the assert fails.
With this fix we can go back to the old behavior and support
VMS_VBUFFER with size 0 and nullptr.

Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/vmstate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..7b4a607 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -109,7 +109,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             vmstate_handle_alloc(first_elem, field, opaque);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
-                assert(first_elem  || !n_elems);
+                assert(first_elem || !n_elems || !size);
             }
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
@@ -117,7 +117,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     curr_elem = *(void **)curr_elem;
                 }
-                if (!curr_elem) {
+                if (!curr_elem && size) {
                     /* if null pointer check placeholder and do not follow */
                     assert(field->flags & VMS_ARRAY_OF_POINTER);
                     ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
@@ -325,7 +325,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
-                assert(first_elem  || !n_elems);
+                assert(first_elem || !n_elems || !size);
             }
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
@@ -336,7 +336,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
                 }
-                if (!curr_elem) {
+                if (!curr_elem && size) {
                     /* if null pointer write placeholder and do not follow */
                     assert(field->flags & VMS_ARRAY_OF_POINTER);
                     vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
-- 
2.9.3

[Qemu-devel] [PULL 5/6] RAMBlocks: qemu_ram_is_shared

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Provide a helper to say whether a RAMBlock was created as a
shared mapping.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 exec.c                    | 5 +++++
 include/exec/cpu-common.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/exec.c b/exec.c
index a22f5a0..e57a8a2 100644
--- a/exec.c
+++ b/exec.c
@@ -1561,6 +1561,11 @@ const char *qemu_ram_get_idstr(RAMBlock *rb)
     return rb->idstr;
 }
 
+bool qemu_ram_is_shared(RAMBlock *rb)
+{
+    return rb->flags & RAM_SHARED;
+}
+
 /* Called with iothread lock held.  */
 void qemu_ram_set_idstr(RAMBlock *new_block, const char *name, DeviceState *dev)
 {
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index b62f0d8..4d45a72 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -69,6 +69,7 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
 void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState *dev);
 void qemu_ram_unset_idstr(RAMBlock *block);
 const char *qemu_ram_get_idstr(RAMBlock *rb);
+bool qemu_ram_is_shared(RAMBlock *rb);
 size_t qemu_ram_pagesize(RAMBlock *block);
 size_t qemu_ram_pagesize_largest(void);
 
-- 
2.9.3

[Qemu-devel] [PULL 6/6] postcopy: Check for shared memory

[Juan Quintela]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Postcopy doesn't support migration of RAM shared with another process
yet (we've got a bunch of things to understand).
Check for the case and don't allow postcopy to be enabled.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/postcopy-ram.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index effbeb6..dc80dbb 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)
     return true;
 }
 
+/* Callback from postcopy_ram_supported_by_host block iterator.
+ */
+static int test_range_shared(const char *block_name, void *host_addr,
+                             ram_addr_t offset, ram_addr_t length, void *opaque)
+{
+    if (qemu_ram_is_shared(qemu_ram_block_by_name(block_name))) {
+        error_report("Postcopy on shared RAM (%s) is not yet supported",
+                     block_name);
+        return 1;
+    }
+    return 0;
+}
+
 /*
  * Note: This has the side effect of munlock'ing all of RAM, that's
  * normally fine since if the postcopy succeeds it gets turned back on at the
@@ -127,6 +140,11 @@ bool postcopy_ram_supported_by_host(void)
         goto out;
     }
 
+    /* We don't support postcopy with shared RAM yet */
+    if (qemu_ram_foreach_block(test_range_shared, NULL)) {
+        goto out;
+    }
+
     /*
      * userfault and mlock don't go together; we'll put it back later if
      * it was enabled.
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/6] Migration fixes

[Peter Maydell]

On 16 March 2017 at 09:01, Juan Quintela <quintela@redhat.com> wrote:
> Hi
>
> This are this week fixes for Migration:
> - fix calculation of dirty-pages-rate (it was being too optimistic)
> - Make sure we can remove tls params for migration (Dan)
> - Disable postcopy with shared memory (Dave)
> - migration-block: Don't call blk_drain too much (Lidong)
> - Fix failed iotests cases (QingFeng)
>
> Please apply, Juan.
>
> The following changes since commit 1883ff34b540daacae948f493b0ba525edf5f642:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2017-03-15 18:44:05 +0000)
>
> are available in the git repository at:
>
>   git://github.com/juanquintela/qemu.git tags/migration/20170316
>
> for you to fetch changes up to 8679638b0e231f97ad3456c681bf569ca7f7f6d5:
>
>   postcopy: Check for shared memory (2017-03-16 09:02:26 +0100)
>
> ----------------------------------------------------------------
> migration/next for 20170316
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM