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

[Juan Quintela]

Peter Xu  wrote:
> On Tue, Aug 08, 2017 at 11:19:35AM +0200, Juan Quintela wrote:
>> Peter Xu  wrote:
>> > On Mon, Jul 17, 2017 at 03:42:30PM +0200, Juan Quintela wrote:
>> >
>> > [...]
>> >
>> >>  int multifd_load_setup(void)
>> >>  {
>> >>  int thread_count;
>> >> -uint8_t i;
>> >>  
>> >>  if (!migrate_use_multifd()) {
>> >>  return 0;
>> >>  }
>> >>  thread_count = migrate_multifd_threads();
>> >>  multifd_recv_state = g_malloc0(sizeof(*multifd_recv_state));
>> >> -multifd_recv_state->params = g_new0(MultiFDRecvParams, thread_count);
>> >> +multifd_recv_state->params = g_new0(MultiFDRecvParams *, 
>> >> thread_count);
>> >>  multifd_recv_state->count = 0;
>> >> -for (i = 0; i < thread_count; i++) {
>> >> -char thread_name[16];
>> >> -MultiFDRecvParams *p = _recv_state->params[i];
>> >> -
>> >> -qemu_mutex_init(>mutex);
>> >> -qemu_sem_init(>sem, 0);
>> >> -p->quit = false;
>> >> -p->id = i;
>> >> -snprintf(thread_name, sizeof(thread_name), "multifdrecv_%d", i);
>> >> -qemu_thread_create(>thread, thread_name, multifd_recv_thread, 
>> >> p,
>> >> -   QEMU_THREAD_JOINABLE);
>> >> -multifd_recv_state->count++;
>> >> -}
>> >
>> > Could I ask why we explicitly switched from MultiFDRecvParams[] array
>> > into a pointer array? Can we still use the old array?  Thanks,
>> 
>> Now, we could receive the channels out of order (the wonders of
>> networking).  So, we have two options that I can see:
>> 
>> * Add interesting global locking to be able to modify inplace (I know
>>   that it should be safe, but yet).
>> * Create a new struct in the new connection, and then atomically switch
>>   the pointer to the right instruction.
>> 
>> I can assure you that the second one makes it much more easier to detect
>> when you use the "channel" before you have fully created it O:-)
>
> Oh, so it's possible that we start to recv pages even if the recv
> channel has not yet been established...
>
> Then would current code be problematic? Like in multifd_recv_page() we
> have:
>
> static void multifd_recv_page(uint8_t *address, uint16_t fd_num)
> {
> ...
> p = multifd_recv_state->params[fd_num];
> qemu_sem_wait(>ready);
> ...
> }
>
> Here can p==NULL if channel is not ready yet?
>
> (If so, I think a static array makes more sense...)

Yeap.  If we make an error (and believe me that I did), we  get a "nice"
segmentation fault, where we can see what fd_num is.  Otherwise we
receive a hang qemu.  I know what I preffer O:-)

Later, Juan.

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

[Peter Xu]

On Tue, Aug 08, 2017 at 11:19:35AM +0200, Juan Quintela wrote:
> Peter Xu  wrote:
> > On Mon, Jul 17, 2017 at 03:42:30PM +0200, Juan Quintela wrote:
> >
> > [...]
> >
> >>  int multifd_load_setup(void)
> >>  {
> >>  int thread_count;
> >> -uint8_t i;
> >>  
> >>  if (!migrate_use_multifd()) {
> >>  return 0;
> >>  }
> >>  thread_count = migrate_multifd_threads();
> >>  multifd_recv_state = g_malloc0(sizeof(*multifd_recv_state));
> >> -multifd_recv_state->params = g_new0(MultiFDRecvParams, thread_count);
> >> +multifd_recv_state->params = g_new0(MultiFDRecvParams *, 
> >> thread_count);
> >>  multifd_recv_state->count = 0;
> >> -for (i = 0; i < thread_count; i++) {
> >> -char thread_name[16];
> >> -MultiFDRecvParams *p = _recv_state->params[i];
> >> -
> >> -qemu_mutex_init(>mutex);
> >> -qemu_sem_init(>sem, 0);
> >> -p->quit = false;
> >> -p->id = i;
> >> -snprintf(thread_name, sizeof(thread_name), "multifdrecv_%d", i);
> >> -qemu_thread_create(>thread, thread_name, multifd_recv_thread, 
> >> p,
> >> -   QEMU_THREAD_JOINABLE);
> >> -multifd_recv_state->count++;
> >> -}
> >
> > Could I ask why we explicitly switched from MultiFDRecvParams[] array
> > into a pointer array? Can we still use the old array?  Thanks,
> 
> Now, we could receive the channels out of order (the wonders of
> networking).  So, we have two options that I can see:
> 
> * Add interesting global locking to be able to modify inplace (I know
>   that it should be safe, but yet).
> * Create a new struct in the new connection, and then atomically switch
>   the pointer to the right instruction.
> 
> I can assure you that the second one makes it much more easier to detect
> when you use the "channel" before you have fully created it O:-)

Oh, so it's possible that we start to recv pages even if the recv
channel has not yet been established...

Then would current code be problematic? Like in multifd_recv_page() we
have:

static void multifd_recv_page(uint8_t *address, uint16_t fd_num)
{
...
p = multifd_recv_state->params[fd_num];
qemu_sem_wait(>ready);
...
}

Here can p==NULL if channel is not ready yet?

(If so, I think a static array makes more sense...)

Thanks,

-- 
Peter Xu

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

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert"  wrote:
> > * Juan Quintela (quint...@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.
> >
> > That text is out of date isn't it?
> 
> oops, fixed.
> 
> 
> >> +gboolean multifd_new_channel(QIOChannel *ioc)
> >> +{
> >> +int thread_count = migrate_multifd_threads();
> >> +MultiFDRecvParams *p = g_new0(MultiFDRecvParams, 1);
> >> +MigrationState *s = migrate_get_current();
> >> +char string[MULTIFD_UUID_MSG];
> >> +char string_uuid[UUID_FMT_LEN];
> >> +char *uuid;
> >> +int id;
> >> +
> >> +qio_channel_read(ioc, string, sizeof(string), _abort);
> >> +sscanf(string, "%s multifd %03d", string_uuid, );
> >> +
> >> +if (qemu_uuid_set) {
> >> +uuid = qemu_uuid_unparse_strdup(_uuid);
> >> +} else {
> >> +uuid = g_strdup(multifd_uuid);
> >> +}
> >> +if (strcmp(string_uuid, uuid)) {
> >> +error_report("multifd: received uuid '%s' and expected uuid '%s'",
> >> + string_uuid, uuid);
> >
> > probably worth adding the channel id as well so we can see
> > when it fails.
> 
> Done.
> 
> >> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
> >> +  MIGRATION_STATUS_FAILED);
> >> +terminate_multifd_recv_threads();
> >> +return FALSE;
> >> +}
> >> +g_free(uuid);
> >> +
> >> +if (multifd_recv_state->params[id] != NULL) {
> >> +error_report("multifd: received id '%d' is already setup'", id);
> >> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
> >> +  MIGRATION_STATUS_FAILED);
> >> +terminate_multifd_recv_threads();
> >> +return FALSE;
> >> +}
> >> +qemu_mutex_init(>mutex);
> >> +qemu_sem_init(>sem, 0);
> >> +p->quit = false;
> >> +p->id = id;
> >> +p->c = ioc;
> >> +atomic_set(_recv_state->params[id], p);
> >
> > Can you explain why this is quite so careful about ordering ? Is there
> > something that could look at params or try and take the mutex before
> > the count is incremented?
> 
> what happened to me in the middle stages of the patches (yes, doing
> asynchronously was painful) was that:
> 
> I created the threads (at the beggining I did the
> multifd_recv_state->params[id] == p inside the thread, that makes things
> really, really racy.  I *think* that now we could probably do this
> as you state.
> 
> 
> 
> > I think it's safe to do:
> >  p->quit = false;
> >  p->id = id;
> >  p->c = ioc;
> >  _recv_state->params[id] = p;
> >  qemu_sem_init(>sem, 0);
> >  qemu_mutex_init(>mutex);
> >  qemu_thread_create(...)
> >  atomic_inc(_recv_state->count);<-- I'm not sure if this
> >  needs to be atomic
> 
> We only change it on the main thread, so it should be enough.  The split
> that I want to do is:
> 
> we do the listen asynchronously
> when something arrives, we just read it (main thread)
> we then read   
> and then after checking that uuid is right, we call whatever function we
> have for "string", in our case "multifd", with  as one string
> parameters.
> 
> This should make it easier to create new "channels" for other purposes.
> So far so good.
> 
> But then it appears what are the responsabilities, At the beggining, I
> read the string on the reception thread for that channel, that created a
> race because I received the 1st message for that channel before the
> channel was fully created (yes, it only happened sometimes, easy to
> understand after debugging).  This is the main reason that I changed to
> an array of pointers to structs instead of one array of structs.
> 
> Then, I had to ve very careful to know when I had created all the
> channels threads, because otherwise I ended having races left and right.
> 
> I will try to test the ordering that you suggested.
> 
> >> +qemu_thread_create(>thread, "multifd_recv", multifd_recv_thread, p,
> >> +   QEMU_THREAD_JOINABLE);
> >
> > You've lost the nice numbered thread names you had created in the
> > previous version of this that you're removing.
> 
> I could get them back, but they really were not showing at gdb, where do
> they show? ps?

If you start qemu with -name debug-threads=on they show up in gdb's
info threads
also in top (hit H) and ps if you turn on the right optioa (H as well?)n.

> >> +multifd_recv_state->count++;
> >> +
> >> +/* We need to return FALSE for the last channel */
> >> +if (multifd_recv_state->count == thread_count) {
> >> +return FALSE;
> >> +} else {
> >> +return TRUE;
> >> +}
> >
> > return multifd_recv_state->count != thread_count;   ?
> 
> For other reasons I change this functions and now they use a different
> way of setting/checking if we have finished.  Look at the new series.
> 
> I didn't do as you 

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

[Juan Quintela]

"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@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.
>
> That text is out of date isn't it?

oops, fixed.


>> +gboolean multifd_new_channel(QIOChannel *ioc)
>> +{
>> +int thread_count = migrate_multifd_threads();
>> +MultiFDRecvParams *p = g_new0(MultiFDRecvParams, 1);
>> +MigrationState *s = migrate_get_current();
>> +char string[MULTIFD_UUID_MSG];
>> +char string_uuid[UUID_FMT_LEN];
>> +char *uuid;
>> +int id;
>> +
>> +qio_channel_read(ioc, string, sizeof(string), _abort);
>> +sscanf(string, "%s multifd %03d", string_uuid, );
>> +
>> +if (qemu_uuid_set) {
>> +uuid = qemu_uuid_unparse_strdup(_uuid);
>> +} else {
>> +uuid = g_strdup(multifd_uuid);
>> +}
>> +if (strcmp(string_uuid, uuid)) {
>> +error_report("multifd: received uuid '%s' and expected uuid '%s'",
>> + string_uuid, uuid);
>
> probably worth adding the channel id as well so we can see
> when it fails.

Done.

>> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
>> +  MIGRATION_STATUS_FAILED);
>> +terminate_multifd_recv_threads();
>> +return FALSE;
>> +}
>> +g_free(uuid);
>> +
>> +if (multifd_recv_state->params[id] != NULL) {
>> +error_report("multifd: received id '%d' is already setup'", id);
>> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
>> +  MIGRATION_STATUS_FAILED);
>> +terminate_multifd_recv_threads();
>> +return FALSE;
>> +}
>> +qemu_mutex_init(>mutex);
>> +qemu_sem_init(>sem, 0);
>> +p->quit = false;
>> +p->id = id;
>> +p->c = ioc;
>> +atomic_set(_recv_state->params[id], p);
>
> Can you explain why this is quite so careful about ordering ? Is there
> something that could look at params or try and take the mutex before
> the count is incremented?

what happened to me in the middle stages of the patches (yes, doing
asynchronously was painful) was that:

I created the threads (at the beggining I did the
multifd_recv_state->params[id] == p inside the thread, that makes things
really, really racy.  I *think* that now we could probably do this
as you state.



> I think it's safe to do:
>  p->quit = false;
>  p->id = id;
>  p->c = ioc;
>  _recv_state->params[id] = p;
>  qemu_sem_init(>sem, 0);
>  qemu_mutex_init(>mutex);
>  qemu_thread_create(...)
>  atomic_inc(_recv_state->count);<-- I'm not sure if this
>  needs to be atomic

We only change it on the main thread, so it should be enough.  The split
that I want to do is:

we do the listen asynchronously
when something arrives, we just read it (main thread)
we then read   
and then after checking that uuid is right, we call whatever function we
have for "string", in our case "multifd", with  as one string
parameters.

This should make it easier to create new "channels" for other purposes.
So far so good.

But then it appears what are the responsabilities, At the beggining, I
read the string on the reception thread for that channel, that created a
race because I received the 1st message for that channel before the
channel was fully created (yes, it only happened sometimes, easy to
understand after debugging).  This is the main reason that I changed to
an array of pointers to structs instead of one array of structs.

Then, I had to ve very careful to know when I had created all the
channels threads, because otherwise I ended having races left and right.

I will try to test the ordering that you suggested.

>> +qemu_thread_create(>thread, "multifd_recv", multifd_recv_thread, p,
>> +   QEMU_THREAD_JOINABLE);
>
> You've lost the nice numbered thread names you had created in the
> previous version of this that you're removing.

I could get them back, but they really were not showing at gdb, where do
they show? ps?

>> +multifd_recv_state->count++;
>> +
>> +/* We need to return FALSE for the last channel */
>> +if (multifd_recv_state->count == thread_count) {
>> +return FALSE;
>> +} else {
>> +return TRUE;
>> +}
>
> return multifd_recv_state->count != thread_count;   ?

For other reasons I change this functions and now they use a different
way of setting/checking if we have finished.  Look at the new series.

I didn't do as you said because I feel it weird that we return a bool
when we expert a gboolean, but .

Thanks, Juan.

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

[Juan Quintela]

Peter Xu  wrote:
> On Mon, Jul 17, 2017 at 03:42:30PM +0200, Juan Quintela wrote:
>
> [...]
>
>>  int multifd_load_setup(void)
>>  {
>>  int thread_count;
>> -uint8_t i;
>>  
>>  if (!migrate_use_multifd()) {
>>  return 0;
>>  }
>>  thread_count = migrate_multifd_threads();
>>  multifd_recv_state = g_malloc0(sizeof(*multifd_recv_state));
>> -multifd_recv_state->params = g_new0(MultiFDRecvParams, thread_count);
>> +multifd_recv_state->params = g_new0(MultiFDRecvParams *, thread_count);
>>  multifd_recv_state->count = 0;
>> -for (i = 0; i < thread_count; i++) {
>> -char thread_name[16];
>> -MultiFDRecvParams *p = _recv_state->params[i];
>> -
>> -qemu_mutex_init(>mutex);
>> -qemu_sem_init(>sem, 0);
>> -p->quit = false;
>> -p->id = i;
>> -snprintf(thread_name, sizeof(thread_name), "multifdrecv_%d", i);
>> -qemu_thread_create(>thread, thread_name, multifd_recv_thread, p,
>> -   QEMU_THREAD_JOINABLE);
>> -multifd_recv_state->count++;
>> -}
>
> Could I ask why we explicitly switched from MultiFDRecvParams[] array
> into a pointer array? Can we still use the old array?  Thanks,

Now, we could receive the channels out of order (the wonders of
networking).  So, we have two options that I can see:

* Add interesting global locking to be able to modify inplace (I know
  that it should be safe, but yet).
* Create a new struct in the new connection, and then atomically switch
  the pointer to the right instruction.

I can assure you that the second one makes it much more easier to detect
when you use the "channel" before you have fully created it O:-)

Later, Juan.

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

[Peter Xu]

On Mon, Jul 17, 2017 at 03:42:30PM +0200, Juan Quintela wrote:

[...]

>  int multifd_load_setup(void)
>  {
>  int thread_count;
> -uint8_t i;
>  
>  if (!migrate_use_multifd()) {
>  return 0;
>  }
>  thread_count = migrate_multifd_threads();
>  multifd_recv_state = g_malloc0(sizeof(*multifd_recv_state));
> -multifd_recv_state->params = g_new0(MultiFDRecvParams, thread_count);
> +multifd_recv_state->params = g_new0(MultiFDRecvParams *, thread_count);
>  multifd_recv_state->count = 0;
> -for (i = 0; i < thread_count; i++) {
> -char thread_name[16];
> -MultiFDRecvParams *p = _recv_state->params[i];
> -
> -qemu_mutex_init(>mutex);
> -qemu_sem_init(>sem, 0);
> -p->quit = false;
> -p->id = i;
> -snprintf(thread_name, sizeof(thread_name), "multifdrecv_%d", i);
> -qemu_thread_create(>thread, thread_name, multifd_recv_thread, p,
> -   QEMU_THREAD_JOINABLE);
> -multifd_recv_state->count++;
> -}

Could I ask why we explicitly switched from MultiFDRecvParams[] array
into a pointer array? Can we still use the old array?  Thanks,

-- 
Peter Xu

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

[Dr. David Alan Gilbert]

* Juan Quintela (quint...@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.

That text is out of date isn't it?

> 
> Signed-off-by: Juan Quintela 
> 
> --
> Split SocketArgs into incoming and outgoing args
> 
> Use UUID's on the initial message, so we are sure we are connecting to
> the right channel.
> 
> Remove init semaphore.  Now that we use uuids on the init message, we
> know that this is our channel.
> 
> Fix recv socket destwroy, we were destroying send channels.
> This was very interesting, because we were using an unreferred object
> without problems.
> 
> Move to struct of pointers
> init channel sooner.
> split recv thread creation.
> listen on main thread
> ---
>  migration/migration.c |   7 ++-
>  migration/ram.c   | 118 
> ++
>  migration/ram.h   |   2 +
>  migration/socket.c|  38 ++--
>  migration/socket.h|  10 +
>  5 files changed, 152 insertions(+), 23 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b81c498..e1c79d5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -389,8 +389,13 @@ gboolean migration_ioc_process_incoming(QIOChannel *ioc)
>  QEMUFile *f = qemu_fopen_channel_input(ioc);
>  mis->from_src_file = f;
>  migration_fd_process_incoming(f);
> +if (!migrate_use_multifd()) {
> +return FALSE;
> +} else {
> +return TRUE;
> +}
>  }
> -return FALSE; /* unregister */
> +return multifd_new_channel(ioc);
>  }
>  
>  /*
> diff --git a/migration/ram.c b/migration/ram.c
> index 8e87533..b80f511 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -36,6 +36,7 @@
>  #include "xbzrle.h"
>  #include "ram.h"
>  #include "migration.h"
> +#include "socket.h"
>  #include "migration/register.h"
>  #include "migration/misc.h"
>  #include "qemu-file.h"
> @@ -46,6 +47,8 @@
>  #include "exec/ram_addr.h"
>  #include "qemu/rcu_queue.h"
>  #include "migration/colo.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/uuid.h"
>  
>  /***/
>  /* ram save/restore */
> @@ -361,6 +364,7 @@ static void compress_threads_save_setup(void)
>  struct MultiFDSendParams {
>  uint8_t id;
>  QemuThread thread;
> +QIOChannel *c;
>  QemuSemaphore sem;
>  QemuMutex mutex;
>  bool quit;
> @@ -401,6 +405,7 @@ void multifd_save_cleanup(void)
>  qemu_thread_join(>thread);
>  qemu_mutex_destroy(>mutex);
>  qemu_sem_destroy(>sem);
> +socket_send_channel_destroy(p->c);
>  }
>  g_free(multifd_send_state->params);
>  multifd_send_state->params = NULL;
> @@ -408,11 +413,38 @@ void multifd_save_cleanup(void)
>  multifd_send_state = NULL;
>  }
>  
> +/* Default uuid for multifd when qemu is not started with uuid */
> +static char multifd_uuid[] = "5c49fd7e-af88-4a07-b6e8-091fd696ad40";
> +/* strlen(multifd) + '-' +  + '-' +  UUID_FMT + '\0' */
> +#define MULTIFD_UUID_MSG (7 + 1 + 3 + 1 + UUID_FMT_LEN + 1)
> +
>  static void *multifd_send_thread(void *opaque)
>  {
>  MultiFDSendParams *p = opaque;
> +char string[MULTIFD_UUID_MSG];
> +char *string_uuid;
> +int res;
> +bool exit = false;
>  
> -while (true) {
> +if (qemu_uuid_set) {
> +string_uuid = qemu_uuid_unparse_strdup(_uuid);
> +} else {
> +string_uuid = g_strdup(multifd_uuid);
> +}
> +res = snprintf(string, MULTIFD_UUID_MSG, "%s multifd %03d",
> +   string_uuid, p->id);
> +g_free(string_uuid);
> +
> +/* -1 due to the wonders of '\0' accounting */
> +if (res != (MULTIFD_UUID_MSG - 1)) {
> +error_report("Multifd UUID message '%s' is not of right length",
> +string);
> +exit = true;
> +} else {
> +qio_channel_write(p->c, string, MULTIFD_UUID_MSG, _abort);
> +}
> +
> +while (!exit) {
>  qemu_mutex_lock(>mutex);
>  if (p->quit) {
>  qemu_mutex_unlock(>mutex);
> @@ -445,6 +477,12 @@ int multifd_save_setup(void)
>  qemu_sem_init(>sem, 0);
>  p->quit = false;
>  p->id = i;
> +p->c = socket_send_channel_create();
> +if (!p->c) {
> +error_report("Error creating a send channel");
> +multifd_save_cleanup();
> +return -1;
> +}
>  snprintf(thread_name, sizeof(thread_name), "multifdsend_%d", i);
>  qemu_thread_create(>thread, thread_name, multifd_send_thread, p,
> QEMU_THREAD_JOINABLE);
> @@ -456,6 +494,7 @@ int multifd_save_setup(void)
>  struct MultiFDRecvParams {
>  uint8_t id;
>  QemuThread thread;
> +QIOChannel *c;
>  QemuSemaphore sem;
>  QemuMutex mutex;
>  bool quit;
> @@ -463,7 +502,7 

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

[Daniel P. Berrange]

On Mon, Jul 17, 2017 at 03:42:30PM +0200, Juan Quintela 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.
> 
> Signed-off-by: Juan Quintela 
> 
> --
> Split SocketArgs into incoming and outgoing args
> 
> Use UUID's on the initial message, so we are sure we are connecting to
> the right channel.
> 
> Remove init semaphore.  Now that we use uuids on the init message, we
> know that this is our channel.
> 
> Fix recv socket destwroy, we were destroying send channels.
> This was very interesting, because we were using an unreferred object
> without problems.
> 
> Move to struct of pointers
> init channel sooner.
> split recv thread creation.
> listen on main thread
> ---
>  migration/migration.c |   7 ++-
>  migration/ram.c   | 118 
> ++
>  migration/ram.h   |   2 +
>  migration/socket.c|  38 ++--
>  migration/socket.h|  10 +
>  5 files changed, 152 insertions(+), 23 deletions(-)
> 


> diff --git a/migration/ram.c b/migration/ram.c
> index 8e87533..b80f511 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -408,11 +413,38 @@ void multifd_save_cleanup(void)
>  multifd_send_state = NULL;
>  }
>  
> +/* Default uuid for multifd when qemu is not started with uuid */
> +static char multifd_uuid[] = "5c49fd7e-af88-4a07-b6e8-091fd696ad40";
> +/* strlen(multifd) + '-' +  + '-' +  UUID_FMT + '\0' */
> +#define MULTIFD_UUID_MSG (7 + 1 + 3 + 1 + UUID_FMT_LEN + 1)

> +
>  static void *multifd_send_thread(void *opaque)
>  {
>  MultiFDSendParams *p = opaque;
> +char string[MULTIFD_UUID_MSG];
> +char *string_uuid;
> +int res;
> +bool exit = false;
>  
> -while (true) {
> +if (qemu_uuid_set) {
> +string_uuid = qemu_uuid_unparse_strdup(_uuid);
> +} else {
> +string_uuid = g_strdup(multifd_uuid);
> +}
> +res = snprintf(string, MULTIFD_UUID_MSG, "%s multifd %03d",
> +   string_uuid, p->id);

Just use  g_strdup_printf() here and avoid the error prone
logically for calculating the "correct"  buffer size.

> +g_free(string_uuid);
> +
> +/* -1 due to the wonders of '\0' accounting */
> +if (res != (MULTIFD_UUID_MSG - 1)) {
> +error_report("Multifd UUID message '%s' is not of right length",
> +string);
> +exit = true;
> +} else {
> +qio_channel_write(p->c, string, MULTIFD_UUID_MSG, _abort);

E, you can't have QEMU abort when there's an I/O error on the
a file descriptor. It needs to fail the migration cleanly.

> +}
> +
> +while (!exit) {
>  qemu_mutex_lock(>mutex);
>  if (p->quit) {
>  qemu_mutex_unlock(>mutex);

> +gboolean multifd_new_channel(QIOChannel *ioc)
> +{
> +int thread_count = migrate_multifd_threads();
> +MultiFDRecvParams *p = g_new0(MultiFDRecvParams, 1);
> +MigrationState *s = migrate_get_current();
> +char string[MULTIFD_UUID_MSG];
> +char string_uuid[UUID_FMT_LEN];
> +char *uuid;
> +int id;
> +
> +qio_channel_read(ioc, string, sizeof(string), _abort);

Again, we can't abort QEMU on I/O errors

> +sscanf(string, "%s multifd %03d", string_uuid, );
> +
> +if (qemu_uuid_set) {
> +uuid = qemu_uuid_unparse_strdup(_uuid);
> +} else {
> +uuid = g_strdup(multifd_uuid);
> +}
> +if (strcmp(string_uuid, uuid)) {
> +error_report("multifd: received uuid '%s' and expected uuid '%s'",
> + string_uuid, uuid);
> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
> +  MIGRATION_STATUS_FAILED);
> +terminate_multifd_recv_threads();
> +return FALSE;
> +}
> +g_free(uuid);
> +
> +if (multifd_recv_state->params[id] != NULL) {
> +error_report("multifd: received id '%d' is already setup'", id);
> +migrate_set_state(>state, MIGRATION_STATUS_ACTIVE,
> +  MIGRATION_STATUS_FAILED);
> +terminate_multifd_recv_threads();
> +return FALSE;
> +}
> +qemu_mutex_init(>mutex);
> +qemu_sem_init(>sem, 0);
> +p->quit = false;
> +p->id = id;
> +p->c = ioc;
> +atomic_set(_recv_state->params[id], p);
> +qemu_thread_create(>thread, "multifd_recv", multifd_recv_thread, p,
> +   QEMU_THREAD_JOINABLE);
> +multifd_recv_state->count++;
> +
> +/* We need to return FALSE for the last channel */
> +if (multifd_recv_state->count == thread_count) {
> +return FALSE;
> +} else {
> +return TRUE;
> +}
> +}
> +

> diff --git a/migration/socket.c b/migration/socket.c
> index 6195596..32a6b39 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -26,6 +26,38 @@
>  #include "io/channel-socket.h"
>  #include "trace.h"
>  
> +int socket_recv_channel_destroy(QIOChannel *recv)
> +{
> +