Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
On 11/26/2013 10:11 PM, Paolo Bonzini wrote: Il 26/11/2013 14:53, Lei Li ha scritto: 1) ram_save_setup stage, it will send all the bytes in this stages to destination, and send_pipefd by ram_control_before_iterate at the end of it. ram_save_setup runs doesn't send anything from guest RAM. It sends the lengths of the various blocks. As you said, at the end of ram_save_setup you send the pipefd. ram_save_iterate runs before ram_save_complete. ram_save_iterate and ram_save_complete write data with exactly the same format. Both of them can use ram_save_page It should not matter if some pages are sent as part of ram_save_iterate and others as part of ram_save_complete. One possibility is that you are hitting a bug due to the way you ignore the 0x01 byte that send_pipefd places on the socket. Oops. I might have said this before thinking about postcopy and/or before seeing the benchmark results from Juan's patches. If this part of the patch is just an optimization, I'd rather leave it out for now. I am afraid that page flipping can not proceed correctly without this.. I really would like to understand why, because it really shouldn't (this shouldn't be a place where you need a hook). Hi Paolo, Sorry for the late reply. Yes, you are right!! I just have a try with this adjustment removed, it works well... I remembered that it can not proceed correctly when debugging in previous version without this as in theory it should like your explanation above. I guess the only answer is that there was a bug regarding the one byte fd control message just like the possibility you listed! Paolo -- Lei
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
Il 21/11/2013 10:11, Lei Li ha scritto:
Signed-off-by: Lei Li li...@linux.vnet.ibm.com
---
migration.c | 10 +++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/migration.c b/migration.c
index 4ac466b..0f98ac1 100644
--- a/migration.c
+++ b/migration.c
@@ -579,10 +579,11 @@ static void *migration_thread(void *opaque)
pending_size = qemu_savevm_state_pending(s-file, max_size);
DPRINTF(pending size % PRIu64 max % PRIu64 \n,
pending_size, max_size);
-if (pending_size pending_size = max_size) {
+if (pending_size pending_size = max_size
+!runstate_needs_reset()) {
qemu_savevm_state_iterate(s-file);
I'm not sure why you need this.
} else {
-int ret;
+int ret = 0;
DPRINTF(done iterating\n);
qemu_mutex_lock_iothread();
@@ -590,7 +591,10 @@ static void *migration_thread(void *opaque)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
old_vm_running = runstate_is_running();
-ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+if (!runstate_needs_reset()) {
+ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+}
This however is okay.
Paolo
if (ret = 0) {
qemu_file_set_rate_limit(s-file, INT_MAX);
qemu_savevm_state_complete(s-file);
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
On 11/26/2013 07:32 PM, Paolo Bonzini wrote:
Il 21/11/2013 10:11, Lei Li ha scritto:
Signed-off-by: Lei Li li...@linux.vnet.ibm.com
---
migration.c | 10 +++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/migration.c b/migration.c
index 4ac466b..0f98ac1 100644
--- a/migration.c
+++ b/migration.c
@@ -579,10 +579,11 @@ static void *migration_thread(void *opaque)
pending_size = qemu_savevm_state_pending(s-file, max_size);
DPRINTF(pending size % PRIu64 max % PRIu64 \n,
pending_size, max_size);
-if (pending_size pending_size = max_size) {
+if (pending_size pending_size = max_size
+!runstate_needs_reset()) {
qemu_savevm_state_iterate(s-file);
I'm not sure why you need this.
The adjustment here is to avoid the iteration stage for page flipping.
Because pending_size = ram_save_remaining() * TARGET_PAGE_SIZE which is
not 0 and pending_size max_size (0) at start.
In the previous version it was like this:
if (pending_size pending_size = max_size
!migrate_unix_page_flipping()) {
And you said 'This is a bit ugly but I understand the need. Perhaps
!runstate_needs_reset() like below?' :)
} else {
-int ret;
+int ret = 0;
DPRINTF(done iterating\n);
qemu_mutex_lock_iothread();
@@ -590,7 +591,10 @@ static void *migration_thread(void *opaque)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
old_vm_running = runstate_is_running();
-ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+if (!runstate_needs_reset()) {
+ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+}
This however is okay.
Paolo
if (ret = 0) {
qemu_file_set_rate_limit(s-file, INT_MAX);
qemu_savevm_state_complete(s-file);
--
Lei
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
Il 26/11/2013 13:03, Lei Li ha scritto:
+if (pending_size pending_size = max_size
+!runstate_needs_reset()) {
qemu_savevm_state_iterate(s-file);
I'm not sure why you need this.
The adjustment here is to avoid the iteration stage for page flipping.
Because pending_size = ram_save_remaining() * TARGET_PAGE_SIZE which is
not 0 and pending_size max_size (0) at start.
It's still not clear to me that avoiding the iteration stage is
necessary. I think it's just an optimization to avoid scanning the
bitmap, but:
(1) Juan's bitmap optimization will make this mostly unnecessary
(2) getting good downtime from page flipping will require postcopy anyway.
And you said 'This is a bit ugly but I understand the need. Perhaps
!runstate_needs_reset() like below?' :)
Oops. I might have said this before thinking about postcopy and/or
before seeing the benchmark results from Juan's patches. If this part
of the patch is just an optimization, I'd rather leave it out for now.
Thanks for putting up with me. :)
Paolo
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
On 11/26/2013 08:54 PM, Paolo Bonzini wrote:
Il 26/11/2013 13:03, Lei Li ha scritto:
+if (pending_size pending_size = max_size
+!runstate_needs_reset()) {
qemu_savevm_state_iterate(s-file);
I'm not sure why you need this.
The adjustment here is to avoid the iteration stage for page flipping.
Because pending_size = ram_save_remaining() * TARGET_PAGE_SIZE which is
not 0 and pending_size max_size (0) at start.
It's still not clear to me that avoiding the iteration stage is
The purpose of it is not just for optimization, but to avoid the
iteration for better alignment.
The current flow of page flipping basically has two stages:
1) ram_save_setup stage, it will send all the bytes in this stages
to destination, and send_pipefd by ram_control_before_iterate
at the end of it.
2) ram_save_complete, it will start to transmit the ram page
in ram_save_block, and send the device state after that.
So it needs to adjust the current migration process to avoid
the iteration stage.
necessary. I think it's just an optimization to avoid scanning the
bitmap, but:
(1) Juan's bitmap optimization will make this mostly unnecessary
(2) getting good downtime from page flipping will require postcopy anyway.
And you said 'This is a bit ugly but I understand the need. Perhaps
!runstate_needs_reset() like below?' :)
Oops. I might have said this before thinking about postcopy and/or
before seeing the benchmark results from Juan's patches. If this part
of the patch is just an optimization, I'd rather leave it out for now.
I am afraid that page flipping can not proceed correctly without this..
Thanks for putting up with me. :)
Paolo
--
Lei
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
Il 26/11/2013 14:53, Lei Li ha scritto: 1) ram_save_setup stage, it will send all the bytes in this stages to destination, and send_pipefd by ram_control_before_iterate at the end of it. ram_save_setup runs doesn't send anything from guest RAM. It sends the lengths of the various blocks. As you said, at the end of ram_save_setup you send the pipefd. ram_save_iterate runs before ram_save_complete. ram_save_iterate and ram_save_complete write data with exactly the same format. Both of them can use ram_save_page It should not matter if some pages are sent as part of ram_save_iterate and others as part of ram_save_complete. One possibility is that you are hitting a bug due to the way you ignore the 0x01 byte that send_pipefd places on the socket. Oops. I might have said this before thinking about postcopy and/or before seeing the benchmark results from Juan's patches. If this part of the patch is just an optimization, I'd rather leave it out for now. I am afraid that page flipping can not proceed correctly without this.. I really would like to understand why, because it really shouldn't (this shouldn't be a place where you need a hook). Paolo
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
Il 22/10/2013 04:25, Lei Li ha scritto:
Signed-off-by: Lei Li li...@linux.vnet.ibm.com
---
migration.c | 10 +++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/migration.c b/migration.c
index 4ac466b..568b73a 100644
--- a/migration.c
+++ b/migration.c
@@ -579,10 +579,11 @@ static void *migration_thread(void *opaque)
pending_size = qemu_savevm_state_pending(s-file, max_size);
DPRINTF(pending size % PRIu64 max % PRIu64 \n,
pending_size, max_size);
-if (pending_size pending_size = max_size) {
+if (pending_size pending_size = max_size
+!migrate_unix_page_flipping()) {
This is a bit ugly but I understand the need. Perhaps
!runstate_needs_reset() like below?
Paolo
qemu_savevm_state_iterate(s-file);
} else {
-int ret;
+int ret = 0;
DPRINTF(done iterating\n);
qemu_mutex_lock_iothread();
@@ -590,7 +591,10 @@ static void *migration_thread(void *opaque)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
old_vm_running = runstate_is_running();
-ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+if (!runstate_needs_reset()) {
+ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+}
+
if (ret = 0) {
qemu_file_set_rate_limit(s-file, INT_MAX);
qemu_savevm_state_complete(s-file);
Re: [Qemu-devel] [PATCH 16/17] migration: adjust migration_thread() process for page flipping
On 10/24/2013 10:15 PM, Paolo Bonzini wrote:
Il 22/10/2013 04:25, Lei Li ha scritto:
Signed-off-by: Lei Li li...@linux.vnet.ibm.com
---
migration.c | 10 +++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/migration.c b/migration.c
index 4ac466b..568b73a 100644
--- a/migration.c
+++ b/migration.c
@@ -579,10 +579,11 @@ static void *migration_thread(void *opaque)
pending_size = qemu_savevm_state_pending(s-file, max_size);
DPRINTF(pending size % PRIu64 max % PRIu64 \n,
pending_size, max_size);
-if (pending_size pending_size = max_size) {
+if (pending_size pending_size = max_size
+!migrate_unix_page_flipping()) {
This is a bit ugly but I understand the need. Perhaps
!runstate_needs_reset() like below?
' !runstate_needs_reset()' is fine, thanks.
Paolo
qemu_savevm_state_iterate(s-file);
} else {
-int ret;
+int ret = 0;
DPRINTF(done iterating\n);
qemu_mutex_lock_iothread();
@@ -590,7 +591,10 @@ static void *migration_thread(void *opaque)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
old_vm_running = runstate_is_running();
-ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+if (!runstate_needs_reset()) {
+ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+}
+
if (ret = 0) {
qemu_file_set_rate_limit(s-file, INT_MAX);
qemu_savevm_state_complete(s-file);
--
Lei
