From 1ffb5dfd35888cd9de78cc97d3e3e3cb1f3c4887 Mon Sep 17 00:00:00 2001 From: Chao Fan Date: Tue, 14 Mar 2017 09:55:07 +0800 Subject: [PATCH 1/6] Change the method to calculate dirty-pages-rate 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 Signed-off-by: Li Zhijian Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- 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 cd432e73ae..b05dc84ab9 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 719425b9b8..de1e0a3b18 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 */ From 4af245dc3e6e5c96405b3edb9d75657504256469 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 15 Mar 2017 16:16:03 +0000 Subject: [PATCH 2/6] migration: use "" as the default for tls-creds/hostname 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 Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Eric Blake Signed-off-by: Juan Quintela --- 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 3dab6845b1..54060f749a 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -110,6 +110,8 @@ MigrationState *migrate_get_current(void) if (!once) { qemu_mutex_init(¤t_migration.src_page_req_mutex); + current_migration.parameters.tls_creds = g_strdup(""); + current_migration.parameters.tls_hostname = g_strdup(""); once = true; } return ¤t_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 203c11d025..45bec44ca4 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 32b4a4b782..eb9bf67bd9 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) From 1cf6aa74b32f18f9b5faf525a93b0c1f609acb1f Mon Sep 17 00:00:00 2001 From: Lidong Chen Date: Wed, 15 Mar 2017 11:37:33 +0800 Subject: [PATCH 3/6] migration/block: Avoid invoking blk_drain too frequently 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 Reviewed-by: Fam Zheng Signed-off-by: Juan Quintela --- migration/block.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/migration/block.c b/migration/block.c index 6741228200..7734ff728a 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; From e1e686c1fad6f4c4f7c98565c130526f64e7f02c Mon Sep 17 00:00:00 2001 From: QingFeng Hao Date: Fri, 10 Mar 2017 05:44:02 +0100 Subject: [PATCH 4/6] vmstate: fix failed iotests case 68 and 91 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 Signed-off-by: Halil Pasic Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- migration/vmstate.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/migration/vmstate.c b/migration/vmstate.c index 78b3cd48e7..7b4a607c51 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); From 463a4ac23bcf0f0b65c850fa66f5ae6e43edd243 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Tue, 7 Mar 2017 18:36:36 +0000 Subject: [PATCH 5/6] RAMBlocks: qemu_ram_is_shared Provide a helper to say whether a RAMBlock was created as a shared mapping. Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- exec.c | 5 +++++ include/exec/cpu-common.h | 1 + 2 files changed, 6 insertions(+) diff --git a/exec.c b/exec.c index a22f5a0385..e57a8a2178 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 b62f0d82e4..4d45a72ea9 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); From 8679638b0e231f97ad3456c681bf569ca7f7f6d5 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Tue, 7 Mar 2017 18:36:37 +0000 Subject: [PATCH 6/6] postcopy: Check for shared memory 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 Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- migration/postcopy-ram.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index effbeb64fb..dc80dbb67f 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.