| Message ID | 20230728121516.16258-3-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix segfault on migration return path | expand |
On Fri, Jul 28, 2023 at 09:15:15AM -0300, Fabiano Rosas wrote: > We're about to reuse this function so move the 'rp_thread_created' > check into it and remove the redundant tracing and comment. > > Add a new tracepoint akin to what is already done at > migration_completion(). > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/migration.c | 21 +++++++-------------- > migration/trace-events | 3 +-- > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 051067f8c5..d6f4470265 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms, > /* Returns 0 if the RP was ok, otherwise there was an error on the RP */ > static int await_return_path_close_on_source(MigrationState *ms) > { > + if (!ms->rp_state.rp_thread_created) { > + return 0; > + } > + > /* > * If this is a normal exit then the destination will send a SHUT and the > * rp_thread will exit, however if there's an error we need to cause > @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s) > goto fail; > } > > - /* > - * If rp was opened we must clean up the thread before > - * cleaning everything else up (since if there are no failures > - * it will wait for the destination to send it's status in > - * a SHUT command). > - */ > - if (s->rp_state.rp_thread_created) { > - int rp_error; > - trace_migration_return_path_end_before(); > - rp_error = await_return_path_close_on_source(s); > - trace_migration_return_path_end_after(rp_error); > - if (rp_error) { > - goto fail; > - } > + if (await_return_path_close_on_source(s)) { > + trace_migration_completion_rp_err(); > + goto fail; > } > > if (qemu_file_get_error(s->to_dst_file)) { > diff --git a/migration/trace-events b/migration/trace-events > index 5259c1044b..33a69064ca 100644 > --- a/migration/trace-events > +++ b/migration/trace-events > @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p > migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d" > migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64 > migration_completion_file_err(void) "" > +migration_completion_rp_err(void) "" > migration_completion_vm_stop(int ret) "ret %d" > migration_completion_postcopy_end(void) "" > migration_completion_postcopy_end_after_complete(void) "" > migration_rate_limit_pre(int ms) "%d ms" > migration_rate_limit_post(int urgent) "urgent: %d" > -migration_return_path_end_before(void) "" > -migration_return_path_end_after(int rp_error) "%d" > migration_thread_after_loop(void) "" > migration_thread_file_err(void) "" > migration_thread_setup_complete(void) "" > -- > 2.35.3 > Just in case someone may still want to see the old trace_migration_return_path_end_before() tracepoint, maybe just move them all over?
Peter Xu <peterx@redhat.com> writes: > On Fri, Jul 28, 2023 at 09:15:15AM -0300, Fabiano Rosas wrote: >> We're about to reuse this function so move the 'rp_thread_created' >> check into it and remove the redundant tracing and comment. >> >> Add a new tracepoint akin to what is already done at >> migration_completion(). >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> migration/migration.c | 21 +++++++-------------- >> migration/trace-events | 3 +-- >> 2 files changed, 8 insertions(+), 16 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 051067f8c5..d6f4470265 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms, >> /* Returns 0 if the RP was ok, otherwise there was an error on the RP */ >> static int await_return_path_close_on_source(MigrationState *ms) >> { >> + if (!ms->rp_state.rp_thread_created) { >> + return 0; >> + } >> + >> /* >> * If this is a normal exit then the destination will send a SHUT and the >> * rp_thread will exit, however if there's an error we need to cause >> @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s) >> goto fail; >> } >> >> - /* >> - * If rp was opened we must clean up the thread before >> - * cleaning everything else up (since if there are no failures >> - * it will wait for the destination to send it's status in >> - * a SHUT command). >> - */ >> - if (s->rp_state.rp_thread_created) { >> - int rp_error; >> - trace_migration_return_path_end_before(); >> - rp_error = await_return_path_close_on_source(s); >> - trace_migration_return_path_end_after(rp_error); >> - if (rp_error) { >> - goto fail; >> - } >> + if (await_return_path_close_on_source(s)) { >> + trace_migration_completion_rp_err(); >> + goto fail; >> } >> >> if (qemu_file_get_error(s->to_dst_file)) { >> diff --git a/migration/trace-events b/migration/trace-events >> index 5259c1044b..33a69064ca 100644 >> --- a/migration/trace-events >> +++ b/migration/trace-events >> @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p >> migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d" >> migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64 >> migration_completion_file_err(void) "" >> +migration_completion_rp_err(void) "" >> migration_completion_vm_stop(int ret) "ret %d" >> migration_completion_postcopy_end(void) "" >> migration_completion_postcopy_end_after_complete(void) "" >> migration_rate_limit_pre(int ms) "%d ms" >> migration_rate_limit_post(int urgent) "urgent: %d" >> -migration_return_path_end_before(void) "" >> -migration_return_path_end_after(int rp_error) "%d" >> migration_thread_after_loop(void) "" >> migration_thread_file_err(void) "" >> migration_thread_setup_complete(void) "" >> -- >> 2.35.3 >> > > Just in case someone may still want to see the old > trace_migration_return_path_end_before() tracepoint, maybe just move them > all over? They are made redundant by trace_await_return_path_close_on_source_joining() and trace_await_return_path_close_on_source_close() which already exist in the function.
On Mon, Jul 31, 2023 at 09:42:27AM -0300, Fabiano Rosas wrote: > They are made redundant by > trace_await_return_path_close_on_source_joining() and > trace_await_return_path_close_on_source_close() which already exist in > the function. Oh.. that's okay then. Thanks,
diff --git a/migration/migration.c b/migration/migration.c index 051067f8c5..d6f4470265 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms, /* Returns 0 if the RP was ok, otherwise there was an error on the RP */ static int await_return_path_close_on_source(MigrationState *ms) { + if (!ms->rp_state.rp_thread_created) { + return 0; + } + /* * If this is a normal exit then the destination will send a SHUT and the * rp_thread will exit, however if there's an error we need to cause @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s) goto fail; } - /* - * If rp was opened we must clean up the thread before - * cleaning everything else up (since if there are no failures - * it will wait for the destination to send it's status in - * a SHUT command). - */ - if (s->rp_state.rp_thread_created) { - int rp_error; - trace_migration_return_path_end_before(); - rp_error = await_return_path_close_on_source(s); - trace_migration_return_path_end_after(rp_error); - if (rp_error) { - goto fail; - } + if (await_return_path_close_on_source(s)) { + trace_migration_completion_rp_err(); + goto fail; } if (qemu_file_get_error(s->to_dst_file)) { diff --git a/migration/trace-events b/migration/trace-events index 5259c1044b..33a69064ca 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d" migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64 migration_completion_file_err(void) "" +migration_completion_rp_err(void) "" migration_completion_vm_stop(int ret) "ret %d" migration_completion_postcopy_end(void) "" migration_completion_postcopy_end_after_complete(void) "" migration_rate_limit_pre(int ms) "%d ms" migration_rate_limit_post(int urgent) "urgent: %d" -migration_return_path_end_before(void) "" -migration_return_path_end_after(int rp_error) "%d" migration_thread_after_loop(void) "" migration_thread_file_err(void) "" migration_thread_setup_complete(void) ""
We're about to reuse this function so move the 'rp_thread_created' check into it and remove the redundant tracing and comment. Add a new tracepoint akin to what is already done at migration_completion(). Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/migration.c | 21 +++++++-------------- migration/trace-events | 3 +-- 2 files changed, 8 insertions(+), 16 deletions(-)