| Message ID | 1502777827-18874-6-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 15, 2017 at 02:17:06PM +0800, Peter Xu wrote: > Store the task tag for migration types: tcp/unix/fd/exec in current > MigrationIncomingState struct. > > For defered migration, no need to store task tag since there is no task > running in the main loop at all. For RDMA, let's mark it as todo. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/migration.c | 22 ++++++++++++++++++---- > migration/migration.h | 2 ++ > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index c9b7085..daf356b 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) > mis->from_src_file = NULL; > } > > + mis->listen_task_tag = 0; > qemu_event_destroy(&mis->main_thread_load_event); > } > > @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, > void qemu_start_incoming_migration(const char *uri, Error **errp) > { > const char *p; > + guint task_tag = 0; > + MigrationIncomingState *mis = migration_incoming_get_current(); > > qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); > if (!strcmp(uri, "defer")) { > deferred_incoming_migration(errp); > } else if (strstart(uri, "tcp:", &p)) { > - tcp_start_incoming_migration(p, errp); > + task_tag = tcp_start_incoming_migration(p, errp); > #ifdef CONFIG_RDMA > } else if (strstart(uri, "rdma:", &p)) { > + /* TODO: store task tag for RDMA migrations */ > rdma_start_incoming_migration(p, errp); > #endif > } else if (strstart(uri, "exec:", &p)) { > - exec_start_incoming_migration(p, errp); > + task_tag = exec_start_incoming_migration(p, errp); > } else if (strstart(uri, "unix:", &p)) { > - unix_start_incoming_migration(p, errp); > + task_tag = unix_start_incoming_migration(p, errp); > } else if (strstart(uri, "fd:", &p)) { > - fd_start_incoming_migration(p, errp); > + task_tag = fd_start_incoming_migration(p, errp); > } else { > error_setg(errp, "unknown migration protocol: %s", uri); > + return; > } > + > + mis->listen_task_tag = task_tag; This is unsafe as currently written. The main loop watches that are associated with these tags are all unregistered when incoming migration starts. So if you save them, and then unregister later when postcopy is "stuck", then you're likely unregistering a tag associated with some other random part of QEMU, because the saved value is no longer valid. Regards, Daniel
On Tue, Aug 15, 2017 at 09:37:14AM +0100, Daniel P. Berrange wrote: > On Tue, Aug 15, 2017 at 02:17:06PM +0800, Peter Xu wrote: > > Store the task tag for migration types: tcp/unix/fd/exec in current > > MigrationIncomingState struct. > > > > For defered migration, no need to store task tag since there is no task > > running in the main loop at all. For RDMA, let's mark it as todo. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/migration.c | 22 ++++++++++++++++++---- > > migration/migration.h | 2 ++ > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index c9b7085..daf356b 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) > > mis->from_src_file = NULL; > > } > > > > + mis->listen_task_tag = 0; > > qemu_event_destroy(&mis->main_thread_load_event); > > } > > > > @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, > > void qemu_start_incoming_migration(const char *uri, Error **errp) > > { > > const char *p; > > + guint task_tag = 0; > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); > > if (!strcmp(uri, "defer")) { > > deferred_incoming_migration(errp); > > } else if (strstart(uri, "tcp:", &p)) { > > - tcp_start_incoming_migration(p, errp); > > + task_tag = tcp_start_incoming_migration(p, errp); > > #ifdef CONFIG_RDMA > > } else if (strstart(uri, "rdma:", &p)) { > > + /* TODO: store task tag for RDMA migrations */ > > rdma_start_incoming_migration(p, errp); > > #endif > > } else if (strstart(uri, "exec:", &p)) { > > - exec_start_incoming_migration(p, errp); > > + task_tag = exec_start_incoming_migration(p, errp); > > } else if (strstart(uri, "unix:", &p)) { > > - unix_start_incoming_migration(p, errp); > > + task_tag = unix_start_incoming_migration(p, errp); > > } else if (strstart(uri, "fd:", &p)) { > > - fd_start_incoming_migration(p, errp); > > + task_tag = fd_start_incoming_migration(p, errp); > > } else { > > error_setg(errp, "unknown migration protocol: %s", uri); > > + return; > > } > > + > > + mis->listen_task_tag = task_tag; > > This is unsafe as currently written. The main loop watches that are > associated with these tags are all unregistered when incoming migration > starts. So if you save them, and then unregister later when postcopy > is "stuck", then you're likely unregistering a tag associated with > some other random part of QEMU, because the saved value is no longer > valid. IIUC for incoming migration, the watch is not detached until migration_fd_process_incoming() completes. So: - for precopy, the watch should be detached when migration completes - for postcopy, the watch should be detached when postcopy starts, then main loop thread returns, while the ram_load_thread starts to continue the migration. So basically the watch is detaching itself after migration_fd_process_incoming() completes. To handle that, this patch has this change: @@ -422,6 +429,13 @@ void migration_fd_process_incoming(QEMUFile *f) co = qemu_coroutine_create(process_incoming_migration_co, f); qemu_coroutine_enter(co); } + + /* + * When reach here, we should not need the listening port any + * more. We'll detach the listening task soon, let's reset the + * listen task tag. + */ + mis->listen_task_tag = 0; Would this make sure the listen_task_tag is always valid if nonzero? Thanks,
On Tue, Aug 15, 2017 at 04:50:06PM +0800, Peter Xu wrote: > On Tue, Aug 15, 2017 at 09:37:14AM +0100, Daniel P. Berrange wrote: > > On Tue, Aug 15, 2017 at 02:17:06PM +0800, Peter Xu wrote: > > > Store the task tag for migration types: tcp/unix/fd/exec in current > > > MigrationIncomingState struct. > > > > > > For defered migration, no need to store task tag since there is no task > > > running in the main loop at all. For RDMA, let's mark it as todo. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > > migration/migration.c | 22 ++++++++++++++++++---- > > > migration/migration.h | 2 ++ > > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > index c9b7085..daf356b 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) > > > mis->from_src_file = NULL; > > > } > > > > > > + mis->listen_task_tag = 0; > > > qemu_event_destroy(&mis->main_thread_load_event); > > > } > > > > > > @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, > > > void qemu_start_incoming_migration(const char *uri, Error **errp) > > > { > > > const char *p; > > > + guint task_tag = 0; > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > > qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); > > > if (!strcmp(uri, "defer")) { > > > deferred_incoming_migration(errp); > > > } else if (strstart(uri, "tcp:", &p)) { > > > - tcp_start_incoming_migration(p, errp); > > > + task_tag = tcp_start_incoming_migration(p, errp); > > > #ifdef CONFIG_RDMA > > > } else if (strstart(uri, "rdma:", &p)) { > > > + /* TODO: store task tag for RDMA migrations */ > > > rdma_start_incoming_migration(p, errp); > > > #endif > > > } else if (strstart(uri, "exec:", &p)) { > > > - exec_start_incoming_migration(p, errp); > > > + task_tag = exec_start_incoming_migration(p, errp); > > > } else if (strstart(uri, "unix:", &p)) { > > > - unix_start_incoming_migration(p, errp); > > > + task_tag = unix_start_incoming_migration(p, errp); > > > } else if (strstart(uri, "fd:", &p)) { > > > - fd_start_incoming_migration(p, errp); > > > + task_tag = fd_start_incoming_migration(p, errp); > > > } else { > > > error_setg(errp, "unknown migration protocol: %s", uri); > > > + return; > > > } > > > + > > > + mis->listen_task_tag = task_tag; > > > > This is unsafe as currently written. The main loop watches that are > > associated with these tags are all unregistered when incoming migration > > starts. So if you save them, and then unregister later when postcopy > > is "stuck", then you're likely unregistering a tag associated with > > some other random part of QEMU, because the saved value is no longer > > valid. > > IIUC for incoming migration, the watch is not detached until > migration_fd_process_incoming() completes. So: > > - for precopy, the watch should be detached when migration completes > > - for postcopy, the watch should be detached when postcopy starts, > then main loop thread returns, while the ram_load_thread starts to > continue the migration. > > So basically the watch is detaching itself after > migration_fd_process_incoming() completes. To handle that, this patch > has this change: > > @@ -422,6 +429,13 @@ void migration_fd_process_incoming(QEMUFile *f) > co = qemu_coroutine_create(process_incoming_migration_co, f); > qemu_coroutine_enter(co); > } > + > + /* > + * When reach here, we should not need the listening port any > + * more. We'll detach the listening task soon, let's reset the > + * listen task tag. > + */ > + mis->listen_task_tag = 0; > > Would this make sure the listen_task_tag is always valid if nonzero? Mixing 'return FALSE' from callbacks, with g_source_remove is a recipe for hard to diagnose bugs, so should be avoided in general. So, IMHO, you should change all the callbacks to 'return TRUE' so that they keep the watch registered, and then explicitly call g_source_remove() passing the listen tag, when incoming migration starts. Regards, Daniel
On Tue, Aug 15, 2017 at 10:27:07AM +0100, Daniel P. Berrange wrote: > On Tue, Aug 15, 2017 at 04:50:06PM +0800, Peter Xu wrote: > > On Tue, Aug 15, 2017 at 09:37:14AM +0100, Daniel P. Berrange wrote: > > > On Tue, Aug 15, 2017 at 02:17:06PM +0800, Peter Xu wrote: > > > > Store the task tag for migration types: tcp/unix/fd/exec in current > > > > MigrationIncomingState struct. > > > > > > > > For defered migration, no need to store task tag since there is no task > > > > running in the main loop at all. For RDMA, let's mark it as todo. > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > migration/migration.c | 22 ++++++++++++++++++---- > > > > migration/migration.h | 2 ++ > > > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > index c9b7085..daf356b 100644 > > > > --- a/migration/migration.c > > > > +++ b/migration/migration.c > > > > @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) > > > > mis->from_src_file = NULL; > > > > } > > > > > > > > + mis->listen_task_tag = 0; > > > > qemu_event_destroy(&mis->main_thread_load_event); > > > > } > > > > > > > > @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, > > > > void qemu_start_incoming_migration(const char *uri, Error **errp) > > > > { > > > > const char *p; > > > > + guint task_tag = 0; > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > > > > qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); > > > > if (!strcmp(uri, "defer")) { > > > > deferred_incoming_migration(errp); > > > > } else if (strstart(uri, "tcp:", &p)) { > > > > - tcp_start_incoming_migration(p, errp); > > > > + task_tag = tcp_start_incoming_migration(p, errp); > > > > #ifdef CONFIG_RDMA > > > > } else if (strstart(uri, "rdma:", &p)) { > > > > + /* TODO: store task tag for RDMA migrations */ > > > > rdma_start_incoming_migration(p, errp); > > > > #endif > > > > } else if (strstart(uri, "exec:", &p)) { > > > > - exec_start_incoming_migration(p, errp); > > > > + task_tag = exec_start_incoming_migration(p, errp); > > > > } else if (strstart(uri, "unix:", &p)) { > > > > - unix_start_incoming_migration(p, errp); > > > > + task_tag = unix_start_incoming_migration(p, errp); > > > > } else if (strstart(uri, "fd:", &p)) { > > > > - fd_start_incoming_migration(p, errp); > > > > + task_tag = fd_start_incoming_migration(p, errp); > > > > } else { > > > > error_setg(errp, "unknown migration protocol: %s", uri); > > > > + return; > > > > } > > > > + > > > > + mis->listen_task_tag = task_tag; > > > > > > This is unsafe as currently written. The main loop watches that are > > > associated with these tags are all unregistered when incoming migration > > > starts. So if you save them, and then unregister later when postcopy > > > is "stuck", then you're likely unregistering a tag associated with > > > some other random part of QEMU, because the saved value is no longer > > > valid. > > > > IIUC for incoming migration, the watch is not detached until > > migration_fd_process_incoming() completes. So: > > > > - for precopy, the watch should be detached when migration completes > > > > - for postcopy, the watch should be detached when postcopy starts, > > then main loop thread returns, while the ram_load_thread starts to > > continue the migration. > > > > So basically the watch is detaching itself after > > migration_fd_process_incoming() completes. To handle that, this patch > > has this change: > > > > @@ -422,6 +429,13 @@ void migration_fd_process_incoming(QEMUFile *f) > > co = qemu_coroutine_create(process_incoming_migration_co, f); > > qemu_coroutine_enter(co); > > } > > + > > + /* > > + * When reach here, we should not need the listening port any > > + * more. We'll detach the listening task soon, let's reset the > > + * listen task tag. > > + */ > > + mis->listen_task_tag = 0; > > > > Would this make sure the listen_task_tag is always valid if nonzero? > > Mixing 'return FALSE' from callbacks, with g_source_remove is a recipe > for hard to diagnose bugs, so should be avoided in general. Makes sense. > > So, IMHO, you should change all the callbacks to 'return TRUE' so that > they keep the watch registered, and then explicitly call g_source_remove() > passing the listen tag, when incoming migration starts. Yes this sounds better. Thanks!
On Tue, Aug 15, 2017 at 05:47:08PM +0800, Peter Xu wrote: > On Tue, Aug 15, 2017 at 10:27:07AM +0100, Daniel P. Berrange wrote: > > On Tue, Aug 15, 2017 at 04:50:06PM +0800, Peter Xu wrote: > > > On Tue, Aug 15, 2017 at 09:37:14AM +0100, Daniel P. Berrange wrote: > > > > On Tue, Aug 15, 2017 at 02:17:06PM +0800, Peter Xu wrote: > > > > > Store the task tag for migration types: tcp/unix/fd/exec in current > > > > > MigrationIncomingState struct. > > > > > > > > > > For defered migration, no need to store task tag since there is no task > > > > > running in the main loop at all. For RDMA, let's mark it as todo. > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > --- > > > > > migration/migration.c | 22 ++++++++++++++++++---- > > > > > migration/migration.h | 2 ++ > > > > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > > index c9b7085..daf356b 100644 > > > > > --- a/migration/migration.c > > > > > +++ b/migration/migration.c > > > > > @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) > > > > > mis->from_src_file = NULL; > > > > > } > > > > > > > > > > + mis->listen_task_tag = 0; > > > > > qemu_event_destroy(&mis->main_thread_load_event); > > > > > } > > > > > > > > > > @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, > > > > > void qemu_start_incoming_migration(const char *uri, Error **errp) > > > > > { > > > > > const char *p; > > > > > + guint task_tag = 0; > > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > > > > > > qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); > > > > > if (!strcmp(uri, "defer")) { > > > > > deferred_incoming_migration(errp); > > > > > } else if (strstart(uri, "tcp:", &p)) { > > > > > - tcp_start_incoming_migration(p, errp); > > > > > + task_tag = tcp_start_incoming_migration(p, errp); > > > > > #ifdef CONFIG_RDMA > > > > > } else if (strstart(uri, "rdma:", &p)) { > > > > > + /* TODO: store task tag for RDMA migrations */ > > > > > rdma_start_incoming_migration(p, errp); > > > > > #endif > > > > > } else if (strstart(uri, "exec:", &p)) { > > > > > - exec_start_incoming_migration(p, errp); > > > > > + task_tag = exec_start_incoming_migration(p, errp); > > > > > } else if (strstart(uri, "unix:", &p)) { > > > > > - unix_start_incoming_migration(p, errp); > > > > > + task_tag = unix_start_incoming_migration(p, errp); > > > > > } else if (strstart(uri, "fd:", &p)) { > > > > > - fd_start_incoming_migration(p, errp); > > > > > + task_tag = fd_start_incoming_migration(p, errp); > > > > > } else { > > > > > error_setg(errp, "unknown migration protocol: %s", uri); > > > > > + return; > > > > > } > > > > > + > > > > > + mis->listen_task_tag = task_tag; > > > > > > > > This is unsafe as currently written. The main loop watches that are > > > > associated with these tags are all unregistered when incoming migration > > > > starts. So if you save them, and then unregister later when postcopy > > > > is "stuck", then you're likely unregistering a tag associated with > > > > some other random part of QEMU, because the saved value is no longer > > > > valid. > > > > > > IIUC for incoming migration, the watch is not detached until > > > migration_fd_process_incoming() completes. So: > > > > > > - for precopy, the watch should be detached when migration completes > > > > > > - for postcopy, the watch should be detached when postcopy starts, > > > then main loop thread returns, while the ram_load_thread starts to > > > continue the migration. > > > > > > So basically the watch is detaching itself after > > > migration_fd_process_incoming() completes. To handle that, this patch > > > has this change: > > > > > > @@ -422,6 +429,13 @@ void migration_fd_process_incoming(QEMUFile *f) > > > co = qemu_coroutine_create(process_incoming_migration_co, f); > > > qemu_coroutine_enter(co); > > > } > > > + > > > + /* > > > + * When reach here, we should not need the listening port any > > > + * more. We'll detach the listening task soon, let's reset the > > > + * listen task tag. > > > + */ > > > + mis->listen_task_tag = 0; > > > > > > Would this make sure the listen_task_tag is always valid if nonzero? > > > > Mixing 'return FALSE' from callbacks, with g_source_remove is a recipe > > for hard to diagnose bugs, so should be avoided in general. > > Makes sense. > > > > > So, IMHO, you should change all the callbacks to 'return TRUE' so that > > they keep the watch registered, and then explicitly call g_source_remove() > > passing the listen tag, when incoming migration starts. > > Yes this sounds better. Thanks! Hmm... when incoming migration starts, we are still in the handler of the listening gsource. I am not sure whether it's safe to remove the gsource in its own handlers. Would this mean I should still keep current way to do it? After all although we may have two places to detach the gsource (either return FALSE in its handler, or we call g_source_detach), we are safe since both of them are in main loop context. Thanks,
On Wed, Aug 16, 2017 at 05:47:13PM +0800, Peter Xu wrote: > On Tue, Aug 15, 2017 at 05:47:08PM +0800, Peter Xu wrote: > > On Tue, Aug 15, 2017 at 10:27:07AM +0100, Daniel P. Berrange wrote: > > > On Tue, Aug 15, 2017 at 04:50:06PM +0800, Peter Xu wrote: > > > > On Tue, Aug 15, 2017 at 09:37:14AM +0100, Daniel P. Berrange wrote: > > > > > On Tue, Aug 15, 2017 at 02:17:06PM +0800, Peter Xu wrote: > > > > > > Store the task tag for migration types: tcp/unix/fd/exec in current > > > > > > MigrationIncomingState struct. > > > > > > > > > > > > For defered migration, no need to store task tag since there is no task > > > > > > running in the main loop at all. For RDMA, let's mark it as todo. > > > > > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > --- > > > > > > migration/migration.c | 22 ++++++++++++++++++---- > > > > > > migration/migration.h | 2 ++ > > > > > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > > > index c9b7085..daf356b 100644 > > > > > > --- a/migration/migration.c > > > > > > +++ b/migration/migration.c > > > > > > @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) > > > > > > mis->from_src_file = NULL; > > > > > > } > > > > > > > > > > > > + mis->listen_task_tag = 0; > > > > > > qemu_event_destroy(&mis->main_thread_load_event); > > > > > > } > > > > > > > > > > > > @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, > > > > > > void qemu_start_incoming_migration(const char *uri, Error **errp) > > > > > > { > > > > > > const char *p; > > > > > > + guint task_tag = 0; > > > > > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > > > > > > > > > > > qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); > > > > > > if (!strcmp(uri, "defer")) { > > > > > > deferred_incoming_migration(errp); > > > > > > } else if (strstart(uri, "tcp:", &p)) { > > > > > > - tcp_start_incoming_migration(p, errp); > > > > > > + task_tag = tcp_start_incoming_migration(p, errp); > > > > > > #ifdef CONFIG_RDMA > > > > > > } else if (strstart(uri, "rdma:", &p)) { > > > > > > + /* TODO: store task tag for RDMA migrations */ > > > > > > rdma_start_incoming_migration(p, errp); > > > > > > #endif > > > > > > } else if (strstart(uri, "exec:", &p)) { > > > > > > - exec_start_incoming_migration(p, errp); > > > > > > + task_tag = exec_start_incoming_migration(p, errp); > > > > > > } else if (strstart(uri, "unix:", &p)) { > > > > > > - unix_start_incoming_migration(p, errp); > > > > > > + task_tag = unix_start_incoming_migration(p, errp); > > > > > > } else if (strstart(uri, "fd:", &p)) { > > > > > > - fd_start_incoming_migration(p, errp); > > > > > > + task_tag = fd_start_incoming_migration(p, errp); > > > > > > } else { > > > > > > error_setg(errp, "unknown migration protocol: %s", uri); > > > > > > + return; > > > > > > } > > > > > > + > > > > > > + mis->listen_task_tag = task_tag; > > > > > > > > > > This is unsafe as currently written. The main loop watches that are > > > > > associated with these tags are all unregistered when incoming migration > > > > > starts. So if you save them, and then unregister later when postcopy > > > > > is "stuck", then you're likely unregistering a tag associated with > > > > > some other random part of QEMU, because the saved value is no longer > > > > > valid. > > > > > > > > IIUC for incoming migration, the watch is not detached until > > > > migration_fd_process_incoming() completes. So: > > > > > > > > - for precopy, the watch should be detached when migration completes > > > > > > > > - for postcopy, the watch should be detached when postcopy starts, > > > > then main loop thread returns, while the ram_load_thread starts to > > > > continue the migration. > > > > > > > > So basically the watch is detaching itself after > > > > migration_fd_process_incoming() completes. To handle that, this patch > > > > has this change: > > > > > > > > @@ -422,6 +429,13 @@ void migration_fd_process_incoming(QEMUFile *f) > > > > co = qemu_coroutine_create(process_incoming_migration_co, f); > > > > qemu_coroutine_enter(co); > > > > } > > > > + > > > > + /* > > > > + * When reach here, we should not need the listening port any > > > > + * more. We'll detach the listening task soon, let's reset the > > > > + * listen task tag. > > > > + */ > > > > + mis->listen_task_tag = 0; > > > > > > > > Would this make sure the listen_task_tag is always valid if nonzero? > > > > > > Mixing 'return FALSE' from callbacks, with g_source_remove is a recipe > > > for hard to diagnose bugs, so should be avoided in general. > > > > Makes sense. > > > > > > > > So, IMHO, you should change all the callbacks to 'return TRUE' so that > > > they keep the watch registered, and then explicitly call g_source_remove() > > > passing the listen tag, when incoming migration starts. > > > > Yes this sounds better. Thanks! > > Hmm... when incoming migration starts, we are still in the handler of > the listening gsource. I am not sure whether it's safe to remove the > gsource in its own handlers. Yes, it should be safe to call g_source_remove from within the handler itself. Regards, Daniel
On Tue, Aug 29, 2017 at 11:38:31AM +0100, Daniel P. Berrange wrote: > > > > So, IMHO, you should change all the callbacks to 'return TRUE' so that > > > > they keep the watch registered, and then explicitly call g_source_remove() > > > > passing the listen tag, when incoming migration starts. > > > > > > Yes this sounds better. Thanks! > > > > Hmm... when incoming migration starts, we are still in the handler of > > the listening gsource. I am not sure whether it's safe to remove the > > gsource in its own handlers. > > Yes, it should be safe to call g_source_remove from within the handler > itself. Thanks for confirmation. Will post a new version soon.
diff --git a/migration/migration.c b/migration/migration.c index c9b7085..daf356b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -171,6 +171,7 @@ void migration_incoming_state_destroy(void) mis->from_src_file = NULL; } + mis->listen_task_tag = 0; qemu_event_destroy(&mis->main_thread_load_event); } @@ -265,25 +266,31 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname, void qemu_start_incoming_migration(const char *uri, Error **errp) { const char *p; + guint task_tag = 0; + MigrationIncomingState *mis = migration_incoming_get_current(); qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort); if (!strcmp(uri, "defer")) { deferred_incoming_migration(errp); } else if (strstart(uri, "tcp:", &p)) { - tcp_start_incoming_migration(p, errp); + task_tag = tcp_start_incoming_migration(p, errp); #ifdef CONFIG_RDMA } else if (strstart(uri, "rdma:", &p)) { + /* TODO: store task tag for RDMA migrations */ rdma_start_incoming_migration(p, errp); #endif } else if (strstart(uri, "exec:", &p)) { - exec_start_incoming_migration(p, errp); + task_tag = exec_start_incoming_migration(p, errp); } else if (strstart(uri, "unix:", &p)) { - unix_start_incoming_migration(p, errp); + task_tag = unix_start_incoming_migration(p, errp); } else if (strstart(uri, "fd:", &p)) { - fd_start_incoming_migration(p, errp); + task_tag = fd_start_incoming_migration(p, errp); } else { error_setg(errp, "unknown migration protocol: %s", uri); + return; } + + mis->listen_task_tag = task_tag; } static void process_incoming_migration_bh(void *opaque) @@ -422,6 +429,13 @@ void migration_fd_process_incoming(QEMUFile *f) co = qemu_coroutine_create(process_incoming_migration_co, f); qemu_coroutine_enter(co); } + + /* + * When reach here, we should not need the listening port any + * more. We'll detach the listening task soon, let's reset the + * listen task tag. + */ + mis->listen_task_tag = 0; } /* diff --git a/migration/migration.h b/migration/migration.h index d041369..1f4faef 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -26,6 +26,8 @@ /* State for the incoming migration */ struct MigrationIncomingState { QEMUFile *from_src_file; + /* Task tag for incoming listen port. Valid when >0. */ + guint listen_task_tag; /* * Free at the start of the main state load, set as the main thread finishes
Store the task tag for migration types: tcp/unix/fd/exec in current MigrationIncomingState struct. For defered migration, no need to store task tag since there is no task running in the main loop at all. For RDMA, let's mark it as todo. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 22 ++++++++++++++++++---- migration/migration.h | 2 ++ 2 files changed, 20 insertions(+), 4 deletions(-)