Re: [Qemu-devel] [PATCH 09/17] migration: Start of multiple fd work

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) 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 <quintela@redhat.com>
> >> ---
> >>  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(&params->mutex);
> >> +    params->started = true;
> >> +    qemu_cond_signal(&params->cond);
> >> +    qemu_mutex_unlock(&params->mutex);
> >> 
> >>      qemu_mutex_lock(&params->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 / dgilbert@redhat.com / Manchester, UK