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

[Juan Quintela <quintela@redhat.com>]

"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.

>> +    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

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.

Will post fix later.

Later, Juan.