| Message ID | 20220331150857.74406-17-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
On Thu, Mar 31, 2022 at 11:08:54AM -0400, Peter Xu wrote: > This patch is based on the async preempt channel creation. It continues > wiring up the new channel with TLS handshake to destionation when enabled. > > Note that only the src QEMU needs such operation; the dest QEMU does not > need any change for TLS support due to the fact that all channels are > established synchronously there, so all the TLS magic is already properly > handled by migration_tls_channel_process_incoming(). > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/postcopy-ram.c | 60 +++++++++++++++++++++++++++++++++++----- > migration/trace-events | 1 + > 2 files changed, 54 insertions(+), 7 deletions(-) > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index ab2a50cf45..f5ba176862 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -36,6 +36,7 @@ > #include "socket.h" > #include "qemu-file-channel.h" > #include "yank_functions.h" > +#include "tls.h" > > /* Arbitrary limit on size of each discard command, > * keeps them around ~200 bytes > @@ -1552,15 +1553,15 @@ bool postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file) > return true; > } > > +/* > + * Setup the postcopy preempt channel with the IOC. If ERROR is specified, > + * setup the error instead. This helper will free the ERROR if specified. > + */ > static void > -postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) > +postcopy_preempt_send_channel_done(MigrationState *s, > + QIOChannel *ioc, Error *local_err) > { > - MigrationState *s = opaque; > - QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); > - Error *local_err = NULL; > - > - if (qio_task_propagate_error(task, &local_err)) { > - /* Something wrong happened.. */ > + if (local_err) { > migrate_set_error(s, local_err); > error_free(local_err); > } else { > @@ -1574,6 +1575,51 @@ postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) > * postcopy_qemufile_src to know whether it failed or not. > */ > qemu_sem_post(&s->postcopy_qemufile_src_sem); > +} > + > +static void > +postcopy_preempt_tls_handshake(QIOTask *task, gpointer opaque) > +{ > + MigrationState *s = opaque; > + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); If using g_autoptr(QIOChannel) ioc = ... > + Error *err = NULL; local_err is normal naming > + > + qio_task_propagate_error(task, &err); > + postcopy_preempt_send_channel_done(s, ioc, err); > + object_unref(OBJECT(ioc)); ...not needed with g_autoptr > +} > + > +static void > +postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) > +{ > + MigrationState *s = opaque; > + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); If you use g_autoptr(QIOChannel) > + QIOChannelTLS *tioc; > + Error *local_err = NULL; > + > + if (qio_task_propagate_error(task, &local_err)) { > + assert(local_err); I don't think we really need to add these asserts everywhere we handle a failure path do we ? > + goto out; > + } > + > + if (migrate_channel_requires_tls(ioc)) { > + tioc = migration_tls_client_create(s, ioc, s->hostname, &local_err); > + if (!tioc) { > + assert(local_err); > + goto out; > + } > + trace_postcopy_preempt_tls_handshake(); > + qio_channel_set_name(QIO_CHANNEL(tioc), "migration-tls-preempt"); > + qio_channel_tls_handshake(tioc, postcopy_preempt_tls_handshake, > + s, NULL, NULL); > + /* Setup the channel until TLS handshake finished */ > + object_unref(OBJECT(ioc)); ...not needed with g_autoptr > + return; > + } > + > +out: > + /* This handles both good and error cases */ > + postcopy_preempt_send_channel_done(s, ioc, local_err); > object_unref(OBJECT(ioc)); ...also not needed with g_autoptr > } > > diff --git a/migration/trace-events b/migration/trace-events > index b21d5f371f..00ab2e1b96 100644 > --- a/migration/trace-events > +++ b/migration/trace-events > @@ -287,6 +287,7 @@ postcopy_request_shared_page(const char *sharer, const char *rb, uint64_t rb_off > postcopy_request_shared_page_present(const char *sharer, const char *rb, uint64_t rb_offset) "%s already %s offset 0x%"PRIx64 > postcopy_wake_shared(uint64_t client_addr, const char *rb) "at 0x%"PRIx64" in %s" > postcopy_page_req_del(void *addr, int count) "resolved page req %p total %d" > +postcopy_preempt_tls_handshake(void) "" > postcopy_preempt_new_channel(void) "" > postcopy_preempt_thread_entry(void) "" > postcopy_preempt_thread_exit(void) "" > -- > 2.32.0 > With regards, Daniel
On Wed, Apr 20, 2022 at 12:35:21PM +0100, Daniel P. Berrangé wrote: > On Thu, Mar 31, 2022 at 11:08:54AM -0400, Peter Xu wrote: > > This patch is based on the async preempt channel creation. It continues > > wiring up the new channel with TLS handshake to destionation when enabled. > > > > Note that only the src QEMU needs such operation; the dest QEMU does not > > need any change for TLS support due to the fact that all channels are > > established synchronously there, so all the TLS magic is already properly > > handled by migration_tls_channel_process_incoming(). > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/postcopy-ram.c | 60 +++++++++++++++++++++++++++++++++++----- > > migration/trace-events | 1 + > > 2 files changed, 54 insertions(+), 7 deletions(-) > > > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > > index ab2a50cf45..f5ba176862 100644 > > --- a/migration/postcopy-ram.c > > +++ b/migration/postcopy-ram.c > > @@ -36,6 +36,7 @@ > > #include "socket.h" > > #include "qemu-file-channel.h" > > #include "yank_functions.h" > > +#include "tls.h" > > > > /* Arbitrary limit on size of each discard command, > > * keeps them around ~200 bytes > > @@ -1552,15 +1553,15 @@ bool postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file) > > return true; > > } > > > > +/* > > + * Setup the postcopy preempt channel with the IOC. If ERROR is specified, > > + * setup the error instead. This helper will free the ERROR if specified. > > + */ > > static void > > -postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) > > +postcopy_preempt_send_channel_done(MigrationState *s, > > + QIOChannel *ioc, Error *local_err) > > { > > - MigrationState *s = opaque; > > - QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); > > - Error *local_err = NULL; > > - > > - if (qio_task_propagate_error(task, &local_err)) { > > - /* Something wrong happened.. */ > > + if (local_err) { > > migrate_set_error(s, local_err); > > error_free(local_err); > > } else { > > @@ -1574,6 +1575,51 @@ postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) > > * postcopy_qemufile_src to know whether it failed or not. > > */ > > qemu_sem_post(&s->postcopy_qemufile_src_sem); > > +} > > + > > +static void > > +postcopy_preempt_tls_handshake(QIOTask *task, gpointer opaque) > > +{ > > + MigrationState *s = opaque; > > + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); > > If using g_autoptr(QIOChannel) ioc = ... New magic learned.. > > > + Error *err = NULL; > > local_err is normal naming OK. > > > + > > + qio_task_propagate_error(task, &err); > > + postcopy_preempt_send_channel_done(s, ioc, err); > > + object_unref(OBJECT(ioc)); > > ...not needed with g_autoptr > > > +} > > + > > +static void > > +postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) > > +{ > > + MigrationState *s = opaque; > > + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); > > If you use g_autoptr(QIOChannel) Will use it here too. > > > + QIOChannelTLS *tioc; > > + Error *local_err = NULL; > > + > > + if (qio_task_propagate_error(task, &local_err)) { > > + assert(local_err); > > I don't think we really need to add these asserts everywhere we > handle a failure path do we ? Maybe I'm just over-cautious, yeah let me drop those. Thanks,
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index ab2a50cf45..f5ba176862 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -36,6 +36,7 @@ #include "socket.h" #include "qemu-file-channel.h" #include "yank_functions.h" +#include "tls.h" /* Arbitrary limit on size of each discard command, * keeps them around ~200 bytes @@ -1552,15 +1553,15 @@ bool postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file) return true; } +/* + * Setup the postcopy preempt channel with the IOC. If ERROR is specified, + * setup the error instead. This helper will free the ERROR if specified. + */ static void -postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) +postcopy_preempt_send_channel_done(MigrationState *s, + QIOChannel *ioc, Error *local_err) { - MigrationState *s = opaque; - QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); - Error *local_err = NULL; - - if (qio_task_propagate_error(task, &local_err)) { - /* Something wrong happened.. */ + if (local_err) { migrate_set_error(s, local_err); error_free(local_err); } else { @@ -1574,6 +1575,51 @@ postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) * postcopy_qemufile_src to know whether it failed or not. */ qemu_sem_post(&s->postcopy_qemufile_src_sem); +} + +static void +postcopy_preempt_tls_handshake(QIOTask *task, gpointer opaque) +{ + MigrationState *s = opaque; + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); + Error *err = NULL; + + qio_task_propagate_error(task, &err); + postcopy_preempt_send_channel_done(s, ioc, err); + object_unref(OBJECT(ioc)); +} + +static void +postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque) +{ + MigrationState *s = opaque; + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task)); + QIOChannelTLS *tioc; + Error *local_err = NULL; + + if (qio_task_propagate_error(task, &local_err)) { + assert(local_err); + goto out; + } + + if (migrate_channel_requires_tls(ioc)) { + tioc = migration_tls_client_create(s, ioc, s->hostname, &local_err); + if (!tioc) { + assert(local_err); + goto out; + } + trace_postcopy_preempt_tls_handshake(); + qio_channel_set_name(QIO_CHANNEL(tioc), "migration-tls-preempt"); + qio_channel_tls_handshake(tioc, postcopy_preempt_tls_handshake, + s, NULL, NULL); + /* Setup the channel until TLS handshake finished */ + object_unref(OBJECT(ioc)); + return; + } + +out: + /* This handles both good and error cases */ + postcopy_preempt_send_channel_done(s, ioc, local_err); object_unref(OBJECT(ioc)); } diff --git a/migration/trace-events b/migration/trace-events index b21d5f371f..00ab2e1b96 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -287,6 +287,7 @@ postcopy_request_shared_page(const char *sharer, const char *rb, uint64_t rb_off postcopy_request_shared_page_present(const char *sharer, const char *rb, uint64_t rb_offset) "%s already %s offset 0x%"PRIx64 postcopy_wake_shared(uint64_t client_addr, const char *rb) "at 0x%"PRIx64" in %s" postcopy_page_req_del(void *addr, int count) "resolved page req %p total %d" +postcopy_preempt_tls_handshake(void) "" postcopy_preempt_new_channel(void) "" postcopy_preempt_thread_entry(void) "" postcopy_preempt_thread_exit(void) ""
This patch is based on the async preempt channel creation. It continues wiring up the new channel with TLS handshake to destionation when enabled. Note that only the src QEMU needs such operation; the dest QEMU does not need any change for TLS support due to the fact that all channels are established synchronously there, so all the TLS magic is already properly handled by migration_tls_channel_process_incoming(). Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/postcopy-ram.c | 60 +++++++++++++++++++++++++++++++++++----- migration/trace-events | 1 + 2 files changed, 54 insertions(+), 7 deletions(-)