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

* 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

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

* 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

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