[PATCH 1/2] Fixed a QEMU hang when guest poweroff in COLO mode

[PATCH 1/2] Fixed a QEMU hang when guest poweroff in COLO mode

[Rao, Lei]

From: "Rao, Lei" <lei.rao@intel.com>

When the PVM guest poweroff, the COLO thread may wait a semaphore
in colo_process_checkpoint().So, we should wake up the COLO thread
before migration shutdown.

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 include/migration/colo.h |  1 +
 migration/colo.c         | 14 ++++++++++++++
 migration/migration.c    | 10 ++++++++++
 3 files changed, 25 insertions(+)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index 768e1f0..525b45a 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -37,4 +37,5 @@ COLOMode get_colo_mode(void);
 void colo_do_failover(void);
 
 void colo_checkpoint_notify(void *opaque);
+void colo_shutdown(COLOMode mode);
 #endif
diff --git a/migration/colo.c b/migration/colo.c
index 2415325..385c1d7 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -820,6 +820,20 @@ static void colo_wait_handle_message(MigrationIncomingState *mis,
     }
 }
 
+void colo_shutdown(COLOMode mode)
+{
+    if (mode == COLO_MODE_PRIMARY) {
+        MigrationState *s = migrate_get_current();
+
+        qemu_event_set(&s->colo_checkpoint_event);
+        qemu_sem_post(&s->colo_exit_sem);
+    } else {
+        MigrationIncomingState *mis = migration_incoming_get_current();
+
+        qemu_sem_post(&mis->colo_incoming_sem);
+    }
+}
+
 void *colo_process_incoming_thread(void *opaque)
 {
     MigrationIncomingState *mis = opaque;
diff --git a/migration/migration.c b/migration/migration.c
index abaf6f9..9df6328 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -225,6 +225,16 @@ void migration_cancel(const Error *error)
 
 void migration_shutdown(void)
 {
+    COLOMode mode = get_colo_mode();
+
+    /*
+     * When the QEMU main thread exit, the COLO thread
+     * may wait a semaphore. So, we should wakeup the
+     * COLO thread before migration shutdown.
+     */
+    if (mode != COLO_MODE_NONE) {
+        colo_shutdown(mode);
+     }
     /*
      * Cancel the current migration - that will (eventually)
      * stop the migration using this structure
-- 
1.8.3.1

[PATCH 2/2] migration/ram.c: Remove the qemu_mutex_lock in colo_flush_ram_cache.

[Rao, Lei]

From: "Rao, Lei" <lei.rao@intel.com>

The code to acquire bitmap_mutex is added in the commit of
"63268c4970a5f126cc9af75f3ccb8057abef5ec0". There is no
need to acquire bitmap_mutex in colo_flush_ram_cache(). This
is because the colo_flush_ram_cache only be called on the COLO
secondary VM, which is the destination side.
On the COLO secondary VM, only the COLO thread will touch
the bitmap of ram cache.

Signed-off-by: Lei Rao <lei.rao@intel.com>
---
 migration/ram.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 863035d..2c688f5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3918,7 +3918,6 @@ void colo_flush_ram_cache(void)
     unsigned long offset = 0;
 
     memory_global_dirty_log_sync();
-    qemu_mutex_lock(&ram_state->bitmap_mutex);
     WITH_RCU_READ_LOCK_GUARD() {
         RAMBLOCK_FOREACH_NOT_IGNORED(block) {
             ramblock_sync_dirty_bitmap(ram_state, block);
@@ -3954,7 +3953,6 @@ void colo_flush_ram_cache(void)
         }
     }
     trace_colo_flush_ram_cache_end();
-    qemu_mutex_unlock(&ram_state->bitmap_mutex);
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH 1/2] Fixed a QEMU hang when guest poweroff in COLO mode

[Juan Quintela]

"Rao, Lei" <lei.rao@intel.com> wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
>
> When the PVM guest poweroff, the COLO thread may wait a semaphore
> in colo_process_checkpoint().So, we should wake up the COLO thread
> before migration shutdown.
>
> Signed-off-by: Lei Rao <lei.rao@intel.com>

A couple of notes.

> ---
>  include/migration/colo.h |  1 +
>  migration/colo.c         | 14 ++++++++++++++
>  migration/migration.c    | 10 ++++++++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/include/migration/colo.h b/include/migration/colo.h
> index 768e1f0..525b45a 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -37,4 +37,5 @@ COLOMode get_colo_mode(void);
>  void colo_do_failover(void);
>  
>  void colo_checkpoint_notify(void *opaque);
> +void colo_shutdown(COLOMode mode);
>  #endif
> diff --git a/migration/colo.c b/migration/colo.c
> index 2415325..385c1d7 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -820,6 +820,20 @@ static void colo_wait_handle_message(MigrationIncomingState *mis,
>      }
>  }
>  
> +void colo_shutdown(COLOMode mode)
> +{
> +    if (mode == COLO_MODE_PRIMARY) {
> +        MigrationState *s = migrate_get_current();
> +
> +        qemu_event_set(&s->colo_checkpoint_event);
> +        qemu_sem_post(&s->colo_exit_sem);
> +    } else {
> +        MigrationIncomingState *mis = migration_incoming_get_current();
> +
> +        qemu_sem_post(&mis->colo_incoming_sem);
> +    }
> +}
> +
>  void *colo_process_incoming_thread(void *opaque)
>  {
>      MigrationIncomingState *mis = opaque;
> diff --git a/migration/migration.c b/migration/migration.c
> index abaf6f9..9df6328 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -225,6 +225,16 @@ void migration_cancel(const Error *error)
>  
>  void migration_shutdown(void)
>  {
> +    COLOMode mode = get_colo_mode();
> +
> +    /*
> +     * When the QEMU main thread exit, the COLO thread
> +     * may wait a semaphore. So, we should wakeup the
> +     * COLO thread before migration shutdown.
> +     */
> +    if (mode != COLO_MODE_NONE) {
> +        colo_shutdown(mode);
> +     }

don't put the code on the main path.

Could you just put all of this inside a colo_shutdown() call?

Thanks, Juan.

>      /*
>       * Cancel the current migration - that will (eventually)
>       * stop the migration using this structure

Re: [PATCH 2/2] migration/ram.c: Remove the qemu_mutex_lock in colo_flush_ram_cache.

[Juan Quintela]

"Rao, Lei" <lei.rao@intel.com> wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
>
> The code to acquire bitmap_mutex is added in the commit of
> "63268c4970a5f126cc9af75f3ccb8057abef5ec0". There is no
> need to acquire bitmap_mutex in colo_flush_ram_cache(). This
> is because the colo_flush_ram_cache only be called on the COLO
> secondary VM, which is the destination side.
> On the COLO secondary VM, only the COLO thread will touch
> the bitmap of ram cache.
>
> Signed-off-by: Lei Rao <lei.rao@intel.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

As we are on the softfreeze, I am queuing this on my next-7.0 branch.

Later, Juan.

RE: [PATCH 1/2] Fixed a QEMU hang when guest poweroff in COLO mode

[Rao, Lei]

OK, will be changed in V2.

Thanks,
Lei

-----Original Message-----
From: Juan Quintela <quintela@redhat.com> 
Sent: Wednesday, November 10, 2021 5:55 PM
To: Rao, Lei <lei.rao@intel.com>
Cc: Zhang, Chen <chen.zhang@intel.com>; zhang.zhanghailiang@huawei.com; dgilbert@redhat.com; qemu-devel@nongnu.org
Subject: Re: [PATCH 1/2] Fixed a QEMU hang when guest poweroff in COLO mode

"Rao, Lei" <lei.rao@intel.com> wrote:
> From: "Rao, Lei" <lei.rao@intel.com>
>
> When the PVM guest poweroff, the COLO thread may wait a semaphore in 
> colo_process_checkpoint().So, we should wake up the COLO thread before 
> migration shutdown.
>
> Signed-off-by: Lei Rao <lei.rao@intel.com>

A couple of notes.

> ---
>  include/migration/colo.h |  1 +
>  migration/colo.c         | 14 ++++++++++++++
>  migration/migration.c    | 10 ++++++++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/include/migration/colo.h b/include/migration/colo.h index 
> 768e1f0..525b45a 100644
> --- a/include/migration/colo.h
> +++ b/include/migration/colo.h
> @@ -37,4 +37,5 @@ COLOMode get_colo_mode(void);  void 
> colo_do_failover(void);
>  
>  void colo_checkpoint_notify(void *opaque);
> +void colo_shutdown(COLOMode mode);
>  #endif
> diff --git a/migration/colo.c b/migration/colo.c index 
> 2415325..385c1d7 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -820,6 +820,20 @@ static void colo_wait_handle_message(MigrationIncomingState *mis,
>      }
>  }
>  
> +void colo_shutdown(COLOMode mode)
> +{
> +    if (mode == COLO_MODE_PRIMARY) {
> +        MigrationState *s = migrate_get_current();
> +
> +        qemu_event_set(&s->colo_checkpoint_event);
> +        qemu_sem_post(&s->colo_exit_sem);
> +    } else {
> +        MigrationIncomingState *mis = 
> + migration_incoming_get_current();
> +
> +        qemu_sem_post(&mis->colo_incoming_sem);
> +    }
> +}
> +
>  void *colo_process_incoming_thread(void *opaque)  {
>      MigrationIncomingState *mis = opaque; diff --git 
> a/migration/migration.c b/migration/migration.c index abaf6f9..9df6328 
> 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -225,6 +225,16 @@ void migration_cancel(const Error *error)
>  
>  void migration_shutdown(void)
>  {
> +    COLOMode mode = get_colo_mode();
> +
> +    /*
> +     * When the QEMU main thread exit, the COLO thread
> +     * may wait a semaphore. So, we should wakeup the
> +     * COLO thread before migration shutdown.
> +     */
> +    if (mode != COLO_MODE_NONE) {
> +        colo_shutdown(mode);
> +     }

don't put the code on the main path.

Could you just put all of this inside a colo_shutdown() call?

Thanks, Juan.

>      /*
>       * Cancel the current migration - that will (eventually)
>       * stop the migration using this structure

RE: [PATCH 2/2] migration/ram.c: Remove the qemu_mutex_lock in colo_flush_ram_cache.

[Zhang, Chen]

> -----Original Message-----
> From: Rao, Lei <lei.rao@intel.com>
> Sent: Wednesday, November 10, 2021 4:38 PM
> To: Zhang, Chen <chen.zhang@intel.com>;
> zhang.zhanghailiang@huawei.com; quintela@redhat.com;
> dgilbert@redhat.com
> Cc: qemu-devel@nongnu.org; Rao, Lei <lei.rao@intel.com>
> Subject: [PATCH 2/2] migration/ram.c: Remove the qemu_mutex_lock in
> colo_flush_ram_cache.
> 
> From: "Rao, Lei" <lei.rao@intel.com>
> 
> The code to acquire bitmap_mutex is added in the commit of
> "63268c4970a5f126cc9af75f3ccb8057abef5ec0". There is no need to acquire
> bitmap_mutex in colo_flush_ram_cache(). This is because the
> colo_flush_ram_cache only be called on the COLO secondary VM, which is
> the destination side.
> On the COLO secondary VM, only the COLO thread will touch the bitmap of
> ram cache.
> 
> Signed-off-by: Lei Rao <lei.rao@intel.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
>  migration/ram.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c index 863035d..2c688f5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3918,7 +3918,6 @@ void colo_flush_ram_cache(void)
>      unsigned long offset = 0;
> 
>      memory_global_dirty_log_sync();
> -    qemu_mutex_lock(&ram_state->bitmap_mutex);
>      WITH_RCU_READ_LOCK_GUARD() {
>          RAMBLOCK_FOREACH_NOT_IGNORED(block) {
>              ramblock_sync_dirty_bitmap(ram_state, block); @@ -3954,7 +3953,6
> @@ void colo_flush_ram_cache(void)
>          }
>      }
>      trace_colo_flush_ram_cache_end();
> -    qemu_mutex_unlock(&ram_state->bitmap_mutex);
>  }
> 
>  /**
> --
> 1.8.3.1