* [PATCH v2 1/2] migration/postcopy: rename postcopy_ram_enable_notify to postcopy_ram_incoming_setup
2019-10-10 1:13 [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy Wei Yang
@ 2019-10-10 1:13 ` Wei Yang
2019-10-10 1:13 ` [PATCH v2 2/2] migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING Wei Yang
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Wei Yang @ 2019-10-10 1:13 UTC (permalink / raw)
To: quintela, dgilbert, peterx; +Cc: qemu-devel, Wei Yang
Function postcopy_ram_incoming_setup and postcopy_ram_incoming_cleanup
is a pair. Rename to make it clear for audience.
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
migration/postcopy-ram.c | 4 ++--
migration/postcopy-ram.h | 2 +-
migration/savevm.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 1f63e65ed7..b24c4a10c2 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1094,7 +1094,7 @@ retry:
return NULL;
}
-int postcopy_ram_enable_notify(MigrationIncomingState *mis)
+int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
{
/* Open the fd for the kernel to give us userfaults */
mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
@@ -1321,7 +1321,7 @@ int postcopy_request_shared_page(struct PostCopyFD *pcfd, RAMBlock *rb,
return -1;
}
-int postcopy_ram_enable_notify(MigrationIncomingState *mis)
+int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
{
assert(0);
return -1;
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index 9c8bd2bae0..d2668cc820 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -20,7 +20,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
* Make all of RAM sensitive to accesses to areas that haven't yet been written
* and wire up anything necessary to deal with it.
*/
-int postcopy_ram_enable_notify(MigrationIncomingState *mis);
+int postcopy_ram_incoming_setup(MigrationIncomingState *mis);
/*
* Initialise postcopy-ram, setting the RAM to a state where we can go into
diff --git a/migration/savevm.c b/migration/savevm.c
index bb9462a54d..78c2965ca4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1865,7 +1865,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
* shouldn't be doing anything yet so don't actually expect requests
*/
if (migrate_postcopy_ram()) {
- if (postcopy_ram_enable_notify(mis)) {
+ if (postcopy_ram_incoming_setup(mis)) {
postcopy_ram_incoming_cleanup(mis);
return -1;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/2] migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING
2019-10-10 1:13 [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy Wei Yang
2019-10-10 1:13 ` [PATCH v2 1/2] migration/postcopy: rename postcopy_ram_enable_notify to postcopy_ram_incoming_setup Wei Yang
@ 2019-10-10 1:13 ` Wei Yang
2019-10-11 10:12 ` Dr. David Alan Gilbert
2019-10-10 2:00 ` [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy Peter Xu
2019-10-11 14:00 ` Dr. David Alan Gilbert
3 siblings, 1 reply; 6+ messages in thread
From: Wei Yang @ 2019-10-10 1:13 UTC (permalink / raw)
To: quintela, dgilbert, peterx; +Cc: qemu-devel, Wei Yang
Currently, we set PostcopyState blindly to RUNNING, even we found the
previous state is not LISTENING. This will lead to a corner case.
First let's look at the code flow:
qemu_loadvm_state_main()
ret = loadvm_process_command()
loadvm_postcopy_handle_run()
return -1;
if (ret < 0) {
if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING)
...
}
>From above snippet, the corner case is loadvm_postcopy_handle_run()
always sets state to RUNNING. And then it checks the previous state. If
the previous state is not LISTENING, it will return -1. But at this
moment, PostcopyState is already been set to RUNNING.
Then ret is checked in qemu_loadvm_state_main(), when it is -1
PostcopyState is checked. Current logic would pause postcopy and retry
if PostcopyState is RUNNING. This is not what we expect, because
postcopy is not active yet.
This patch makes sure state is set to RUNNING only previous state is
LISTENING by checking the state first.
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Suggested by: Peter Xu <peterx@redhat.com>
---
migration/savevm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 78c2965ca4..b9f30a7090 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1934,7 +1934,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
/* After all discards we can start running and asking for pages */
static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
{
- PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
+ PostcopyState ps = postcopy_state_get();
trace_loadvm_postcopy_handle_run();
if (ps != POSTCOPY_INCOMING_LISTENING) {
@@ -1942,6 +1942,7 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
return -1;
}
+ postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
mis->bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, mis);
qemu_bh_schedule(mis->bh);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/2] migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING
2019-10-10 1:13 ` [PATCH v2 2/2] migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING Wei Yang
@ 2019-10-11 10:12 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-11 10:12 UTC (permalink / raw)
To: Wei Yang; +Cc: qemu-devel, peterx, quintela
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Currently, we set PostcopyState blindly to RUNNING, even we found the
> previous state is not LISTENING. This will lead to a corner case.
>
> First let's look at the code flow:
>
> qemu_loadvm_state_main()
> ret = loadvm_process_command()
> loadvm_postcopy_handle_run()
> return -1;
> if (ret < 0) {
> if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING)
> ...
> }
>
> >From above snippet, the corner case is loadvm_postcopy_handle_run()
> always sets state to RUNNING. And then it checks the previous state. If
> the previous state is not LISTENING, it will return -1. But at this
> moment, PostcopyState is already been set to RUNNING.
>
> Then ret is checked in qemu_loadvm_state_main(), when it is -1
> PostcopyState is checked. Current logic would pause postcopy and retry
> if PostcopyState is RUNNING. This is not what we expect, because
> postcopy is not active yet.
>
> This patch makes sure state is set to RUNNING only previous state is
> LISTENING by checking the state first.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> Suggested by: Peter Xu <peterx@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/savevm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 78c2965ca4..b9f30a7090 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1934,7 +1934,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
> /* After all discards we can start running and asking for pages */
> static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> {
> - PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
> + PostcopyState ps = postcopy_state_get();
>
> trace_loadvm_postcopy_handle_run();
> if (ps != POSTCOPY_INCOMING_LISTENING) {
> @@ -1942,6 +1942,7 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> return -1;
> }
>
> + postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
> mis->bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, mis);
> qemu_bh_schedule(mis->bh);
>
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy
2019-10-10 1:13 [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy Wei Yang
2019-10-10 1:13 ` [PATCH v2 1/2] migration/postcopy: rename postcopy_ram_enable_notify to postcopy_ram_incoming_setup Wei Yang
2019-10-10 1:13 ` [PATCH v2 2/2] migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING Wei Yang
@ 2019-10-10 2:00 ` Peter Xu
2019-10-11 14:00 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2019-10-10 2:00 UTC (permalink / raw)
To: Wei Yang; +Cc: qemu-devel, dgilbert, quintela
On Thu, Oct 10, 2019 at 09:13:14AM +0800, Wei Yang wrote:
> Refine a function name and handle on corner case related to PostcopyState.
>
> v2:
> * remove one unnecessary patch
> * simplify corner case handling
>
> Wei Yang (2):
> migration/postcopy: rename postcopy_ram_enable_notify to
> postcopy_ram_incoming_setup
> migration/postcopy: check PostcopyState before setting to
> POSTCOPY_INCOMING_RUNNING
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy
2019-10-10 1:13 [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy Wei Yang
` (2 preceding siblings ...)
2019-10-10 2:00 ` [PATCH v2 0/2] migration/postcopy: cleanup related to postcopy Peter Xu
@ 2019-10-11 14:00 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-11 14:00 UTC (permalink / raw)
To: Wei Yang; +Cc: qemu-devel, peterx, quintela
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Refine a function name and handle on corner case related to PostcopyState.
>
> v2:
> * remove one unnecessary patch
> * simplify corner case handling
>
> Wei Yang (2):
> migration/postcopy: rename postcopy_ram_enable_notify to
> postcopy_ram_incoming_setup
> migration/postcopy: check PostcopyState before setting to
> POSTCOPY_INCOMING_RUNNING
>
> migration/postcopy-ram.c | 4 ++--
> migration/postcopy-ram.h | 2 +-
> migration/savevm.c | 5 +++--
> 3 files changed, 6 insertions(+), 5 deletions(-)
Queued
>
> --
> 2.17.1
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread