[PATCH 0/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[PATCH 0/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Wei Yang]

have_listen_thread is used to be a indication of whether postcopy thread is
running. Since we use PostcopyState to record state and the postcopy thread
only runs when postcopy_is_running(), we can leverage the PostcopyState to
replace the meaning of have_listen_thread.

To do so, two preparation cleanup is included.

Wei Yang (3):
  migration/postcopy: mis->have_listen_thread check will never be
    touched
  migration/postcopy: postpone setting PostcopyState to END
  migration/postcopy: replace have_listen_thread check with
    PostcopyState check

 migration/migration.h    |  1 -
 migration/postcopy-ram.c |  2 --
 migration/ram.c          |  2 +-
 migration/ram.h          |  1 +
 migration/savevm.c       | 11 +++--------
 5 files changed, 5 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/3] migration/postcopy: mis->have_listen_thread check will never be touched

[Wei Yang]

If mis->have_listen_thread is true, this means current PostcopyState
must be LISTENING or RUNNING. While the check at the beginning of the
function makes sure the state transaction happens when its previous
PostcopyState is ADVISE or DISCARD.

This means we would never touch this check.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/savevm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index feb757de79..eaa4cf58ef 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1878,11 +1878,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
         return -1;
     }
 
-    if (mis->have_listen_thread) {
-        error_report("CMD_POSTCOPY_RAM_LISTEN already has a listen thread");
-        return -1;
-    }
-
     mis->have_listen_thread = true;
     /* Start up the listening thread and wait for it to signal ready */
     qemu_sem_init(&mis->listen_thread_sem, 0);
-- 
2.17.1

Re: [PATCH 1/3] migration/postcopy: mis->have_listen_thread check will never be touched

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> If mis->have_listen_thread is true, this means current PostcopyState
> must be LISTENING or RUNNING. While the check at the beginning of the
> function makes sure the state transaction happens when its previous
> PostcopyState is ADVISE or DISCARD.
> 
> This means we would never touch this check.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Yes, OK - although I think it's OK sometimes to leave in checks
for things you don't expect to happen!


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index feb757de79..eaa4cf58ef 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1878,11 +1878,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>          return -1;
>      }
>  
> -    if (mis->have_listen_thread) {
> -        error_report("CMD_POSTCOPY_RAM_LISTEN already has a listen thread");
> -        return -1;
> -    }
> -
>      mis->have_listen_thread = true;
>      /* Start up the listening thread and wait for it to signal ready */
>      qemu_sem_init(&mis->listen_thread_sem, 0);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/3] migration/postcopy: postpone setting PostcopyState to END

[Wei Yang]

There are two places to call function postcopy_ram_incoming_cleanup()

    postcopy_ram_listen_thread on migration success
    loadvm_postcopy_handle_listen one setup failure

On success, the vm will never accept another migration. On failure,
PostcopyState is transited from LISTENING to END and would be checked in
qemu_loadvm_state_main(). If PostcopyState is RUNNING, migration would
be paused and retried.

Currently PostcopyState is set to END in function
postcopy_ram_incoming_cleanup(). With above analysis, we can take this
step out and postpone this till the end of listen thread to indicate the
listen thread is done.

This is a preparation patch for later cleanup.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/postcopy-ram.c | 2 --
 migration/savevm.c       | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a394c7c3a6..5da6de8c8b 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -577,8 +577,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
         }
     }
 
-    postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
-
     if (mis->postcopy_tmp_page) {
         munmap(mis->postcopy_tmp_page, mis->largest_page_size);
         mis->postcopy_tmp_page = NULL;
diff --git a/migration/savevm.c b/migration/savevm.c
index eaa4cf58ef..dcad8897a3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1837,6 +1837,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
 
     rcu_unregister_thread();
     mis->have_listen_thread = false;
+    postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
+
     return NULL;
 }
 
-- 
2.17.1

Re: [PATCH 2/3] migration/postcopy: postpone setting PostcopyState to END

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> There are two places to call function postcopy_ram_incoming_cleanup()
> 
>     postcopy_ram_listen_thread on migration success
>     loadvm_postcopy_handle_listen one setup failure
> 
> On success, the vm will never accept another migration. On failure,
> PostcopyState is transited from LISTENING to END and would be checked in
> qemu_loadvm_state_main(). If PostcopyState is RUNNING, migration would
> be paused and retried.
> 
> Currently PostcopyState is set to END in function
> postcopy_ram_incoming_cleanup(). With above analysis, we can take this
> step out and postpone this till the end of listen thread to indicate the
> listen thread is done.
> 
> This is a preparation patch for later cleanup.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Yes, I think that's OK - I couldn't see anywhere that's currently
checking the state in between.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/postcopy-ram.c | 2 --
>  migration/savevm.c       | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a394c7c3a6..5da6de8c8b 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -577,8 +577,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>          }
>      }
>  
> -    postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
> -
>      if (mis->postcopy_tmp_page) {
>          munmap(mis->postcopy_tmp_page, mis->largest_page_size);
>          mis->postcopy_tmp_page = NULL;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index eaa4cf58ef..dcad8897a3 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1837,6 +1837,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
>  
>      rcu_unregister_thread();
>      mis->have_listen_thread = false;
> +    postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
> +
>      return NULL;
>  }
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Wei Yang]

After previous cleanup, postcopy thread is running only when
PostcopyState is LISTENNING or RUNNING. This means it is not necessary
to spare a variable have_listen_thread to represent the state.

Replace the check on have_listen_thread with PostcopyState and remove
the variable.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/migration.h | 1 -
 migration/ram.c       | 2 +-
 migration/ram.h       | 1 +
 migration/savevm.c    | 4 +---
 4 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 4f2fe193dc..a4d639663d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -63,7 +63,6 @@ struct MigrationIncomingState {
     /* Set this when we want the fault thread to quit */
     bool           fault_thread_quit;
 
-    bool           have_listen_thread;
     QemuThread     listen_thread;
     QemuSemaphore  listen_thread_sem;
 
diff --git a/migration/ram.c b/migration/ram.c
index 769d3f6454..dfc50d57d5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
     return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
 }
 
-static bool postcopy_is_running(void)
+bool postcopy_is_running(void)
 {
     PostcopyState ps = postcopy_state_get();
     return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
diff --git a/migration/ram.h b/migration/ram.h
index bd0eee79b6..44fe4753ad 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
 /* For incoming postcopy discard */
 int ram_discard_range(const char *block_name, uint64_t start, size_t length);
 int ram_postcopy_incoming_init(MigrationIncomingState *mis);
+bool postcopy_is_running(void);
 
 void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index dcad8897a3..2a0e0b94df 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
     qemu_loadvm_state_cleanup();
 
     rcu_unregister_thread();
-    mis->have_listen_thread = false;
     postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
 
     return NULL;
@@ -1880,7 +1879,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
         return -1;
     }
 
-    mis->have_listen_thread = true;
     /* Start up the listening thread and wait for it to signal ready */
     qemu_sem_init(&mis->listen_thread_sem, 0);
     qemu_thread_create(&mis->listen_thread, "postcopy/listen",
@@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
 
     trace_qemu_loadvm_state_post_main(ret);
 
-    if (mis->have_listen_thread) {
+    if (postcopy_is_running()) {
         /* Listen thread still going, can't clean up yet */
         return ret;
     }
-- 
2.17.1

Re: [PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> After previous cleanup, postcopy thread is running only when
> PostcopyState is LISTENNING or RUNNING. This means it is not necessary
> to spare a variable have_listen_thread to represent the state.
> 
> Replace the check on have_listen_thread with PostcopyState and remove
> the variable.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/migration.h | 1 -
>  migration/ram.c       | 2 +-
>  migration/ram.h       | 1 +
>  migration/savevm.c    | 4 +---
>  4 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 4f2fe193dc..a4d639663d 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -63,7 +63,6 @@ struct MigrationIncomingState {
>      /* Set this when we want the fault thread to quit */
>      bool           fault_thread_quit;
>  
> -    bool           have_listen_thread;
>      QemuThread     listen_thread;
>      QemuSemaphore  listen_thread_sem;
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 769d3f6454..dfc50d57d5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
>      return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
>  }
>  
> -static bool postcopy_is_running(void)
> +bool postcopy_is_running(void)
>  {
>      PostcopyState ps = postcopy_state_get();
>      return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
> diff --git a/migration/ram.h b/migration/ram.h
> index bd0eee79b6..44fe4753ad 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
>  /* For incoming postcopy discard */
>  int ram_discard_range(const char *block_name, uint64_t start, size_t length);
>  int ram_postcopy_incoming_init(MigrationIncomingState *mis);
> +bool postcopy_is_running(void);
>  
>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index dcad8897a3..2a0e0b94df 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>      qemu_loadvm_state_cleanup();
>  
>      rcu_unregister_thread();
> -    mis->have_listen_thread = false;
>      postcopy_state_set(POSTCOPY_INCOMING_END, NULL);

That now needs a big comment saying it must be the last thing in the
thread, because now it's got meaning that it's here.

>  
>      return NULL;
> @@ -1880,7 +1879,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>          return -1;
>      }
>  
> -    mis->have_listen_thread = true;
>      /* Start up the listening thread and wait for it to signal ready */
>      qemu_sem_init(&mis->listen_thread_sem, 0);
>      qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> @@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
>  
>      trace_qemu_loadvm_state_post_main(ret);
>  
> -    if (mis->have_listen_thread) {
> +    if (postcopy_is_running()) {
>          /* Listen thread still going, can't clean up yet */
>          return ret;
>      }

Can you explain to me why this is afe in the case of a failure in
loadvm_postcopy_handle_listen between the start where it sets
the state to LISTENING, and the point where it currently sets
hasve_listen_thread ?  Wouldn't this cause qemu_loadvm_state
not to cleanup?

Dave

> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Wei Yang]

On Tue, Oct 08, 2019 at 08:15:51PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> After previous cleanup, postcopy thread is running only when
>> PostcopyState is LISTENNING or RUNNING. This means it is not necessary
>> to spare a variable have_listen_thread to represent the state.
>> 
>> Replace the check on have_listen_thread with PostcopyState and remove
>> the variable.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/migration.h | 1 -
>>  migration/ram.c       | 2 +-
>>  migration/ram.h       | 1 +
>>  migration/savevm.c    | 4 +---
>>  4 files changed, 3 insertions(+), 5 deletions(-)
>> 
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 4f2fe193dc..a4d639663d 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -63,7 +63,6 @@ struct MigrationIncomingState {
>>      /* Set this when we want the fault thread to quit */
>>      bool           fault_thread_quit;
>>  
>> -    bool           have_listen_thread;
>>      QemuThread     listen_thread;
>>      QemuSemaphore  listen_thread_sem;
>>  
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 769d3f6454..dfc50d57d5 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
>>      return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
>>  }
>>  
>> -static bool postcopy_is_running(void)
>> +bool postcopy_is_running(void)
>>  {
>>      PostcopyState ps = postcopy_state_get();
>>      return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
>> diff --git a/migration/ram.h b/migration/ram.h
>> index bd0eee79b6..44fe4753ad 100644
>> --- a/migration/ram.h
>> +++ b/migration/ram.h
>> @@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
>>  /* For incoming postcopy discard */
>>  int ram_discard_range(const char *block_name, uint64_t start, size_t length);
>>  int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>> +bool postcopy_is_running(void);
>>  
>>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>>  
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dcad8897a3..2a0e0b94df 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>>      qemu_loadvm_state_cleanup();
>>  
>>      rcu_unregister_thread();
>> -    mis->have_listen_thread = false;
>>      postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
>
>That now needs a big comment saying it must be the last thing in the
>thread, because now it's got meaning that it's here.
>
>>  
>>      return NULL;
>> @@ -1880,7 +1879,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>>          return -1;
>>      }
>>  
>> -    mis->have_listen_thread = true;
>>      /* Start up the listening thread and wait for it to signal ready */
>>      qemu_sem_init(&mis->listen_thread_sem, 0);
>>      qemu_thread_create(&mis->listen_thread, "postcopy/listen",
>> @@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>  
>>      trace_qemu_loadvm_state_post_main(ret);
>>  
>> -    if (mis->have_listen_thread) {
>> +    if (postcopy_is_running()) {
>>          /* Listen thread still going, can't clean up yet */
>>          return ret;
>>      }
>
>Can you explain to me why this is afe in the case of a failure in
>loadvm_postcopy_handle_listen between the start where it sets
>the state to LISTENING, and the point where it currently sets
>hasve_listen_thread ?  Wouldn't this cause qemu_loadvm_state
>not to cleanup?
>

I have to say you are right.  listen_thread may not started when PostcopyState
is already set to LISTENING.

The ugly fix may be set PostcopyState back to original one. Not sure whether
you would like this. 

>Dave
>
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

Re: [PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Tue, Oct 08, 2019 at 08:15:51PM +0100, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> After previous cleanup, postcopy thread is running only when
> >> PostcopyState is LISTENNING or RUNNING. This means it is not necessary
> >> to spare a variable have_listen_thread to represent the state.
> >> 
> >> Replace the check on have_listen_thread with PostcopyState and remove
> >> the variable.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> ---
> >>  migration/migration.h | 1 -
> >>  migration/ram.c       | 2 +-
> >>  migration/ram.h       | 1 +
> >>  migration/savevm.c    | 4 +---
> >>  4 files changed, 3 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/migration/migration.h b/migration/migration.h
> >> index 4f2fe193dc..a4d639663d 100644
> >> --- a/migration/migration.h
> >> +++ b/migration/migration.h
> >> @@ -63,7 +63,6 @@ struct MigrationIncomingState {
> >>      /* Set this when we want the fault thread to quit */
> >>      bool           fault_thread_quit;
> >>  
> >> -    bool           have_listen_thread;
> >>      QemuThread     listen_thread;
> >>      QemuSemaphore  listen_thread_sem;
> >>  
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 769d3f6454..dfc50d57d5 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
> >>      return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
> >>  }
> >>  
> >> -static bool postcopy_is_running(void)
> >> +bool postcopy_is_running(void)
> >>  {
> >>      PostcopyState ps = postcopy_state_get();
> >>      return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
> >> diff --git a/migration/ram.h b/migration/ram.h
> >> index bd0eee79b6..44fe4753ad 100644
> >> --- a/migration/ram.h
> >> +++ b/migration/ram.h
> >> @@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
> >>  /* For incoming postcopy discard */
> >>  int ram_discard_range(const char *block_name, uint64_t start, size_t length);
> >>  int ram_postcopy_incoming_init(MigrationIncomingState *mis);
> >> +bool postcopy_is_running(void);
> >>  
> >>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> >>  
> >> diff --git a/migration/savevm.c b/migration/savevm.c
> >> index dcad8897a3..2a0e0b94df 100644
> >> --- a/migration/savevm.c
> >> +++ b/migration/savevm.c
> >> @@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >>      qemu_loadvm_state_cleanup();
> >>  
> >>      rcu_unregister_thread();
> >> -    mis->have_listen_thread = false;
> >>      postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
> >
> >That now needs a big comment saying it must be the last thing in the
> >thread, because now it's got meaning that it's here.
> >
> >>  
> >>      return NULL;
> >> @@ -1880,7 +1879,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> >>          return -1;
> >>      }
> >>  
> >> -    mis->have_listen_thread = true;
> >>      /* Start up the listening thread and wait for it to signal ready */
> >>      qemu_sem_init(&mis->listen_thread_sem, 0);
> >>      qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> >> @@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >>  
> >>      trace_qemu_loadvm_state_post_main(ret);
> >>  
> >> -    if (mis->have_listen_thread) {
> >> +    if (postcopy_is_running()) {
> >>          /* Listen thread still going, can't clean up yet */
> >>          return ret;
> >>      }
> >
> >Can you explain to me why this is afe in the case of a failure in
> >loadvm_postcopy_handle_listen between the start where it sets
> >the state to LISTENING, and the point where it currently sets
> >hasve_listen_thread ?  Wouldn't this cause qemu_loadvm_state
> >not to cleanup?
> >
> 
> I have to say you are right.  listen_thread may not started when PostcopyState
> is already set to LISTENING.
> 
> The ugly fix may be set PostcopyState back to original one. Not sure whether
> you would like this. 

I think the 'have_listen_thread' might be the simplest solution though;
it's very simple!

Dave

> >Dave
> >
> >> -- 
> >> 2.17.1
> >> 
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Wei Yang]

On Wed, Oct 09, 2019 at 11:17:16AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> On Tue, Oct 08, 2019 at 08:15:51PM +0100, Dr. David Alan Gilbert wrote:
>> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> >> After previous cleanup, postcopy thread is running only when
>> >> PostcopyState is LISTENNING or RUNNING. This means it is not necessary
>> >> to spare a variable have_listen_thread to represent the state.
>> >> 
>> >> Replace the check on have_listen_thread with PostcopyState and remove
>> >> the variable.
>> >> 
>> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> ---
>> >>  migration/migration.h | 1 -
>> >>  migration/ram.c       | 2 +-
>> >>  migration/ram.h       | 1 +
>> >>  migration/savevm.c    | 4 +---
>> >>  4 files changed, 3 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/migration/migration.h b/migration/migration.h
>> >> index 4f2fe193dc..a4d639663d 100644
>> >> --- a/migration/migration.h
>> >> +++ b/migration/migration.h
>> >> @@ -63,7 +63,6 @@ struct MigrationIncomingState {
>> >>      /* Set this when we want the fault thread to quit */
>> >>      bool           fault_thread_quit;
>> >>  
>> >> -    bool           have_listen_thread;
>> >>      QemuThread     listen_thread;
>> >>      QemuSemaphore  listen_thread_sem;
>> >>  
>> >> diff --git a/migration/ram.c b/migration/ram.c
>> >> index 769d3f6454..dfc50d57d5 100644
>> >> --- a/migration/ram.c
>> >> +++ b/migration/ram.c
>> >> @@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
>> >>      return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
>> >>  }
>> >>  
>> >> -static bool postcopy_is_running(void)
>> >> +bool postcopy_is_running(void)
>> >>  {
>> >>      PostcopyState ps = postcopy_state_get();
>> >>      return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
>> >> diff --git a/migration/ram.h b/migration/ram.h
>> >> index bd0eee79b6..44fe4753ad 100644
>> >> --- a/migration/ram.h
>> >> +++ b/migration/ram.h
>> >> @@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
>> >>  /* For incoming postcopy discard */
>> >>  int ram_discard_range(const char *block_name, uint64_t start, size_t length);
>> >>  int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>> >> +bool postcopy_is_running(void);
>> >>  
>> >>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>> >>  
>> >> diff --git a/migration/savevm.c b/migration/savevm.c
>> >> index dcad8897a3..2a0e0b94df 100644
>> >> --- a/migration/savevm.c
>> >> +++ b/migration/savevm.c
>> >> @@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>> >>      qemu_loadvm_state_cleanup();
>> >>  
>> >>      rcu_unregister_thread();
>> >> -    mis->have_listen_thread = false;
>> >>      postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
>> >
>> >That now needs a big comment saying it must be the last thing in the
>> >thread, because now it's got meaning that it's here.
>> >
>> >>  
>> >>      return NULL;
>> >> @@ -1880,7 +1879,6 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>> >>          return -1;
>> >>      }
>> >>  
>> >> -    mis->have_listen_thread = true;
>> >>      /* Start up the listening thread and wait for it to signal ready */
>> >>      qemu_sem_init(&mis->listen_thread_sem, 0);
>> >>      qemu_thread_create(&mis->listen_thread, "postcopy/listen",
>> >> @@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
>> >>  
>> >>      trace_qemu_loadvm_state_post_main(ret);
>> >>  
>> >> -    if (mis->have_listen_thread) {
>> >> +    if (postcopy_is_running()) {
>> >>          /* Listen thread still going, can't clean up yet */
>> >>          return ret;
>> >>      }
>> >
>> >Can you explain to me why this is afe in the case of a failure in
>> >loadvm_postcopy_handle_listen between the start where it sets
>> >the state to LISTENING, and the point where it currently sets
>> >hasve_listen_thread ?  Wouldn't this cause qemu_loadvm_state
>> >not to cleanup?
>> >
>> 
>> I have to say you are right.  listen_thread may not started when PostcopyState
>> is already set to LISTENING.
>> 
>> The ugly fix may be set PostcopyState back to original one. Not sure whether
>> you would like this. 
>
>I think the 'have_listen_thread' might be the simplest solution though;
>it's very simple!
>

Yep, you are right.

>Dave
>
>> >Dave
>> >
>> >> -- 
>> >> 2.17.1
>> >> 
>> >--
>> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> 
>> -- 
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/3] migration/postcopy: replace have_listen_thread check with PostcopyState check

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> have_listen_thread is used to be a indication of whether postcopy thread is
> running. Since we use PostcopyState to record state and the postcopy thread
> only runs when postcopy_is_running(), we can leverage the PostcopyState to
> replace the meaning of have_listen_thread.
> 
> To do so, two preparation cleanup is included.

1,2 queued (2 with fixup for original postcopy_state_set)

> 
> Wei Yang (3):
>   migration/postcopy: mis->have_listen_thread check will never be
>     touched
>   migration/postcopy: postpone setting PostcopyState to END
>   migration/postcopy: replace have_listen_thread check with
>     PostcopyState check
> 
>  migration/migration.h    |  1 -
>  migration/postcopy-ram.c |  2 --
>  migration/ram.c          |  2 +-
>  migration/ram.h          |  1 +
>  migration/savevm.c       | 11 +++--------
>  5 files changed, 5 insertions(+), 12 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK