[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 09/17] migration: Start of multiple fd work
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 09/17] migration: Start of multiple fd work |
Date: |
Mon, 13 Feb 2017 16:39:32 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> wrote:
> > * Juan Quintela (address@hidden) wrote:
> >> We create new channels for each new thread created. We only send through
> >> them a character to be sure that we are creating the channels in the
> >> right order.
> >>
> >> Note: Reference count/freeing of channels is not done
> >>
> >> Signed-off-by: Juan Quintela <address@hidden>
> >> ---
> >> include/migration/migration.h | 6 +++++
> >> migration/ram.c | 45 +++++++++++++++++++++++++++++++++-
> >> migration/socket.c | 56
> >> +++++++++++++++++++++++++++++++++++++++++--
> >> 3 files changed, 104 insertions(+), 3 deletions(-)
> >
> > One thing not direclt in here, you should probably look at the migration
> > cancel
> > code to get it to call shutdown() on all your extra sockets, it stops things
> > blocking in any one of them.
>
> Will do.
>
> >>
> >> diff --git a/include/migration/migration.h b/include/migration/migration.h
> >> index f119ba0..3989bd6 100644
> >> --- a/include/migration/migration.h
> >> +++ b/include/migration/migration.h
> >> @@ -22,6 +22,7 @@
> >> #include "qapi-types.h"
> >> #include "exec/cpu-common.h"
> >> #include "qemu/coroutine_int.h"
> >> +#include "io/channel.h"
> >>
> >> #define QEMU_VM_FILE_MAGIC 0x5145564d
> >> #define QEMU_VM_FILE_VERSION_COMPAT 0x00000002
> >> @@ -218,6 +219,11 @@ void tcp_start_incoming_migration(const char
> >> *host_port, Error **errp);
> >>
> >> void tcp_start_outgoing_migration(MigrationState *s, const char
> >> *host_port, Error **errp);
> >>
> >> +QIOChannel *socket_recv_channel_create(void);
> >> +int socket_recv_channel_destroy(QIOChannel *recv);
> >> +QIOChannel *socket_send_channel_create(void);
> >> +int socket_send_channel_destroy(QIOChannel *send);
> >> +
> >> void unix_start_incoming_migration(const char *path, Error **errp);
> >>
> >> void unix_start_outgoing_migration(MigrationState *s, const char *path,
> >> Error **errp);
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 939f364..5ad7cb3 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -386,9 +386,11 @@ void migrate_compress_threads_create(void)
> >>
> >> struct MultiFDSendParams {
> >> QemuThread thread;
> >> + QIOChannel *c;
> >> QemuCond cond;
> >> QemuMutex mutex;
> >> bool quit;
> >> + bool started;
> >> };
> >> typedef struct MultiFDSendParams MultiFDSendParams;
> >>
> >> @@ -397,6 +399,13 @@ static MultiFDSendParams *multifd_send;
> >> static void *multifd_send_thread(void *opaque)
> >> {
> >> MultiFDSendParams *params = opaque;
> >> + char start = 's';
> >> +
> >> + qio_channel_write(params->c, &start, 1, &error_abort);
> >
> > I'd be tempted to send something stronger as a guarantee
> > that you're connecting the right thing to the right place;
> > maybe something like a QEMU + UUID + fd index?
> > I guarantee someone is going to mess up the fd's in the wrong
> > order or connect some random other process to one of them.
>
> which UUID? I can put anything there.
I was thinking just something to stop two migrations getting mixed
up; but I see there is a qemu_uuid variable defined, might be as good
as anything.
> >> + qemu_mutex_lock(¶ms->mutex);
> >> + params->started = true;
> >> + qemu_cond_signal(¶ms->cond);
> >> + qemu_mutex_unlock(¶ms->mutex);
> >>
> >> qemu_mutex_lock(¶ms->mutex);
> >
> > That unlock/lock pair is odd.
>
> Fixed.
>
> >
> >> while (!params->quit){
> >> @@ -433,6 +442,7 @@ void migrate_multifd_send_threads_join(void)
> >> qemu_thread_join(&multifd_send[i].thread);
> >> qemu_mutex_destroy(&multifd_send[i].mutex);
> >> qemu_cond_destroy(&multifd_send[i].cond);
> >> + socket_send_channel_destroy(multifd_send[i].c);
> >> }
> >> g_free(multifd_send);
> >> multifd_send = NULL;
> >> @@ -452,18 +462,31 @@ void migrate_multifd_send_threads_create(void)
> >> qemu_mutex_init(&multifd_send[i].mutex);
> >> qemu_cond_init(&multifd_send[i].cond);
> >> multifd_send[i].quit = false;
> >> + multifd_send[i].started = false;
> >> + multifd_send[i].c = socket_send_channel_create();
> >> + if(!multifd_send[i].c) {
> >> + error_report("Error creating a send channel");
> >> + exit(0);
> >
> > Hmm no exit!
>
> I have to add the error path before to the callers :-(
>
>
> >
> > We need to be careful there since that's a sync; it depends what
> > calls this, if I'm reading the code above correctly then it gets called
> > from the main thread and that would be bad if it blocked; it's ok if it
> > was the fd threads or the migration thread though.
>
> I think it is from the migration thread, no?
> /me checks
Probably worth a comment saying which thread it comes from :-)
> I stand corrected. It is called from the main thread. It works if
> destination is not up. It segfaults if destination is launched but not
> conffigured for multithread.
I think I was more worried what happens if the destination or network
is blocked.
Dave
> Will post fix later.
>
> Later, Juan.
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK