| Message ID | 20241220104220.2007786-2-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | replay: Fixes and avocado test updates | expand |
On Fri, Dec 20, 2024 at 08:42:03PM +1000, Nicholas Piggin wrote: > Migration reads CLOCK_HOST when not holding the replay_mutex, which > asserts when recording a trace. These are not guest visible so should > be CLOCK_REALTIME like other statistics in MigrationState, which do > not require the replay_mutex. Irrelevant of the change, should we document such lock implications in timer.h? > > 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 8c5bd0a75c8..2eb9e50a263 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -3433,7 +3433,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; > Error *local_err = NULL; > @@ -3504,7 +3504,7 @@ static void *migration_thread(void *opaque) > goto out; > } > > - 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(); > > @@ -3584,7 +3584,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. > @@ -3629,7 +3629,7 @@ static void *bg_migration_thread(void *opaque) > goto fail_setup; > } > > - 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(); > > -- > 2.45.2 >
On Sat Dec 21, 2024 at 2:31 AM AEST, Peter Xu wrote: > On Fri, Dec 20, 2024 at 08:42:03PM +1000, Nicholas Piggin wrote: > > Migration reads CLOCK_HOST when not holding the replay_mutex, which > > asserts when recording a trace. These are not guest visible so should > > be CLOCK_REALTIME like other statistics in MigrationState, which do > > not require the replay_mutex. > > Irrelevant of the change, should we document such lock implications in > timer.h? I guess the intention was to try to avoid caller caring too much about replay internals, so I'm not sure if that will help or hinder understanding :( I think the big rule is something like "if it affects guest state, then you must use HOST or VIRTUAL*, if it does not affect guest state then you must use REALTIME". record-replay code then takes care of replay mutex locking. Does get a little fuzzy around edges in code that is somewhat aware of record-replay though, like migration/snapshots. (Pavel please correct me if I've been saying the wrong things) Thanks, Nick > > > > > 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 8c5bd0a75c8..2eb9e50a263 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -3433,7 +3433,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; > > Error *local_err = NULL; > > @@ -3504,7 +3504,7 @@ static void *migration_thread(void *opaque) > > goto out; > > } > > > > - 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(); > > > > @@ -3584,7 +3584,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. > > @@ -3629,7 +3629,7 @@ static void *bg_migration_thread(void *opaque) > > goto fail_setup; > > } > > > > - 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(); > > > > -- > > 2.45.2 > >
diff --git a/migration/migration.c b/migration/migration.c index 8c5bd0a75c8..2eb9e50a263 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3433,7 +3433,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; Error *local_err = NULL; @@ -3504,7 +3504,7 @@ static void *migration_thread(void *opaque) goto out; } - 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(); @@ -3584,7 +3584,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. @@ -3629,7 +3629,7 @@ static void *bg_migration_thread(void *opaque) goto fail_setup; } - 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();
Migration reads CLOCK_HOST when not holding the replay_mutex, which asserts when recording a trace. These are not guest visible so should be CLOCK_REALTIME like other statistics in MigrationState, which 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(-)