Re: [Qemu-devel] [PATCH v6 11/19] migration: Start of multiple fd work
"Daniel P. Berrange" wrote:
> On Tue, Aug 08, 2017 at 06:26:21PM +0200, Juan Quintela wrote:
>> We create new channels for each new thread created. We send through
>> them a string containing multifd so we are
>> sure that we connect the right channels in both sides.
>>
>> Signed-off-by: Juan Quintela
>>
>> +/* Default uuid for multifd when qemu is not started with uuid */
>> +static char multifd_uuid[] = "5c49fd7e-af88-4a07-b6e8-091fd696ad40";
>
> Why is this better than just using the qemu_uuid unconditionally.
> UUIC, it'll just be ----.
>
> Either way you've got a non-unique UUID if multiple QEMUs are
> started, so I dont see a benefit in inventing a new uuid here.
I hate a message full of zeros, it is the default value. If you have
more than one qemu and you don't set uuid, you are asking for trouble.
But if people preffer the 0 uuid, it is also ok with me.
>
>> +/* 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;
>> +char *string_uuid;
>> +
>> +if (qemu_uuid_set) {
>> +string_uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
>> +} else {
>> +string_uuid = g_strdup(multifd_uuid);
>> +}
>> +string = g_strdup_printf("%s multifd %03d", string_uuid, p->id);
>> +g_free(string_uuid);
>> +qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
>
> Must check return code here as it can do a short write, which won't
> trigger error_abort. Also you must not use error_abort at all. QEMU
> must not abort when migration hits an I/O error as that needlessly
> kills the user's VM.
>
> I also think it is not nice to be formatting a string with printf
> here, sending it and then using scanf to extract the data. If we
> need to send structured data, then we should define a proper binary
> format for it eg
>
> struct MigrateUUIDMsg {
> uint32_t chnanelid;
> QemuUUID uuid;
> } __attribute__((__packed__));
>
> and then just send the raw struct across.
Not that I believe that it still works (or that it worked ever), but
qemu migration "on stream" was supposed to be Endian safe .
>> +g_free(string);
>>
>> while (true) {
>> qemu_mutex_lock(&p->mutex);
>> @@ -445,6 +467,12 @@ int multifd_save_setup(void)
>> qemu_sem_init(&p->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;
>> +}
>
> We should pass an 'Error *' object into socket_send_channel_create()
> so that we can receive & report the actual error message, instead
> of a useless generic message.
Yeap, that is what I told was doing next on the Cover letter.
Just that it means changing lots of functions prototypes
>> +qio_channel_read(ioc, string, sizeof(string), &error_abort);
>
> Must not use error_abort which kills the whole VM ungracefully
Changing also that.
>
>> +sscanf(string, "%s multifd %03d", string_uuid, &id);
>> +
>> +if (qemu_uuid_set) {
>> +uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
>> +} else {
>> +uuid = g_strdup(multifd_uuid);
>> +}
>> +if (strcmp(string_uuid, uuid)) {
>> +error_report("multifd: received uuid '%s' and expected uuid '%s'"
>> + " for channel %d", string_uuid, uuid, id);
>> +migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>> + MIGRATION_STATUS_FAILED);
>> +terminate_multifd_recv_threads();
>> +return;
>> +}
>> +g_free(uuid);
>
> As mentioned above, we should receive a binary struct here instead
> of playing games with scanf.
>> @@ -534,22 +612,15 @@ int multifd_load_setup(void)
>> multifd_recv_state = g_malloc0(sizeof(*multifd_recv_state));
>> 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 = multifd_recv_state->params[i];
>> -
>> -qemu_mutex_init(&p->mutex);
>> -qemu_sem_init(&p->sem, 0);
>> -p->quit = false;
>> -p->id = i;
>> -snprintf(thread_name, sizeof(thread_name), "multifdrecv_%d", i);
>> -qemu_thread_create(&p->thread, thread_name, multifd_recv_thread, p,
>> - QEMU_THREAD_JOINABLE);
>> -multifd_recv_state->count++;
>> -}
>
> It us a little strange to be deleting a bunch of code you just added in the
> previous patch
We create the send/recv channels on previous patch. And then we switch
to create them as we receive the connections. I will try to see if
creating first the rec
Re: [Qemu-devel] [PATCH v6 11/19] migration: Start of multiple fd work
On Mon, Aug 14, 2017 at 03:43:58PM +0200, Juan Quintela wrote:
> "Daniel P. Berrange" wrote:
> > On Tue, Aug 08, 2017 at 06:26:21PM +0200, Juan Quintela wrote:
> >> We create new channels for each new thread created. We send through
> >> them a string containing multifd so we are
> >> sure that we connect the right channels in both sides.
> >>
> >> Signed-off-by: Juan Quintela
> >>
>
> >> +/* Default uuid for multifd when qemu is not started with uuid */
> >> +static char multifd_uuid[] = "5c49fd7e-af88-4a07-b6e8-091fd696ad40";
> >
> > Why is this better than just using the qemu_uuid unconditionally.
> > UUIC, it'll just be ----.
> >
> > Either way you've got a non-unique UUID if multiple QEMUs are
> > started, so I dont see a benefit in inventing a new uuid here.
>
> I hate a message full of zeros, it is the default value. If you have
> more than one qemu and you don't set uuid, you are asking for trouble.
>
> But if people preffer the 0 uuid, it is also ok with me.
I don't see a fixed UUID of all-zeros being any different from
a fixed UUID hardcoded in the source code here. Both are simply
conceptually broken in the same way if you care about distinguishing
VMs.
> > I also think it is not nice to be formatting a string with printf
> > here, sending it and then using scanf to extract the data. If we
> > need to send structured data, then we should define a proper binary
> > format for it eg
> >
> > struct MigrateUUIDMsg {
> > uint32_t chnanelid;
> > QemuUUID uuid;
> > } __attribute__((__packed__));
> >
> > and then just send the raw struct across.
>
> Not that I believe that it still works (or that it worked ever), but
> qemu migration "on stream" was supposed to be Endian safe .
The uuid field is just a byte array, so not endian sensitive. For
the channelid, just do a hton / ntoh call before/after writing/reading
the struct.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v6 11/19] migration: Start of multiple fd work
On Tue, Aug 08, 2017 at 06:26:21PM +0200, Juan Quintela wrote:
> We create new channels for each new thread created. We send through
> them a string containing multifd so we are
> sure that we connect the right channels in both sides.
>
> 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
> We count the number of created threads to know when we need to stop listening
> Use g_strdup_printf
> report channel id on errors
> ---
> migration/migration.c | 5 +++
> migration/ram.c | 99
> +++
> migration/ram.h | 3 ++
> migration/socket.c| 36 ++-
> migration/socket.h| 10 ++
> 5 files changed, 138 insertions(+), 15 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index e36e880..944d6e2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -413,6 +413,11 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
> */
> bool migration_has_all_channels(void)
> {
> +if (migrate_use_multifd()) {
> +int thread_count = migrate_multifd_threads();
> +
> +return thread_count == multifd_created_threads();
> +}
> return true;
> }
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 9fb3496..e9fa556 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(&p->thread);
> qemu_mutex_destroy(&p->mutex);
> qemu_sem_destroy(&p->sem);
> +socket_send_channel_destroy(p->c);
> }
> g_free(multifd_send_state->params);
> multifd_send_state->params = NULL;
> @@ -408,9 +413,26 @@ 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";
Why is this better than just using the qemu_uuid unconditionally.
UUIC, it'll just be ----.
Either way you've got a non-unique UUID if multiple QEMUs are
started, so I dont see a benefit in inventing a new uuid here.
> +/* 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;
> +char *string_uuid;
> +
> +if (qemu_uuid_set) {
> +string_uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
> +} else {
> +string_uuid = g_strdup(multifd_uuid);
> +}
> +string = g_strdup_printf("%s multifd %03d", string_uuid, p->id);
> +g_free(string_uuid);
> +qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
Must check return code here as it can do a short write, which won't
trigger error_abort. Also you must not use error_abort at all. QEMU
must not abort when migration hits an I/O error as that needlessly
kills the user's VM.
I also think it is not nice to be formatting a string with printf
here, sending it and then using scanf to extract the data. If we
need to send structured data, then we should define a proper binary
format for it eg
struct MigrateUUIDMsg {
uint32_t chnanelid;
QemuUUID uuid;
} __attribute__((__packed__));
and then just send the raw struct across.
> +g_free(string);
>
> while (true) {
> qemu_mutex_lock(&p->mutex);
> @@ -445,6 +467,12 @@ int multifd_save_setup(void)
> qemu_sem_init(&p->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;
> +}
We sho
[Qemu-devel] [PATCH v6 11/19] migration: Start of multiple fd work
We create new channels for each new thread created. We send through
them a string containing multifd so we are
sure that we connect the right channels in both sides.
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
We count the number of created threads to know when we need to stop listening
Use g_strdup_printf
report channel id on errors
---
migration/migration.c | 5 +++
migration/ram.c | 99 +++
migration/ram.h | 3 ++
migration/socket.c| 36 ++-
migration/socket.h| 10 ++
5 files changed, 138 insertions(+), 15 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index e36e880..944d6e2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -413,6 +413,11 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
*/
bool migration_has_all_channels(void)
{
+if (migrate_use_multifd()) {
+int thread_count = migrate_multifd_threads();
+
+return thread_count == multifd_created_threads();
+}
return true;
}
diff --git a/migration/ram.c b/migration/ram.c
index 9fb3496..e9fa556 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(&p->thread);
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
+socket_send_channel_destroy(p->c);
}
g_free(multifd_send_state->params);
multifd_send_state->params = NULL;
@@ -408,9 +413,26 @@ 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;
+char *string_uuid;
+
+if (qemu_uuid_set) {
+string_uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
+} else {
+string_uuid = g_strdup(multifd_uuid);
+}
+string = g_strdup_printf("%s multifd %03d", string_uuid, p->id);
+g_free(string_uuid);
+qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
+g_free(string);
while (true) {
qemu_mutex_lock(&p->mutex);
@@ -445,6 +467,12 @@ int multifd_save_setup(void)
qemu_sem_init(&p->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(&p->thread, thread_name, multifd_send_thread, p,
QEMU_THREAD_JOINABLE);
@@ -456,6 +484,7 @@ int multifd_save_setup(void)
struct MultiFDRecvParams {
uint8_t id;
QemuThread thread;
+QIOChannel *c;
QemuSemaphore sem;
QemuMutex mutex;
bool quit;
@@ -466,12 +495,16 @@ struct {
MultiFDRecvParams **params;
/* number of created threads */
int count;
+/* Should we finish */
+bool quit;
} *multifd_recv_state;
static void terminate_multifd_recv_threads(void)
{
int i;
+multifd_recv_state->quit = true;
+
for (i = 0; i < multifd_recv_state->count; i++) {
MultiFDRecvParams *p = multifd_recv_state->params[i];
@@ -496,6 +529,7 @@ void multifd_load_cleanup(void)
qemu_thread_join(&p->thread);
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
+socket_recv_channel_destroy(p->c);
g_free(p);
multifd_recv_state->params[i] = NULL;
}
@@ -522,10 +556,54 @@ static void *multifd_recv_thread(void *opaque
[Qemu-devel] [PATCH v6 11/19] migration: Start of multiple fd work
We create new channels for each new thread created. We send through
them a string containing multifd so we are
sure that we connect the right channels in both sides.
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
We count the number of created threads to know when we need to stop listening
Use g_strdup_printf
report channel id on errors
---
migration/migration.c | 5 +++
migration/ram.c | 99 +++
migration/ram.h | 3 ++
migration/socket.c| 36 ++-
migration/socket.h| 10 ++
5 files changed, 138 insertions(+), 15 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index e36e880..944d6e2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -413,6 +413,11 @@ void migration_ioc_process_incoming(QIOChannel *ioc)
*/
bool migration_has_all_channels(void)
{
+if (migrate_use_multifd()) {
+int thread_count = migrate_multifd_threads();
+
+return thread_count == multifd_created_threads();
+}
return true;
}
diff --git a/migration/ram.c b/migration/ram.c
index 9fb3496..e9fa556 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(&p->thread);
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
+socket_send_channel_destroy(p->c);
}
g_free(multifd_send_state->params);
multifd_send_state->params = NULL;
@@ -408,9 +413,26 @@ 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;
+char *string_uuid;
+
+if (qemu_uuid_set) {
+string_uuid = qemu_uuid_unparse_strdup(&qemu_uuid);
+} else {
+string_uuid = g_strdup(multifd_uuid);
+}
+string = g_strdup_printf("%s multifd %03d", string_uuid, p->id);
+g_free(string_uuid);
+qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
+g_free(string);
while (true) {
qemu_mutex_lock(&p->mutex);
@@ -445,6 +467,12 @@ int multifd_save_setup(void)
qemu_sem_init(&p->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(&p->thread, thread_name, multifd_send_thread, p,
QEMU_THREAD_JOINABLE);
@@ -456,6 +484,7 @@ int multifd_save_setup(void)
struct MultiFDRecvParams {
uint8_t id;
QemuThread thread;
+QIOChannel *c;
QemuSemaphore sem;
QemuMutex mutex;
bool quit;
@@ -466,12 +495,16 @@ struct {
MultiFDRecvParams **params;
/* number of created threads */
int count;
+/* Should we finish */
+bool quit;
} *multifd_recv_state;
static void terminate_multifd_recv_threads(void)
{
int i;
+multifd_recv_state->quit = true;
+
for (i = 0; i < multifd_recv_state->count; i++) {
MultiFDRecvParams *p = multifd_recv_state->params[i];
@@ -496,6 +529,7 @@ void multifd_load_cleanup(void)
qemu_thread_join(&p->thread);
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
+socket_recv_channel_destroy(p->c);
g_free(p);
multifd_recv_state->params[i] = NULL;
}
@@ -522,10 +556,54 @@ static void *multifd_recv_thread(void *opaque
