diff mbox series

[v5,08/24] replay: Fix migration use of clock

Message ID 20240318154621.2361161-9-npiggin@gmail.com
State New
Headers show
Series replay: fixes and new test cases | expand

Commit Message

Nicholas Piggin March 18, 2024, 3:46 p.m. UTC
Migration reads host clocks when not holding the replay_mutex, which
asserts when recording a trace. It seems that these migration times
should be host times like other statistics in MigrationState. These
do not require the replay_mutex.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 migration/migration.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Alex Bennée March 19, 2024, 8:40 p.m. UTC | #1
Nicholas Piggin <npiggin@gmail.com> writes:

> Migration reads host clocks when not holding the replay_mutex, which
> asserts when recording a trace. It seems that these migration times
> should be host times like other statistics in MigrationState.

s/host/CLOCK_HOST/ and s/host/CLOCK_REALTIME/ but its a confusing
sentence. If the MigrationState is guest visible it should be
QEMU_CLOCK_VIRTUAL surely?

> These
> do not require the replay_mutex.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  migration/migration.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 644e073b7d..2c286ccf63 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3424,7 +3424,7 @@ static void *migration_thread(void *opaque)
>  {
>      MigrationState *s = opaque;
>      MigrationThread *thread = NULL;
> -    int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>      MigThrError thr_error;
>      bool urgent = false;
>  
> @@ -3476,7 +3476,7 @@ static void *migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - setup_start;
>  
>      trace_migration_thread_setup_complete();
>  
> @@ -3555,7 +3555,7 @@ static void *bg_migration_thread(void *opaque)
>  
>      migration_rate_set(RATE_LIMIT_DISABLED);
>  
> -    setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> +    setup_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>      /*
>       * We want to save vmstate for the moment when migration has been
>       * initiated but also we want to save RAM content while VM is running.
> @@ -3588,7 +3588,7 @@ static void *bg_migration_thread(void *opaque)
>      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>                                 MIGRATION_STATUS_ACTIVE);
>  
> -    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> +    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - setup_start;
>  
>      trace_migration_thread_setup_complete();
Nicholas Piggin March 20, 2024, 4:48 a.m. UTC | #2
On Wed Mar 20, 2024 at 6:40 AM AEST, Alex Bennée wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > Migration reads host clocks when not holding the replay_mutex, which
> > asserts when recording a trace. It seems that these migration times
> > should be host times like other statistics in MigrationState.
>
> s/host/CLOCK_HOST/ and s/host/CLOCK_REALTIME/ but its a confusing
> sentence.

Yes you're right.

> If the MigrationState is guest visible it should be
> QEMU_CLOCK_VIRTUAL surely?

I didn't think it was visible. It was added with ed4fbd10823 and
just exported to QMP.

It was the first and only user of host clock in migration stats,
the rest use rt clock. OTOH it does seem to have deliberately
chosen host... can you see any reason for it?

Thanks,
Nick
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 644e073b7d..2c286ccf63 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3424,7 +3424,7 @@  static void *migration_thread(void *opaque)
 {
     MigrationState *s = opaque;
     MigrationThread *thread = NULL;
-    int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+    int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     MigThrError thr_error;
     bool urgent = false;
 
@@ -3476,7 +3476,7 @@  static void *migration_thread(void *opaque)
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
                                MIGRATION_STATUS_ACTIVE);
 
-    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
+    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - setup_start;
 
     trace_migration_thread_setup_complete();
 
@@ -3555,7 +3555,7 @@  static void *bg_migration_thread(void *opaque)
 
     migration_rate_set(RATE_LIMIT_DISABLED);
 
-    setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
+    setup_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     /*
      * We want to save vmstate for the moment when migration has been
      * initiated but also we want to save RAM content while VM is running.
@@ -3588,7 +3588,7 @@  static void *bg_migration_thread(void *opaque)
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
                                MIGRATION_STATUS_ACTIVE);
 
-    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
+    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - setup_start;
 
     trace_migration_thread_setup_complete();