* [Qemu-devel] [PATCH 01/16] qio: create new qio_channel_write_all
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:29 ` Daniel P. Berrange
2017-03-13 12:44 ` [Qemu-devel] [PATCH 02/16] qio: create new qio_channel_read_all Juan Quintela
` (15 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
The function waits until it is able to write the full iov.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
include/io/channel.h | 23 +++++++++++++++++++++++
io/channel.c | 39 +++++++++++++++++++++++++++++++++++++++
migration/qemu-file-channel.c | 29 +----------------------------
3 files changed, 63 insertions(+), 28 deletions(-)
diff --git a/include/io/channel.h b/include/io/channel.h
index 5d48906..f786c4f 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -269,6 +269,29 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
Error **errp);
/**
+ * qio_channel_writev_all:
+ * @ioc: the channel object
+ * @iov: the array of memory regions to write data from
+ * @niov: the length of the @iov array
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Write data to the IO channel, reading it from the
+ * memory regions referenced by @iov. Each element
+ * in the @iov will be fully sent, before the next
+ * one is used. The @niov parameter specifies the
+ * total number of elements in @iov.
+ *
+ * It is required for all @iov data to be fully
+ * sent.
+ *
+ * Returns: the number of bytes sent, or -1 on error,
+ */
+ssize_t qio_channel_writev_all(QIOChannel *ioc,
+ const struct iovec *iov,
+ size_t niov,
+ Error **erp);
+
+/**
* qio_channel_readv:
* @ioc: the channel object
* @iov: the array of memory regions to read data into
diff --git a/io/channel.c b/io/channel.c
index cdf7454..c5a1bd5 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -22,6 +22,7 @@
#include "io/channel.h"
#include "qapi/error.h"
#include "qemu/main-loop.h"
+#include "qemu/iov.h"
bool qio_channel_has_feature(QIOChannel *ioc,
QIOChannelFeature feature)
@@ -85,6 +86,44 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
}
+
+ssize_t qio_channel_writev_all(QIOChannel *ioc,
+ const struct iovec *iov,
+ size_t niov,
+ Error **errp)
+{
+ ssize_t done = 0;
+ struct iovec *local_iov = g_new(struct iovec, niov);
+ struct iovec *local_iov_head = local_iov;
+ unsigned int nlocal_iov = niov;
+
+ nlocal_iov = iov_copy(local_iov, nlocal_iov,
+ iov, niov,
+ 0, iov_size(iov, niov));
+
+ while (nlocal_iov > 0) {
+ ssize_t len;
+ len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
+ if (len == QIO_CHANNEL_ERR_BLOCK) {
+ qio_channel_wait(ioc, G_IO_OUT);
+ continue;
+ }
+ if (len < 0) {
+ error_setg_errno(errp, EIO,
+ "Channel was not able to write full iov");
+ done = -1;
+ goto cleanup;
+ }
+
+ iov_discard_front(&local_iov, &nlocal_iov, len);
+ done += len;
+ }
+
+ cleanup:
+ g_free(local_iov_head);
+ return done;
+}
+
ssize_t qio_channel_readv(QIOChannel *ioc,
const struct iovec *iov,
size_t niov,
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 45c13f1..dd95e06 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -34,35 +34,8 @@ static ssize_t channel_writev_buffer(void *opaque,
int64_t pos)
{
QIOChannel *ioc = QIO_CHANNEL(opaque);
- ssize_t done = 0;
- struct iovec *local_iov = g_new(struct iovec, iovcnt);
- struct iovec *local_iov_head = local_iov;
- unsigned int nlocal_iov = iovcnt;
- nlocal_iov = iov_copy(local_iov, nlocal_iov,
- iov, iovcnt,
- 0, iov_size(iov, iovcnt));
-
- while (nlocal_iov > 0) {
- ssize_t len;
- len = qio_channel_writev(ioc, local_iov, nlocal_iov, NULL);
- if (len == QIO_CHANNEL_ERR_BLOCK) {
- qio_channel_wait(ioc, G_IO_OUT);
- continue;
- }
- if (len < 0) {
- /* XXX handle Error objects */
- done = -EIO;
- goto cleanup;
- }
-
- iov_discard_front(&local_iov, &nlocal_iov, len);
- done += len;
- }
-
- cleanup:
- g_free(local_iov_head);
- return done;
+ return qio_channel_writev_all(ioc, iov, iovcnt, NULL);
}
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 01/16] qio: create new qio_channel_write_all
2017-03-13 12:44 ` [Qemu-devel] [PATCH 01/16] qio: create new qio_channel_write_all Juan Quintela
@ 2017-03-13 16:29 ` Daniel P. Berrange
2017-04-27 8:19 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-13 16:29 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 01:44:19PM +0100, Juan Quintela wrote:
> The function waits until it is able to write the full iov.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> include/io/channel.h | 23 +++++++++++++++++++++++
> io/channel.c | 39 +++++++++++++++++++++++++++++++++++++++
> migration/qemu-file-channel.c | 29 +----------------------------
> 3 files changed, 63 insertions(+), 28 deletions(-)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 5d48906..f786c4f 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -269,6 +269,29 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
> Error **errp);
>
> /**
> + * qio_channel_writev_all:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to write data from
> + * @niov: the length of the @iov array
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Write data to the IO channel, reading it from the
> + * memory regions referenced by @iov. Each element
> + * in the @iov will be fully sent, before the next
> + * one is used. The @niov parameter specifies the
> + * total number of elements in @iov.
> + *
> + * It is required for all @iov data to be fully
> + * sent.
> + *
> + * Returns: the number of bytes sent, or -1 on error,
> + */
> +ssize_t qio_channel_writev_all(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + Error **erp);
> +
> +/**
> * qio_channel_readv:
> * @ioc: the channel object
> * @iov: the array of memory regions to read data into
> diff --git a/io/channel.c b/io/channel.c
> index cdf7454..c5a1bd5 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -22,6 +22,7 @@
> #include "io/channel.h"
> #include "qapi/error.h"
> #include "qemu/main-loop.h"
> +#include "qemu/iov.h"
>
> bool qio_channel_has_feature(QIOChannel *ioc,
> QIOChannelFeature feature)
> @@ -85,6 +86,44 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
> }
>
>
> +
> +ssize_t qio_channel_writev_all(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + Error **errp)
> +{
> + ssize_t done = 0;
> + struct iovec *local_iov = g_new(struct iovec, niov);
> + struct iovec *local_iov_head = local_iov;
> + unsigned int nlocal_iov = niov;
> +
> + nlocal_iov = iov_copy(local_iov, nlocal_iov,
> + iov, niov,
> + 0, iov_size(iov, niov));
> +
> + while (nlocal_iov > 0) {
> + ssize_t len;
> + len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
> + if (len == QIO_CHANNEL_ERR_BLOCK) {
> + qio_channel_wait(ioc, G_IO_OUT);
> + continue;
> + }
> + if (len < 0) {
> + error_setg_errno(errp, EIO,
> + "Channel was not able to write full iov");
> + done = -1;
> + goto cleanup;
> + }
> +
> + iov_discard_front(&local_iov, &nlocal_iov, len);
> + done += len;
> + }
> +
> + cleanup:
> + g_free(local_iov_head);
> + return done;
> +}
> +
Can you make sure this is covered in the test suite. Something added
or changed in tests/io-channel-helpers.c should be sufficient. eg use
it from test_io_thread_writer() when applicable
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 01/16] qio: create new qio_channel_write_all
2017-03-13 16:29 ` Daniel P. Berrange
@ 2017-04-27 8:19 ` Juan Quintela
0 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-04-27 8:19 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, amit.shah, dgilbert
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Mar 13, 2017 at 01:44:19PM +0100, Juan Quintela wrote:
>> The function waits until it is able to write the full iov.
>>
>
> Can you make sure this is covered in the test suite. Something added
> or changed in tests/io-channel-helpers.c should be sufficient. eg use
> it from test_io_thread_writer() when applicable
Ok, I integreted together the two patches, and added tests for:
qio_channel_writev - qio_channel_readv
qio_channel_writev_all - qio_channel_readv
qio_channel_writev - qio_channel_readv_all
qio_channel_writev_all - qio_channel_readv_all
I did it only for test-io-channel-buffer.c, though.
Thanks, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 02/16] qio: create new qio_channel_read_all
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 01/16] qio: create new qio_channel_write_all Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:30 ` Daniel P. Berrange
2017-03-13 12:44 ` [Qemu-devel] [PATCH 03/16] migration: Test for disabled features on reception Juan Quintela
` (14 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
It is the symmetric function from qio_channel_write_all
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
include/io/channel.h | 23 +++++++++++++++++++++++
io/channel.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
diff --git a/include/io/channel.h b/include/io/channel.h
index f786c4f..2f55831 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -269,6 +269,29 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
Error **errp);
/**
+ * qio_channel_readv_all:
+ * @ioc: the channel object
+ * @iov: the array of memory regions to read data into
+ * @niov: the length of the @iov array
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Read data from the IO channel, storing it in the
+ * memory regions referenced by @iov. Each element
+ * in the @iov will be fully populated with data
+ * before the next one is used. The @niov parameter
+ * specifies the total number of elements in @iov.
+ *
+ * Returns: the number of bytes read, or -1 on error,
+ * or QIO_CHANNEL_ERR_BLOCK if no data is available
+ * and the channel is non-blocking
+ */
+ssize_t qio_channel_readv_all(QIOChannel *ioc,
+ const struct iovec *iov,
+ size_t niov,
+ Error **errp);
+
+
+/**
* qio_channel_writev_all:
* @ioc: the channel object
* @iov: the array of memory regions to write data from
diff --git a/io/channel.c b/io/channel.c
index c5a1bd5..82203ef 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -87,6 +87,43 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
+ssize_t qio_channel_readv_all(QIOChannel *ioc,
+ const struct iovec *iov,
+ size_t niov,
+ Error **errp)
+{
+ ssize_t done = 0;
+ struct iovec *local_iov = g_new(struct iovec, niov);
+ struct iovec *local_iov_head = local_iov;
+ unsigned int nlocal_iov = niov;
+
+ nlocal_iov = iov_copy(local_iov, nlocal_iov,
+ iov, niov,
+ 0, iov_size(iov, niov));
+
+ while (nlocal_iov > 0) {
+ ssize_t len;
+ len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
+ if (len == QIO_CHANNEL_ERR_BLOCK) {
+ qio_channel_wait(ioc, G_IO_OUT);
+ continue;
+ }
+ if (len < 0) {
+ error_setg_errno(errp, EIO,
+ "Channel was not able to read full iov");
+ done = -1;
+ goto cleanup;
+ }
+
+ iov_discard_front(&local_iov, &nlocal_iov, len);
+ done += len;
+ }
+
+ cleanup:
+ g_free(local_iov_head);
+ return done;
+}
+
ssize_t qio_channel_writev_all(QIOChannel *ioc,
const struct iovec *iov,
size_t niov,
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 02/16] qio: create new qio_channel_read_all
2017-03-13 12:44 ` [Qemu-devel] [PATCH 02/16] qio: create new qio_channel_read_all Juan Quintela
@ 2017-03-13 16:30 ` Daniel P. Berrange
0 siblings, 0 replies; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-13 16:30 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 01:44:20PM +0100, Juan Quintela wrote:
> It is the symmetric function from qio_channel_write_all
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> include/io/channel.h | 23 +++++++++++++++++++++++
> io/channel.c | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index f786c4f..2f55831 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -269,6 +269,29 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
> Error **errp);
>
> /**
> + * qio_channel_readv_all:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data into
> + * @niov: the length of the @iov array
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Read data from the IO channel, storing it in the
> + * memory regions referenced by @iov. Each element
> + * in the @iov will be fully populated with data
> + * before the next one is used. The @niov parameter
> + * specifies the total number of elements in @iov.
> + *
> + * Returns: the number of bytes read, or -1 on error,
> + * or QIO_CHANNEL_ERR_BLOCK if no data is available
> + * and the channel is non-blocking
> + */
> +ssize_t qio_channel_readv_all(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + Error **errp);
> +
> +
> +/**
> * qio_channel_writev_all:
> * @ioc: the channel object
> * @iov: the array of memory regions to write data from
> diff --git a/io/channel.c b/io/channel.c
> index c5a1bd5..82203ef 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -87,6 +87,43 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
>
>
>
> +ssize_t qio_channel_readv_all(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + Error **errp)
> +{
> + ssize_t done = 0;
> + struct iovec *local_iov = g_new(struct iovec, niov);
> + struct iovec *local_iov_head = local_iov;
> + unsigned int nlocal_iov = niov;
> +
> + nlocal_iov = iov_copy(local_iov, nlocal_iov,
> + iov, niov,
> + 0, iov_size(iov, niov));
> +
> + while (nlocal_iov > 0) {
> + ssize_t len;
> + len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
> + if (len == QIO_CHANNEL_ERR_BLOCK) {
> + qio_channel_wait(ioc, G_IO_OUT);
> + continue;
> + }
> + if (len < 0) {
> + error_setg_errno(errp, EIO,
> + "Channel was not able to read full iov");
> + done = -1;
> + goto cleanup;
> + }
> +
> + iov_discard_front(&local_iov, &nlocal_iov, len);
> + done += len;
> + }
> +
> + cleanup:
> + g_free(local_iov_head);
> + return done;
> +}
> +
Again, can you make sure this is covered in the test suite - the
test_io_thread_reader() method can probably be replaced by this code.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 03/16] migration: Test for disabled features on reception
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 01/16] qio: create new qio_channel_write_all Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 02/16] qio: create new qio_channel_read_all Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:21 ` Dr. David Alan Gilbert
2017-03-13 12:44 ` [Qemu-devel] [PATCH 04/16] migration: Don't create decompression threads if not enabled Juan Quintela
` (13 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Right now, if we receive a compressed page while this features are
disabled, Bad Things (TM) can happen. Just add a test for them.
Signed-off-by: Juan Quintela <quintela@redhat.com>
--
I had XBZRLE here also, but it don't need extra resources on
destination, only on source. Additionally libvirt don't enable it on
destination, so don't put it here.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 719425b..65419c1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2514,7 +2514,7 @@ static int ram_load_postcopy(QEMUFile *f)
static int ram_load(QEMUFile *f, void *opaque, int version_id)
{
- int flags = 0, ret = 0;
+ int flags = 0, ret = 0, invalid_flags;
static uint64_t seq_iter;
int len = 0;
/*
@@ -2531,6 +2531,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
ret = -EINVAL;
}
+ invalid_flags = 0;
+
+ if (!migrate_use_compression()) {
+ invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
+ }
/* This RCU critical section can be very long running.
* When RCU reclaims in the code start to become numerous,
* it will be necessary to reduce the granularity of this
@@ -2551,6 +2556,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
flags = addr & ~TARGET_PAGE_MASK;
addr &= TARGET_PAGE_MASK;
+ if (flags & invalid_flags) {
+ if (flags & invalid_flags & RAM_SAVE_FLAG_COMPRESS_PAGE) {
+ error_report("Received an unexpected compressed page");
+ }
+
+ ret = -EINVAL;
+ break;
+ }
+
if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
RAMBlock *block = ram_block_from_stream(f, flags);
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 03/16] migration: Test for disabled features on reception
2017-03-13 12:44 ` [Qemu-devel] [PATCH 03/16] migration: Test for disabled features on reception Juan Quintela
@ 2017-03-13 16:21 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-13 16:21 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel
* Juan Quintela (quintela@redhat.com) wrote:
> Right now, if we receive a compressed page while this features are
> disabled, Bad Things (TM) can happen. Just add a test for them.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> --
>
> I had XBZRLE here also, but it don't need extra resources on
> destination, only on source. Additionally libvirt don't enable it on
> destination, so don't put it here.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/ram.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 719425b..65419c1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2514,7 +2514,7 @@ static int ram_load_postcopy(QEMUFile *f)
>
> static int ram_load(QEMUFile *f, void *opaque, int version_id)
> {
> - int flags = 0, ret = 0;
> + int flags = 0, ret = 0, invalid_flags;
> static uint64_t seq_iter;
> int len = 0;
> /*
> @@ -2531,6 +2531,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> ret = -EINVAL;
> }
>
> + invalid_flags = 0;
> +
> + if (!migrate_use_compression()) {
> + invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
> + }
> /* This RCU critical section can be very long running.
> * When RCU reclaims in the code start to become numerous,
> * it will be necessary to reduce the granularity of this
> @@ -2551,6 +2556,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
> flags = addr & ~TARGET_PAGE_MASK;
> addr &= TARGET_PAGE_MASK;
>
> + if (flags & invalid_flags) {
> + if (flags & invalid_flags & RAM_SAVE_FLAG_COMPRESS_PAGE) {
> + error_report("Received an unexpected compressed page");
> + }
> +
> + ret = -EINVAL;
> + break;
> + }
> +
> if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
> RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
> RAMBlock *block = ram_block_from_stream(f, flags);
> --
> 2.9.3
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 04/16] migration: Don't create decompression threads if not enabled
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (2 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 03/16] migration: Test for disabled features on reception Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:25 ` Dr. David Alan Gilbert
2017-03-13 12:44 ` [Qemu-devel] [PATCH 05/16] migration: Add multifd capability Juan Quintela
` (12 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Signed-off-by: Juan Quintela <quintela@redhat.com>
--
I removed the [HACK] part because previous patch just check that
compression pages are not received.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/migration/ram.c b/migration/ram.c
index 65419c1..aa51dbd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2306,6 +2306,9 @@ void migrate_decompress_threads_create(void)
{
int i, thread_count;
+ if (!migrate_use_compression()) {
+ return;
+ }
thread_count = migrate_decompress_threads();
decompress_threads = g_new0(QemuThread, thread_count);
decomp_param = g_new0(DecompressParam, thread_count);
@@ -2327,6 +2330,9 @@ void migrate_decompress_threads_join(void)
{
int i, thread_count;
+ if (!migrate_use_compression()) {
+ return;
+ }
thread_count = migrate_decompress_threads();
for (i = 0; i < thread_count; i++) {
qemu_mutex_lock(&decomp_param[i].mutex);
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 04/16] migration: Don't create decompression threads if not enabled
2017-03-13 12:44 ` [Qemu-devel] [PATCH 04/16] migration: Don't create decompression threads if not enabled Juan Quintela
@ 2017-03-13 16:25 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-13 16:25 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah
* Juan Quintela (quintela@redhat.com) wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> --
>
> I removed the [HACK] part because previous patch just check that
> compression pages are not received.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/ram.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 65419c1..aa51dbd 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2306,6 +2306,9 @@ void migrate_decompress_threads_create(void)
> {
> int i, thread_count;
>
> + if (!migrate_use_compression()) {
> + return;
> + }
> thread_count = migrate_decompress_threads();
> decompress_threads = g_new0(QemuThread, thread_count);
> decomp_param = g_new0(DecompressParam, thread_count);
> @@ -2327,6 +2330,9 @@ void migrate_decompress_threads_join(void)
> {
> int i, thread_count;
>
> + if (!migrate_use_compression()) {
> + return;
> + }
> thread_count = migrate_decompress_threads();
> for (i = 0; i < thread_count; i++) {
> qemu_mutex_lock(&decomp_param[i].mutex);
> --
> 2.9.3
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 05/16] migration: Add multifd capability
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (3 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 04/16] migration: Don't create decompression threads if not enabled Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 06/16] migration: Create x-multifd-threads parameter Juan Quintela
` (11 subsequent siblings)
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
include/migration/migration.h | 1 +
migration/migration.c | 9 +++++++++
qapi-schema.json | 6 ++++--
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 5720c88..d48934b 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -323,6 +323,7 @@ bool migrate_postcopy_ram(void);
bool migrate_zero_blocks(void);
bool migrate_auto_converge(void);
+bool migrate_use_multifd(void);
int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
uint8_t *dst, int dlen);
diff --git a/migration/migration.c b/migration/migration.c
index 3dab684..0e5b0bf 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1409,6 +1409,15 @@ bool migrate_use_events(void)
return s->enabled_capabilities[MIGRATION_CAPABILITY_EVENTS];
}
+bool migrate_use_multifd(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ return s->enabled_capabilities[MIGRATION_CAPABILITY_X_MULTIFD];
+}
+
int migrate_use_xbzrle(void)
{
MigrationState *s;
diff --git a/qapi-schema.json b/qapi-schema.json
index 32b4a4b..d21934b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -868,12 +868,14 @@
# @release-ram: if enabled, qemu will free the migrated ram pages on the source
# during postcopy-ram migration. (since 2.9)
#
+# @x-multifd: Use more than one fd for migration (since 2.9)
+#
# Since: 1.2
##
{ 'enum': 'MigrationCapability',
'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
- 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram'] }
-
+ 'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
+ 'x-multifd'] }
##
# @MigrationCapabilityStatus:
#
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 06/16] migration: Create x-multifd-threads parameter
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (4 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 05/16] migration: Add multifd capability Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:37 ` Daniel P. Berrange
2017-03-13 12:44 ` [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter Juan Quintela
` (10 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Indicates the number of threads that we would create. By default we
create 2 threads.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
--
Catch inconsistent defaults.
Thanks Eric
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 8 ++++++++
include/migration/migration.h | 2 ++
migration/migration.c | 23 +++++++++++++++++++++++
qapi-schema.json | 13 +++++++++++--
4 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/hmp.c b/hmp.c
index 261843f..7d6db63 100644
--- a/hmp.c
+++ b/hmp.c
@@ -323,6 +323,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, " %s: %" PRId64,
MigrationParameter_lookup[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY],
params->x_checkpoint_delay);
+ monitor_printf(mon, " %s: %" PRId64,
+ MigrationParameter_lookup[MIGRATION_PARAMETER_X_MULTIFD_THREADS],
+ params->x_multifd_threads);
monitor_printf(mon, "\n");
}
@@ -1400,6 +1403,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p.has_x_checkpoint_delay = true;
use_int_value = true;
break;
+ case MIGRATION_PARAMETER_X_MULTIFD_THREADS:
+ p.has_x_multifd_threads = true;
+ use_int_value = true;
+ break;
}
if (use_int_value) {
@@ -1417,6 +1424,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p.cpu_throttle_increment = valueint;
p.downtime_limit = valueint;
p.x_checkpoint_delay = valueint;
+ p.x_multifd_threads = valueint;
}
qmp_migrate_set_parameters(&p, &err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index d48934b..2950270 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -265,6 +265,8 @@ bool migration_in_postcopy(MigrationState *);
bool migration_in_postcopy_after_devices(MigrationState *);
MigrationState *migrate_get_current(void);
+int migrate_multifd_threads(void);
+
void migrate_compress_threads_create(void);
void migrate_compress_threads_join(void);
void migrate_decompress_threads_create(void);
diff --git a/migration/migration.c b/migration/migration.c
index 0e5b0bf..cc9440b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -71,6 +71,7 @@
* Note: Please change this default value to 10000 when we support hybrid mode.
*/
#define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY 200
+#define DEFAULT_MIGRATE_MULTIFD_THREADS 2
static NotifierList migration_state_notifiers =
NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -105,6 +106,7 @@ MigrationState *migrate_get_current(void)
.max_bandwidth = MAX_THROTTLE,
.downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
.x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
+ .x_multifd_threads = DEFAULT_MIGRATE_MULTIFD_THREADS,
},
};
@@ -596,6 +598,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
params->downtime_limit = s->parameters.downtime_limit;
params->has_x_checkpoint_delay = true;
params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
+ params->has_x_multifd_threads = true;
+ params->x_multifd_threads = s->parameters.x_multifd_threads;
return params;
}
@@ -860,6 +864,13 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
"x_checkpoint_delay",
"is invalid, it should be positive");
}
+ if (params->has_x_multifd_threads &&
+ (params->x_multifd_threads < 1 || params->x_multifd_threads > 255)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ "multifd_threads",
+ "is invalid, it should be in the range of 1 to 255");
+ return;
+ }
if (params->has_compress_level) {
s->parameters.compress_level = params->compress_level;
@@ -901,6 +912,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
colo_checkpoint_notify(s);
}
}
+ if (params->has_x_multifd_threads) {
+ s->parameters.x_multifd_threads = params->x_multifd_threads;
+ }
}
@@ -1418,6 +1432,15 @@ bool migrate_use_multifd(void)
return s->enabled_capabilities[MIGRATION_CAPABILITY_X_MULTIFD];
}
+int migrate_multifd_threads(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ return s->parameters.x_multifd_threads;
+}
+
int migrate_use_xbzrle(void)
{
MigrationState *s;
diff --git a/qapi-schema.json b/qapi-schema.json
index d21934b..b7cb26d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -985,13 +985,17 @@
# @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
# periodic mode. (Since 2.8)
#
+# @x-multifd-threads: Number of threads used to migrate data in parallel
+# The default value is 2 (since 2.9)
+#
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
'data': ['compress-level', 'compress-threads', 'decompress-threads',
'cpu-throttle-initial', 'cpu-throttle-increment',
'tls-creds', 'tls-hostname', 'max-bandwidth',
- 'downtime-limit', 'x-checkpoint-delay' ] }
+ 'downtime-limit', 'x-checkpoint-delay',
+ 'x-multifd-threads'] }
##
# @migrate-set-parameters:
@@ -1054,6 +1058,10 @@
#
# @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
#
+#
+# @x-multifd-threads: Number of threads used to migrate data in parallel
+# The default value is 2 (since 2.9)
+#
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
@@ -1066,7 +1074,8 @@
'*tls-hostname': 'str',
'*max-bandwidth': 'int',
'*downtime-limit': 'int',
- '*x-checkpoint-delay': 'int'} }
+ '*x-checkpoint-delay': 'int',
+ '*x-multifd-threads': 'int'} }
##
# @query-migrate-parameters:
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 06/16] migration: Create x-multifd-threads parameter
2017-03-13 12:44 ` [Qemu-devel] [PATCH 06/16] migration: Create x-multifd-threads parameter Juan Quintela
@ 2017-03-13 16:37 ` Daniel P. Berrange
2017-03-13 16:50 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-13 16:37 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 01:44:24PM +0100, Juan Quintela wrote:
> Indicates the number of threads that we would create. By default we
> create 2 threads.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d21934b..b7cb26d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -985,13 +985,17 @@
> # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
> # periodic mode. (Since 2.8)
> #
> +# @x-multifd-threads: Number of threads used to migrate data in parallel
> +# The default value is 2 (since 2.9)
> +#
> # Since: 2.4
> ##
> { 'enum': 'MigrationParameter',
> 'data': ['compress-level', 'compress-threads', 'decompress-threads',
> 'cpu-throttle-initial', 'cpu-throttle-increment',
> 'tls-creds', 'tls-hostname', 'max-bandwidth',
> - 'downtime-limit', 'x-checkpoint-delay' ] }
> + 'downtime-limit', 'x-checkpoint-delay',
> + 'x-multifd-threads'] }
>
> ##
> # @migrate-set-parameters:
> @@ -1054,6 +1058,10 @@
> #
> # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
> #
> +#
> +# @x-multifd-threads: Number of threads used to migrate data in parallel
> +# The default value is 2 (since 2.9)
> +#
Presumably, the number of threads is the same as the number of sockets
opened ? It is probably useful to make it explicit that increasing
threads also causes increased number of TCP sockets to be opened.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 06/16] migration: Create x-multifd-threads parameter
2017-03-13 16:37 ` Daniel P. Berrange
@ 2017-03-13 16:50 ` Juan Quintela
0 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 16:50 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, amit.shah, dgilbert
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Mar 13, 2017 at 01:44:24PM +0100, Juan Quintela wrote:
>> Indicates the number of threads that we would create. By default we
>> create 2 threads.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d21934b..b7cb26d 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -985,13 +985,17 @@
>> # @x-checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
>> # periodic mode. (Since 2.8)
>> #
>> +# @x-multifd-threads: Number of threads used to migrate data in parallel
>> +# The default value is 2 (since 2.9)
>> +#
>> # Since: 2.4
>> ##
>> { 'enum': 'MigrationParameter',
>> 'data': ['compress-level', 'compress-threads', 'decompress-threads',
>> 'cpu-throttle-initial', 'cpu-throttle-increment',
>> 'tls-creds', 'tls-hostname', 'max-bandwidth',
>> - 'downtime-limit', 'x-checkpoint-delay' ] }
>> + 'downtime-limit', 'x-checkpoint-delay',
>> + 'x-multifd-threads'] }
>>
>> ##
>> # @migrate-set-parameters:
>> @@ -1054,6 +1058,10 @@
>> #
>> # @x-checkpoint-delay: the delay time between two COLO checkpoints. (Since 2.8)
>> #
>> +#
>> +# @x-multifd-threads: Number of threads used to migrate data in parallel
>> +# The default value is 2 (since 2.9)
>> +#
>
> Presumably, the number of threads is the same as the number of sockets
> opened ? It is probably useful to make it explicit that increasing
> threads also causes increased number of TCP sockets to be opened.
Yeap. 1 socket by thread. I will make that explicit.
Thanks, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (5 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 06/16] migration: Create x-multifd-threads parameter Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:34 ` Daniel P. Berrange
2017-03-13 12:44 ` [Qemu-devel] [PATCH 08/16] migration: Create multifd migration threads Juan Quintela
` (9 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Indicates how many pages we are going to send in each bach to a multifd
thread.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
--
Be consistent with defaults and documentation
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 8 ++++++++
include/migration/migration.h | 1 +
migration/migration.c | 23 +++++++++++++++++++++++
qapi-schema.json | 11 +++++++++--
4 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/hmp.c b/hmp.c
index 7d6db63..ab02773 100644
--- a/hmp.c
+++ b/hmp.c
@@ -326,6 +326,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, " %s: %" PRId64,
MigrationParameter_lookup[MIGRATION_PARAMETER_X_MULTIFD_THREADS],
params->x_multifd_threads);
+ monitor_printf(mon, " %s: %" PRId64,
+ MigrationParameter_lookup[MIGRATION_PARAMETER_X_MULTIFD_GROUP],
+ params->x_multifd_group);
monitor_printf(mon, "\n");
}
@@ -1407,6 +1410,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p.has_x_multifd_threads = true;
use_int_value = true;
break;
+ case MIGRATION_PARAMETER_X_MULTIFD_GROUP:
+ p.has_x_multifd_group = true;
+ use_int_value = true;
+ break;
}
if (use_int_value) {
@@ -1425,6 +1432,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
p.downtime_limit = valueint;
p.x_checkpoint_delay = valueint;
p.x_multifd_threads = valueint;
+ p.x_multifd_group = valueint;
}
qmp_migrate_set_parameters(&p, &err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 2950270..bacde15 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -266,6 +266,7 @@ bool migration_in_postcopy_after_devices(MigrationState *);
MigrationState *migrate_get_current(void);
int migrate_multifd_threads(void);
+int migrate_multifd_group(void);
void migrate_compress_threads_create(void);
void migrate_compress_threads_join(void);
diff --git a/migration/migration.c b/migration/migration.c
index cc9440b..4cc45a4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -72,6 +72,7 @@
*/
#define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY 200
#define DEFAULT_MIGRATE_MULTIFD_THREADS 2
+#define DEFAULT_MIGRATE_MULTIFD_GROUP 16
static NotifierList migration_state_notifiers =
NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -107,6 +108,7 @@ MigrationState *migrate_get_current(void)
.downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
.x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
.x_multifd_threads = DEFAULT_MIGRATE_MULTIFD_THREADS,
+ .x_multifd_group = DEFAULT_MIGRATE_MULTIFD_GROUP,
},
};
@@ -600,6 +602,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
params->has_x_multifd_threads = true;
params->x_multifd_threads = s->parameters.x_multifd_threads;
+ params->has_x_multifd_group = true;
+ params->x_multifd_group = s->parameters.x_multifd_group;
return params;
}
@@ -871,6 +875,13 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
"is invalid, it should be in the range of 1 to 255");
return;
}
+ if (params->has_x_multifd_group &&
+ (params->x_multifd_group < 1 || params->x_multifd_group > 10000)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ "multifd_group",
+ "is invalid, it should be in the range of 1 to 10000");
+ return;
+ }
if (params->has_compress_level) {
s->parameters.compress_level = params->compress_level;
@@ -915,6 +926,9 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
if (params->has_x_multifd_threads) {
s->parameters.x_multifd_threads = params->x_multifd_threads;
}
+ if (params->has_x_multifd_group) {
+ s->parameters.x_multifd_group = params->x_multifd_group;
+ }
}
@@ -1441,6 +1455,15 @@ int migrate_multifd_threads(void)
return s->parameters.x_multifd_threads;
}
+int migrate_multifd_group(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ return s->parameters.x_multifd_group;
+}
+
int migrate_use_xbzrle(void)
{
MigrationState *s;
diff --git a/qapi-schema.json b/qapi-schema.json
index b7cb26d..33a6267 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -988,6 +988,9 @@
# @x-multifd-threads: Number of threads used to migrate data in parallel
# The default value is 2 (since 2.9)
#
+# @x-multifd-group: Number of pages sent together to a thread
+# The default value is 16 (since 2.9)
+#
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
@@ -995,7 +998,7 @@
'cpu-throttle-initial', 'cpu-throttle-increment',
'tls-creds', 'tls-hostname', 'max-bandwidth',
'downtime-limit', 'x-checkpoint-delay',
- 'x-multifd-threads'] }
+ 'x-multifd-threads', 'x-multifd-group'] }
##
# @migrate-set-parameters:
@@ -1062,6 +1065,9 @@
# @x-multifd-threads: Number of threads used to migrate data in parallel
# The default value is 2 (since 2.9)
#
+# @x-multifd-group: Number of pages sent together in a bunch
+# The default value is 16 (since 2.9)
+#
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
@@ -1075,7 +1081,8 @@
'*max-bandwidth': 'int',
'*downtime-limit': 'int',
'*x-checkpoint-delay': 'int',
- '*x-multifd-threads': 'int'} }
+ '*x-multifd-threads': 'int',
+ '*x-multifd-group': 'int'} }
##
# @query-migrate-parameters:
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter
2017-03-13 12:44 ` [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter Juan Quintela
@ 2017-03-13 16:34 ` Daniel P. Berrange
2017-03-13 16:49 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-13 16:34 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 01:44:25PM +0100, Juan Quintela wrote:
> Indicates how many pages we are going to send in each bach to a multifd
> thread.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b7cb26d..33a6267 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -988,6 +988,9 @@
> # @x-multifd-threads: Number of threads used to migrate data in parallel
> # The default value is 2 (since 2.9)
> #
> +# @x-multifd-group: Number of pages sent together to a thread
> +# The default value is 16 (since 2.9)
> +#
> # Since: 2.4
> ##
> { 'enum': 'MigrationParameter',
> @@ -995,7 +998,7 @@
> 'cpu-throttle-initial', 'cpu-throttle-increment',
> 'tls-creds', 'tls-hostname', 'max-bandwidth',
> 'downtime-limit', 'x-checkpoint-delay',
> - 'x-multifd-threads'] }
> + 'x-multifd-threads', 'x-multifd-group'] }
>
> ##
> # @migrate-set-parameters:
> @@ -1062,6 +1065,9 @@
> # @x-multifd-threads: Number of threads used to migrate data in parallel
> # The default value is 2 (since 2.9)
> #
> +# @x-multifd-group: Number of pages sent together in a bunch
> +# The default value is 16 (since 2.9)
> +#
How is this parameter supposed to be used ? Or to put it another way,
what are the benefits / effects of changing it from its default
value and can an application usefully decide what value to set ? I'm
loathe to see us expose another "black magic" parameter where you can't
easily determine what values to set, without predicting future guest
workloads
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter
2017-03-13 16:34 ` Daniel P. Berrange
@ 2017-03-13 16:49 ` Juan Quintela
2017-03-13 17:12 ` Daniel P. Berrange
0 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 16:49 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, amit.shah, dgilbert
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Mar 13, 2017 at 01:44:25PM +0100, Juan Quintela wrote:
>> Indicates how many pages we are going to send in each bach to a multifd
>> thread.
>
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b7cb26d..33a6267 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -988,6 +988,9 @@
>> # @x-multifd-threads: Number of threads used to migrate data in parallel
>> # The default value is 2 (since 2.9)
>> #
>> +# @x-multifd-group: Number of pages sent together to a thread
>> +# The default value is 16 (since 2.9)
>> +#
>> # Since: 2.4
>> ##
>> { 'enum': 'MigrationParameter',
>> @@ -995,7 +998,7 @@
>> 'cpu-throttle-initial', 'cpu-throttle-increment',
>> 'tls-creds', 'tls-hostname', 'max-bandwidth',
>> 'downtime-limit', 'x-checkpoint-delay',
>> - 'x-multifd-threads'] }
>> + 'x-multifd-threads', 'x-multifd-group'] }
>>
>> ##
>> # @migrate-set-parameters:
>> @@ -1062,6 +1065,9 @@
>> # @x-multifd-threads: Number of threads used to migrate data in parallel
>> # The default value is 2 (since 2.9)
>> #
>> +# @x-multifd-group: Number of pages sent together in a bunch
>> +# The default value is 16 (since 2.9)
>> +#
>
> How is this parameter supposed to be used ? Or to put it another way,
> what are the benefits / effects of changing it from its default
> value and can an application usefully decide what value to set ? I'm
> loathe to see us expose another "black magic" parameter where you can't
> easily determine what values to set, without predicting future guest
> workloads
We have multiple threads, we can send to each thread the number of pages
that it needs to send one by one, two by two, n x n. The bigger the
number, the less locking to do, and then less contention. But if it is
too big, we could probably end with too few distribution. Reason to add
this parameter is that if we send page by page, we end spending too much
time in locking.
Later, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter
2017-03-13 16:49 ` Juan Quintela
@ 2017-03-13 17:12 ` Daniel P. Berrange
2017-03-13 18:35 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-13 17:12 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 05:49:59PM +0100, Juan Quintela wrote:
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > On Mon, Mar 13, 2017 at 01:44:25PM +0100, Juan Quintela wrote:
> >> Indicates how many pages we are going to send in each bach to a multifd
> >> thread.
> >
> >
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index b7cb26d..33a6267 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -988,6 +988,9 @@
> >> # @x-multifd-threads: Number of threads used to migrate data in parallel
> >> # The default value is 2 (since 2.9)
> >> #
> >> +# @x-multifd-group: Number of pages sent together to a thread
> >> +# The default value is 16 (since 2.9)
> >> +#
> >> # Since: 2.4
> >> ##
> >> { 'enum': 'MigrationParameter',
> >> @@ -995,7 +998,7 @@
> >> 'cpu-throttle-initial', 'cpu-throttle-increment',
> >> 'tls-creds', 'tls-hostname', 'max-bandwidth',
> >> 'downtime-limit', 'x-checkpoint-delay',
> >> - 'x-multifd-threads'] }
> >> + 'x-multifd-threads', 'x-multifd-group'] }
> >>
> >> ##
> >> # @migrate-set-parameters:
> >> @@ -1062,6 +1065,9 @@
> >> # @x-multifd-threads: Number of threads used to migrate data in parallel
> >> # The default value is 2 (since 2.9)
> >> #
> >> +# @x-multifd-group: Number of pages sent together in a bunch
> >> +# The default value is 16 (since 2.9)
> >> +#
> >
> > How is this parameter supposed to be used ? Or to put it another way,
> > what are the benefits / effects of changing it from its default
> > value and can an application usefully decide what value to set ? I'm
> > loathe to see us expose another "black magic" parameter where you can't
> > easily determine what values to set, without predicting future guest
> > workloads
>
> We have multiple threads, we can send to each thread the number of pages
> that it needs to send one by one, two by two, n x n. The bigger the
> number, the less locking to do, and then less contention. But if it is
> too big, we could probably end with too few distribution. Reason to add
> this parameter is that if we send page by page, we end spending too much
> time in locking.
The question is how is an application like OpenStack / oVirt supposed to
know what the right number of pages is to get the right tradeoff between
lock contention & distribution ? Lock contention may well change over
time as the QEMU impl is improved, so the right answer for setting this
parameter might vary depending on QEMU version. IMHO, you should just
pick a sensible default value and not expose this to applications.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter
2017-03-13 17:12 ` Daniel P. Berrange
@ 2017-03-13 18:35 ` Juan Quintela
0 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 18:35 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, amit.shah, dgilbert
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Mar 13, 2017 at 05:49:59PM +0100, Juan Quintela wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> > On Mon, Mar 13, 2017 at 01:44:25PM +0100, Juan Quintela wrote:
>> >> Indicates how many pages we are going to send in each bach to a multifd
>> >> thread.
>> >
>> >
>> >> diff --git a/qapi-schema.json b/qapi-schema.json
>> >> index b7cb26d..33a6267 100644
>> >> --- a/qapi-schema.json
>> >> +++ b/qapi-schema.json
>> >> @@ -988,6 +988,9 @@
>> >> # @x-multifd-threads: Number of threads used to migrate data in parallel
>> >> # The default value is 2 (since 2.9)
>> >> #
>> >> +# @x-multifd-group: Number of pages sent together to a thread
>> >> +# The default value is 16 (since 2.9)
>> >> +#
>> >> # Since: 2.4
>> >> ##
>> >> { 'enum': 'MigrationParameter',
>> >> @@ -995,7 +998,7 @@
>> >> 'cpu-throttle-initial', 'cpu-throttle-increment',
>> >> 'tls-creds', 'tls-hostname', 'max-bandwidth',
>> >> 'downtime-limit', 'x-checkpoint-delay',
>> >> - 'x-multifd-threads'] }
>> >> + 'x-multifd-threads', 'x-multifd-group'] }
>> >>
>> >> ##
>> >> # @migrate-set-parameters:
>> >> @@ -1062,6 +1065,9 @@
>> >> # @x-multifd-threads: Number of threads used to migrate data in parallel
>> >> # The default value is 2 (since 2.9)
>> >> #
>> >> +# @x-multifd-group: Number of pages sent together in a bunch
>> >> +# The default value is 16 (since 2.9)
>> >> +#
>> >
>> > How is this parameter supposed to be used ? Or to put it another way,
>> > what are the benefits / effects of changing it from its default
>> > value and can an application usefully decide what value to set ? I'm
>> > loathe to see us expose another "black magic" parameter where you can't
>> > easily determine what values to set, without predicting future guest
>> > workloads
>>
>> We have multiple threads, we can send to each thread the number of pages
>> that it needs to send one by one, two by two, n x n. The bigger the
>> number, the less locking to do, and then less contention. But if it is
>> too big, we could probably end with too few distribution. Reason to add
>> this parameter is that if we send page by page, we end spending too much
>> time in locking.
>
> The question is how is an application like OpenStack / oVirt supposed to
> know what the right number of pages is to get the right tradeoff between
> lock contention & distribution ? Lock contention may well change over
> time as the QEMU impl is improved, so the right answer for setting this
> parameter might vary depending on QEMU version. IMHO, you should just
> pick a sensible default value and not expose this to applications.
It is still a x-<field>. It is good for testing. I agree that if there
is a sensible default, we should stick with it.
Thanks, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 08/16] migration: Create multifd migration threads
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (6 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 07/16] migration: Create x-multifd-group parameter Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work Juan Quintela
` (8 subsequent siblings)
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Creation of the threads, nothing inside yet.
Signed-off-by: Juan Quintela <quintela@redhat.com>
--
Use pointers instead of long array names
Move to use semaphores instead of conditions as paolo suggestion
Put all the state inside one struct.
Use a counter for the number of threads created. Needed during cancellation.
Add error return to thread creation
Add id field
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
include/migration/migration.h | 4 +
migration/migration.c | 15 ++++
migration/ram.c | 188 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 207 insertions(+)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bacde15..e8b9fcb 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -267,6 +267,10 @@ MigrationState *migrate_get_current(void);
int migrate_multifd_threads(void);
int migrate_multifd_group(void);
+int migrate_multifd_send_threads_create(void);
+void migrate_multifd_send_threads_join(void);
+int migrate_multifd_recv_threads_create(void);
+void migrate_multifd_recv_threads_join(void);
void migrate_compress_threads_create(void);
void migrate_compress_threads_join(void);
diff --git a/migration/migration.c b/migration/migration.c
index 4cc45a4..5bbd688 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -348,6 +348,7 @@ static void process_incoming_migration_bh(void *opaque)
MIGRATION_STATUS_FAILED);
error_report_err(local_err);
migrate_decompress_threads_join();
+ migrate_multifd_recv_threads_join();
exit(EXIT_FAILURE);
}
@@ -372,6 +373,7 @@ static void process_incoming_migration_bh(void *opaque)
runstate_set(global_state_get_runstate());
}
migrate_decompress_threads_join();
+ migrate_multifd_recv_threads_join();
/*
* This must happen after any state changes since as soon as an external
* observer sees this event they might start to prod at the VM assuming
@@ -438,6 +440,7 @@ static void process_incoming_migration_co(void *opaque)
MIGRATION_STATUS_FAILED);
error_report("load of migration failed: %s", strerror(-ret));
migrate_decompress_threads_join();
+ migrate_multifd_recv_threads_join();
exit(EXIT_FAILURE);
}
@@ -450,6 +453,11 @@ void migration_fd_process_incoming(QEMUFile *f)
Coroutine *co = qemu_coroutine_create(process_incoming_migration_co, f);
migrate_decompress_threads_create();
+ if (migrate_multifd_recv_threads_create() != 0) {
+ /* We haven't been able to create multifd threads
+ nothing better to do */
+ exit(EXIT_FAILURE);
+ }
qemu_file_set_blocking(f, false);
qemu_coroutine_enter(co);
}
@@ -983,6 +991,7 @@ static void migrate_fd_cleanup(void *opaque)
qemu_mutex_lock_iothread();
migrate_compress_threads_join();
+ migrate_multifd_send_threads_join();
qemu_fclose(s->to_dst_file);
s->to_dst_file = NULL;
}
@@ -2144,6 +2153,12 @@ void migrate_fd_connect(MigrationState *s)
}
migrate_compress_threads_create();
+ if (migrate_multifd_send_threads_create() != 0) {
+ migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+ MIGRATION_STATUS_FAILED);
+ migrate_fd_cleanup(s);
+ return;
+ }
qemu_thread_create(&s->thread, "live_migration", migration_thread, s,
QEMU_THREAD_JOINABLE);
s->migration_thread_running = true;
diff --git a/migration/ram.c b/migration/ram.c
index aa51dbd..ee32fa8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -382,6 +382,194 @@ void migrate_compress_threads_create(void)
}
}
+/* Multiple fd's */
+
+struct MultiFDSendParams {
+ int id;
+ QemuThread thread;
+ QemuSemaphore sem;
+ QemuMutex mutex;
+ bool quit;
+};
+typedef struct MultiFDSendParams MultiFDSendParams;
+
+struct {
+ MultiFDSendParams *params;
+ /* number o6 created threads */
+ int count;
+} *multifd_send_state;
+
+static void terminate_multifd_send_threads(void)
+{
+ int i;
+
+ for (i = 0; i < multifd_send_state->count; i++) {
+ MultiFDSendParams *p = &multifd_send_state->params[i];
+
+ qemu_mutex_lock(&p->mutex);
+ p->quit = true;
+ qemu_sem_post(&p->sem);
+ qemu_mutex_unlock(&p->mutex);
+ }
+}
+
+void migrate_multifd_send_threads_join(void)
+{
+ int i;
+
+ if (!migrate_use_multifd()) {
+ return;
+ }
+ terminate_multifd_send_threads();
+ for (i = 0; i < multifd_send_state->count; i++) {
+ MultiFDSendParams *p = &multifd_send_state->params[i];
+
+ qemu_thread_join(&p->thread);
+ qemu_mutex_destroy(&p->mutex);
+ qemu_sem_destroy(&p->sem);
+ }
+ g_free(multifd_send_state->params);
+ multifd_send_state->params = NULL;
+ g_free(multifd_send_state);
+ multifd_send_state = NULL;
+}
+
+static void *multifd_send_thread(void *opaque)
+{
+ MultiFDSendParams *p = opaque;
+
+ while (true) {
+ qemu_mutex_lock(&p->mutex);
+ if (p->quit) {
+ qemu_mutex_unlock(&p->mutex);
+ break;
+ }
+ qemu_mutex_unlock(&p->mutex);
+ qemu_sem_wait(&p->sem);
+ }
+
+ return NULL;
+}
+
+int migrate_multifd_send_threads_create(void)
+{
+ int i, thread_count;
+
+ if (!migrate_use_multifd()) {
+ return 0;
+ }
+ thread_count = migrate_multifd_threads();
+ multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
+ multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
+ multifd_send_state->count = 0;
+ for (i = 0; i < thread_count; i++) {
+ char thread_name[15];
+ MultiFDSendParams *p = &multifd_send_state->params[i];
+
+ qemu_mutex_init(&p->mutex);
+ qemu_sem_init(&p->sem, 0);
+ p->quit = false;
+ p->id = i;
+ snprintf(thread_name, 15, "multifd_send_%d", i);
+ qemu_thread_create(&p->thread, thread_name, multifd_send_thread, p,
+ QEMU_THREAD_JOINABLE);
+ multifd_send_state->count++;
+ }
+ return 0;
+}
+
+struct MultiFDRecvParams {
+ int id;
+ QemuThread thread;
+ QemuSemaphore sem;
+ QemuMutex mutex;
+ bool quit;
+};
+typedef struct MultiFDRecvParams MultiFDRecvParams;
+
+struct {
+ MultiFDRecvParams *params;
+ /* number o6 created threads */
+ int count;
+} *multifd_recv_state;
+
+static void terminate_multifd_recv_threads(void)
+{
+ int i;
+
+ for (i = 0; i < multifd_recv_state->count; i++) {
+ MultiFDRecvParams *p = &multifd_recv_state->params[i];
+
+ qemu_mutex_lock(&p->mutex);
+ p->quit = true;
+ qemu_sem_post(&p->sem);
+ qemu_mutex_unlock(&p->mutex);
+ }
+}
+
+void migrate_multifd_recv_threads_join(void)
+{
+ int i;
+
+ if (!migrate_use_multifd()) {
+ return;
+ }
+ terminate_multifd_recv_threads();
+ for (i = 0; i < multifd_recv_state->count; i++) {
+ MultiFDRecvParams *p = &multifd_recv_state->params[i];
+
+ qemu_thread_join(&p->thread);
+ qemu_mutex_destroy(&p->mutex);
+ qemu_sem_destroy(&p->sem);
+ }
+ g_free(multifd_recv_state->params);
+ multifd_recv_state->params = NULL;
+ g_free(multifd_recv_state);
+ multifd_recv_state = NULL;
+}
+
+static void *multifd_recv_thread(void *opaque)
+{
+ MultiFDRecvParams *p = opaque;
+
+ while (true) {
+ qemu_mutex_lock(&p->mutex);
+ if (p->quit) {
+ qemu_mutex_unlock(&p->mutex);
+ break;
+ }
+ qemu_mutex_unlock(&p->mutex);
+ qemu_sem_wait(&p->sem);
+ }
+
+ return NULL;
+}
+
+int migrate_multifd_recv_threads_create(void)
+{
+ int i, thread_count;
+
+ 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->count = 0;
+ for (i = 0; i < thread_count; i++) {
+ MultiFDRecvParams *p = &multifd_recv_state->params[i];
+
+ qemu_mutex_init(&p->mutex);
+ qemu_sem_init(&p->sem, 0);
+ p->quit = false;
+ p->id = i;
+ qemu_thread_create(&p->thread, "multifd_recv", multifd_recv_thread, p,
+ QEMU_THREAD_JOINABLE);
+ multifd_recv_state->count++;
+ }
+ return 0;
+}
+
/**
* save_page_header: Write page header to wire
*
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (7 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 08/16] migration: Create multifd migration threads Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 16:41 ` Daniel P. Berrange
2017-03-13 12:44 ` [Qemu-devel] [PATCH 10/16] migration: Create ram_multifd_page Juan Quintela
` (7 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
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 <quintela@redhat.com>
--
Split SocketArgs into incoming and outgoing args
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
include/migration/migration.h | 7 +++++
migration/ram.c | 35 ++++++++++++++++++++++
migration/socket.c | 67 +++++++++++++++++++++++++++++++++++++++++--
3 files changed, 106 insertions(+), 3 deletions(-)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index e8b9fcb..cbb049d 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -23,6 +23,7 @@
#include "exec/cpu-common.h"
#include "qemu/coroutine_int.h"
#include "qom/object.h"
+#include "io/channel.h"
#define QEMU_VM_FILE_MAGIC 0x5145564d
#define QEMU_VM_FILE_VERSION_COMPAT 0x00000002
@@ -235,6 +236,12 @@ 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);
+int socket_recv_channel_close_listening(void);
+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 ee32fa8..7833e6f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -387,7 +387,9 @@ void migrate_compress_threads_create(void)
struct MultiFDSendParams {
int id;
QemuThread thread;
+ QIOChannel *c;
QemuSemaphore sem;
+ QemuSemaphore init;
QemuMutex mutex;
bool quit;
};
@@ -427,6 +429,8 @@ void migrate_multifd_send_threads_join(void)
qemu_thread_join(&p->thread);
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
+ qemu_sem_destroy(&p->init);
+ socket_send_channel_destroy(p->c);
}
g_free(multifd_send_state->params);
multifd_send_state->params = NULL;
@@ -438,6 +442,11 @@ static void *multifd_send_thread(void *opaque)
{
MultiFDSendParams *p = opaque;
+ char start = 's';
+
+ qio_channel_write(p->c, &start, 1, &error_abort);
+ qemu_sem_post(&p->init);
+
while (true) {
qemu_mutex_lock(&p->mutex);
if (p->quit) {
@@ -468,12 +477,20 @@ int migrate_multifd_send_threads_create(void)
qemu_mutex_init(&p->mutex);
qemu_sem_init(&p->sem, 0);
+ qemu_sem_init(&p->init, 0);
p->quit = false;
p->id = i;
+ p->c = socket_send_channel_create();
+ if (!p->c) {
+ error_report("Error creating a send channel");
+ migrate_multifd_send_threads_join();
+ return -1;
+ }
snprintf(thread_name, 15, "multifd_send_%d", i);
qemu_thread_create(&p->thread, thread_name, multifd_send_thread, p,
QEMU_THREAD_JOINABLE);
multifd_send_state->count++;
+ qemu_sem_wait(&p->init);
}
return 0;
}
@@ -481,6 +498,8 @@ int migrate_multifd_send_threads_create(void)
struct MultiFDRecvParams {
int id;
QemuThread thread;
+ QIOChannel *c;
+ QemuSemaphore init;
QemuSemaphore sem;
QemuMutex mutex;
bool quit;
@@ -521,6 +540,8 @@ void migrate_multifd_recv_threads_join(void)
qemu_thread_join(&p->thread);
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
+ qemu_sem_destroy(&p->init);
+ socket_send_channel_destroy(multifd_recv_state->params[i].c);
}
g_free(multifd_recv_state->params);
multifd_recv_state->params = NULL;
@@ -531,6 +552,10 @@ void migrate_multifd_recv_threads_join(void)
static void *multifd_recv_thread(void *opaque)
{
MultiFDRecvParams *p = opaque;
+ char start;
+
+ qio_channel_read(p->c, &start, 1, &error_abort);
+ qemu_sem_post(&p->init);
while (true) {
qemu_mutex_lock(&p->mutex);
@@ -561,12 +586,22 @@ int migrate_multifd_recv_threads_create(void)
qemu_mutex_init(&p->mutex);
qemu_sem_init(&p->sem, 0);
+ qemu_sem_init(&p->init, 0);
p->quit = false;
p->id = i;
+ p->c = socket_recv_channel_create();
+
+ if (!p->c) {
+ error_report("Error creating a recv channel");
+ migrate_multifd_recv_threads_join();
+ return -1;
+ }
qemu_thread_create(&p->thread, "multifd_recv", multifd_recv_thread, p,
QEMU_THREAD_JOINABLE);
multifd_recv_state->count++;
+ qemu_sem_wait(&p->init);
}
+ socket_recv_channel_close_listening();
return 0;
}
diff --git a/migration/socket.c b/migration/socket.c
index 13966f1..58a16b5 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -24,6 +24,65 @@
#include "io/channel-socket.h"
#include "trace.h"
+struct SocketIncomingArgs {
+ QIOChannelSocket *ioc;
+} incoming_args;
+
+QIOChannel *socket_recv_channel_create(void)
+{
+ QIOChannelSocket *sioc;
+ Error *err = NULL;
+
+ sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(incoming_args.ioc),
+ &err);
+ if (!sioc) {
+ error_report("could not accept migration connection (%s)",
+ error_get_pretty(err));
+ return NULL;
+ }
+ return QIO_CHANNEL(sioc);
+}
+
+int socket_recv_channel_destroy(QIOChannel *recv)
+{
+ /* Remove channel */
+ object_unref(OBJECT(send));
+ return 0;
+}
+
+/* we have created all the recv channels, we can close the main one */
+int socket_recv_channel_close_listening(void)
+{
+ /* Close listening socket as its no longer needed */
+ qio_channel_close(QIO_CHANNEL(incoming_args.ioc), NULL);
+ return 0;
+}
+
+struct SocketOutgoingArgs {
+ SocketAddress *saddr;
+ Error **errp;
+} outgoing_args;
+
+QIOChannel *socket_send_channel_create(void)
+{
+ QIOChannelSocket *sioc = qio_channel_socket_new();
+
+ qio_channel_socket_connect_sync(sioc, outgoing_args.saddr,
+ outgoing_args.errp);
+ qio_channel_set_delay(QIO_CHANNEL(sioc), false);
+ return QIO_CHANNEL(sioc);
+}
+
+int socket_send_channel_destroy(QIOChannel *send)
+{
+ /* Remove channel */
+ object_unref(OBJECT(send));
+ if (outgoing_args.saddr) {
+ qapi_free_SocketAddress(outgoing_args.saddr);
+ outgoing_args.saddr = NULL;
+ }
+ return 0;
+}
static SocketAddress *tcp_build_address(const char *host_port, Error **errp)
{
@@ -97,6 +156,10 @@ static void socket_start_outgoing_migration(MigrationState *s,
struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
data->s = s;
+
+ outgoing_args.saddr = saddr;
+ outgoing_args.errp = errp;
+
if (saddr->type == SOCKET_ADDRESS_KIND_INET) {
data->hostname = g_strdup(saddr->u.inet.data->host);
}
@@ -107,7 +170,6 @@ static void socket_start_outgoing_migration(MigrationState *s,
socket_outgoing_migration,
data,
socket_connect_data_free);
- qapi_free_SocketAddress(saddr);
}
void tcp_start_outgoing_migration(MigrationState *s,
@@ -154,8 +216,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
object_unref(OBJECT(sioc));
out:
- /* Close listening socket as its no longer needed */
- qio_channel_close(ioc, NULL);
return FALSE; /* unregister */
}
@@ -164,6 +224,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
Error **errp)
{
QIOChannelSocket *listen_ioc = qio_channel_socket_new();
+ incoming_args.ioc = listen_ioc;
qio_channel_set_name(QIO_CHANNEL(listen_ioc),
"migration-socket-listener");
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work
2017-03-13 12:44 ` [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work Juan Quintela
@ 2017-03-13 16:41 ` Daniel P. Berrange
2017-03-13 16:58 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-13 16:41 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 01:44:27PM +0100, 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 <quintela@redhat.com>
>
> --
> Split SocketArgs into incoming and outgoing args
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> include/migration/migration.h | 7 +++++
> migration/ram.c | 35 ++++++++++++++++++++++
> migration/socket.c | 67 +++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/migration/socket.c b/migration/socket.c
> index 13966f1..58a16b5 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -24,6 +24,65 @@
> #include "io/channel-socket.h"
> #include "trace.h"
>
> +struct SocketIncomingArgs {
> + QIOChannelSocket *ioc;
> +} incoming_args;
> +
> +QIOChannel *socket_recv_channel_create(void)
> +{
> + QIOChannelSocket *sioc;
> + Error *err = NULL;
> +
> + sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(incoming_args.ioc),
> + &err);
> + if (!sioc) {
> + error_report("could not accept migration connection (%s)",
> + error_get_pretty(err));
> + return NULL;
> + }
> + return QIO_CHANNEL(sioc);
> +}
> +
> +int socket_recv_channel_destroy(QIOChannel *recv)
> +{
> + /* Remove channel */
> + object_unref(OBJECT(send));
> + return 0;
> +}
> +
> +/* we have created all the recv channels, we can close the main one */
> +int socket_recv_channel_close_listening(void)
> +{
> + /* Close listening socket as its no longer needed */
> + qio_channel_close(QIO_CHANNEL(incoming_args.ioc), NULL);
> + return 0;
> +}
> +
> +struct SocketOutgoingArgs {
> + SocketAddress *saddr;
> + Error **errp;
> +} outgoing_args;
> +
> +QIOChannel *socket_send_channel_create(void)
> +{
> + QIOChannelSocket *sioc = qio_channel_socket_new();
> +
> + qio_channel_socket_connect_sync(sioc, outgoing_args.saddr,
> + outgoing_args.errp);
> + qio_channel_set_delay(QIO_CHANNEL(sioc), false);
> + return QIO_CHANNEL(sioc);
> +}
> +
> +int socket_send_channel_destroy(QIOChannel *send)
> +{
> + /* Remove channel */
> + object_unref(OBJECT(send));
> + if (outgoing_args.saddr) {
> + qapi_free_SocketAddress(outgoing_args.saddr);
> + outgoing_args.saddr = NULL;
> + }
> + return 0;
> +}
>
> static SocketAddress *tcp_build_address(const char *host_port, Error **errp)
> {
> @@ -97,6 +156,10 @@ static void socket_start_outgoing_migration(MigrationState *s,
> struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
>
> data->s = s;
> +
> + outgoing_args.saddr = saddr;
> + outgoing_args.errp = errp;
> +
> if (saddr->type == SOCKET_ADDRESS_KIND_INET) {
> data->hostname = g_strdup(saddr->u.inet.data->host);
> }
> @@ -107,7 +170,6 @@ static void socket_start_outgoing_migration(MigrationState *s,
> socket_outgoing_migration,
> data,
> socket_connect_data_free);
> - qapi_free_SocketAddress(saddr);
> }
>
> void tcp_start_outgoing_migration(MigrationState *s,
> @@ -154,8 +216,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
> object_unref(OBJECT(sioc));
>
> out:
> - /* Close listening socket as its no longer needed */
> - qio_channel_close(ioc, NULL);
> return FALSE; /* unregister */
> }
>
> @@ -164,6 +224,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
> Error **errp)
> {
> QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> + incoming_args.ioc = listen_ioc;
>
> qio_channel_set_name(QIO_CHANNEL(listen_ioc),
> "migration-socket-listener");
I still don't really like any of the changes in this file. We've now got
two sets of methods which connect to a remote host and two sets of methods
which accept incoming clients. I've got to think there's a better way to
refactor the existing code, such that we don't need two sets of methods
for the same actions
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work
2017-03-13 16:41 ` Daniel P. Berrange
@ 2017-03-13 16:58 ` Juan Quintela
2017-03-14 10:34 ` Daniel P. Berrange
0 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 16:58 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, amit.shah, dgilbert
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Mar 13, 2017 at 01:44:27PM +0100, 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 <quintela@redhat.com>
>>
>> --
>> Split SocketArgs into incoming and outgoing args
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> include/migration/migration.h | 7 +++++
>> migration/ram.c | 35 ++++++++++++++++++++++
>> migration/socket.c | 67 +++++++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 106 insertions(+), 3 deletions(-)
>>
>
>> diff --git a/migration/socket.c b/migration/socket.c
>> index 13966f1..58a16b5 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -24,6 +24,65 @@
>> #include "io/channel-socket.h"
>> #include "trace.h"
>>
>> +struct SocketIncomingArgs {
>> + QIOChannelSocket *ioc;
>> +} incoming_args;
>> +
>> +QIOChannel *socket_recv_channel_create(void)
>> +{
>> + QIOChannelSocket *sioc;
>> + Error *err = NULL;
>> +
>> + sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(incoming_args.ioc),
>> + &err);
>> + if (!sioc) {
>> + error_report("could not accept migration connection (%s)",
>> + error_get_pretty(err));
>> + return NULL;
>> + }
>> + return QIO_CHANNEL(sioc);
>> +}
>> +
>> +int socket_recv_channel_destroy(QIOChannel *recv)
>> +{
>> + /* Remove channel */
>> + object_unref(OBJECT(send));
>> + return 0;
>> +}
>> +
>> +/* we have created all the recv channels, we can close the main one */
>> +int socket_recv_channel_close_listening(void)
>> +{
>> + /* Close listening socket as its no longer needed */
>> + qio_channel_close(QIO_CHANNEL(incoming_args.ioc), NULL);
>> + return 0;
>> +}
>> +
>> +struct SocketOutgoingArgs {
>> + SocketAddress *saddr;
>> + Error **errp;
>> +} outgoing_args;
>> +
>> +QIOChannel *socket_send_channel_create(void)
>> +{
>> + QIOChannelSocket *sioc = qio_channel_socket_new();
>> +
>> + qio_channel_socket_connect_sync(sioc, outgoing_args.saddr,
>> + outgoing_args.errp);
>> + qio_channel_set_delay(QIO_CHANNEL(sioc), false);
>> + return QIO_CHANNEL(sioc);
>> +}
>> +
>> +int socket_send_channel_destroy(QIOChannel *send)
>> +{
>> + /* Remove channel */
>> + object_unref(OBJECT(send));
>> + if (outgoing_args.saddr) {
>> + qapi_free_SocketAddress(outgoing_args.saddr);
>> + outgoing_args.saddr = NULL;
>> + }
>> + return 0;
>> +}
>>
>> static SocketAddress *tcp_build_address(const char *host_port, Error **errp)
>> {
>> @@ -97,6 +156,10 @@ static void socket_start_outgoing_migration(MigrationState *s,
>> struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
>>
>> data->s = s;
>> +
>> + outgoing_args.saddr = saddr;
>> + outgoing_args.errp = errp;
>> +
>> if (saddr->type == SOCKET_ADDRESS_KIND_INET) {
>> data->hostname = g_strdup(saddr->u.inet.data->host);
>> }
>> @@ -107,7 +170,6 @@ static void socket_start_outgoing_migration(MigrationState *s,
>> socket_outgoing_migration,
>> data,
>> socket_connect_data_free);
>> - qapi_free_SocketAddress(saddr);
>> }
>>
>> void tcp_start_outgoing_migration(MigrationState *s,
>> @@ -154,8 +216,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
>> object_unref(OBJECT(sioc));
>>
>> out:
>> - /* Close listening socket as its no longer needed */
>> - qio_channel_close(ioc, NULL);
>> return FALSE; /* unregister */
*HERE*
>> }
>>
>> @@ -164,6 +224,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>> Error **errp)
>> {
>> QIOChannelSocket *listen_ioc = qio_channel_socket_new();
>> + incoming_args.ioc = listen_ioc;
>>
>> qio_channel_set_name(QIO_CHANNEL(listen_ioc),
>> "migration-socket-listener");
>
> I still don't really like any of the changes in this file. We've now got
> two sets of methods which connect to a remote host and two sets of methods
> which accept incoming clients. I've got to think there's a better way to
> refactor the existing code, such that we don't need two sets of methods
> for the same actions
I am open to suggestions, basically we want to be able to:
- open one + n channels
- be sure that we got the same id on both sides of the connection.
You suggested on the previous iteration that I changed the FALSE in
*HERE* for TRUE, but I was not able to:
- make sure that we have opened n sockets before we continue with
migration
- making sure that we got same id numbers in both sides, that is doable,
just add a new id field
- right now I open a channel, and wait for the other side to open it
before open the following one. I can do things in parallel, but
locking is going to be "interesting".
So, as said, I don't really care how we open the channels, I am totally
open to suggestions. Looking at the current code, this is the best way
that I have been able to think of.
Later, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work
2017-03-13 16:58 ` Juan Quintela
@ 2017-03-14 10:34 ` Daniel P. Berrange
2017-03-14 12:32 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-14 10:34 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel, amit.shah, dgilbert
On Mon, Mar 13, 2017 at 05:58:06PM +0100, Juan Quintela wrote:
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > On Mon, Mar 13, 2017 at 01:44:27PM +0100, 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 <quintela@redhat.com>
> >>
> >> --
> >> Split SocketArgs into incoming and outgoing args
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >> include/migration/migration.h | 7 +++++
> >> migration/ram.c | 35 ++++++++++++++++++++++
> >> migration/socket.c | 67 +++++++++++++++++++++++++++++++++++++++++--
> >> 3 files changed, 106 insertions(+), 3 deletions(-)
> >>
> >
> >> diff --git a/migration/socket.c b/migration/socket.c
> >> index 13966f1..58a16b5 100644
> >> --- a/migration/socket.c
> >> +++ b/migration/socket.c
> >> @@ -24,6 +24,65 @@
> >> #include "io/channel-socket.h"
> >> #include "trace.h"
> >>
> >> +struct SocketIncomingArgs {
> >> + QIOChannelSocket *ioc;
> >> +} incoming_args;
> >> +
> >> +QIOChannel *socket_recv_channel_create(void)
> >> +{
> >> + QIOChannelSocket *sioc;
> >> + Error *err = NULL;
> >> +
> >> + sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(incoming_args.ioc),
> >> + &err);
> >> + if (!sioc) {
> >> + error_report("could not accept migration connection (%s)",
> >> + error_get_pretty(err));
> >> + return NULL;
> >> + }
> >> + return QIO_CHANNEL(sioc);
> >> +}
> >> +
> >> +int socket_recv_channel_destroy(QIOChannel *recv)
> >> +{
> >> + /* Remove channel */
> >> + object_unref(OBJECT(send));
> >> + return 0;
> >> +}
> >> +
> >> +/* we have created all the recv channels, we can close the main one */
> >> +int socket_recv_channel_close_listening(void)
> >> +{
> >> + /* Close listening socket as its no longer needed */
> >> + qio_channel_close(QIO_CHANNEL(incoming_args.ioc), NULL);
> >> + return 0;
> >> +}
> >> +
> >> +struct SocketOutgoingArgs {
> >> + SocketAddress *saddr;
> >> + Error **errp;
> >> +} outgoing_args;
> >> +
> >> +QIOChannel *socket_send_channel_create(void)
> >> +{
> >> + QIOChannelSocket *sioc = qio_channel_socket_new();
> >> +
> >> + qio_channel_socket_connect_sync(sioc, outgoing_args.saddr,
> >> + outgoing_args.errp);
> >> + qio_channel_set_delay(QIO_CHANNEL(sioc), false);
> >> + return QIO_CHANNEL(sioc);
> >> +}
> >> +
> >> +int socket_send_channel_destroy(QIOChannel *send)
> >> +{
> >> + /* Remove channel */
> >> + object_unref(OBJECT(send));
> >> + if (outgoing_args.saddr) {
> >> + qapi_free_SocketAddress(outgoing_args.saddr);
> >> + outgoing_args.saddr = NULL;
> >> + }
> >> + return 0;
> >> +}
> >>
> >> static SocketAddress *tcp_build_address(const char *host_port, Error **errp)
> >> {
> >> @@ -97,6 +156,10 @@ static void socket_start_outgoing_migration(MigrationState *s,
> >> struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
> >>
> >> data->s = s;
> >> +
> >> + outgoing_args.saddr = saddr;
> >> + outgoing_args.errp = errp;
> >> +
> >> if (saddr->type == SOCKET_ADDRESS_KIND_INET) {
> >> data->hostname = g_strdup(saddr->u.inet.data->host);
> >> }
> >> @@ -107,7 +170,6 @@ static void socket_start_outgoing_migration(MigrationState *s,
> >> socket_outgoing_migration,
> >> data,
> >> socket_connect_data_free);
> >> - qapi_free_SocketAddress(saddr);
> >> }
> >>
> >> void tcp_start_outgoing_migration(MigrationState *s,
> >> @@ -154,8 +216,6 @@ static gboolean socket_accept_incoming_migration(QIOChannel *ioc,
> >> object_unref(OBJECT(sioc));
> >>
> >> out:
> >> - /* Close listening socket as its no longer needed */
> >> - qio_channel_close(ioc, NULL);
> >> return FALSE; /* unregister */
>
> *HERE*
>
>
> >> }
> >>
> >> @@ -164,6 +224,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
> >> Error **errp)
> >> {
> >> QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> >> + incoming_args.ioc = listen_ioc;
> >>
> >> qio_channel_set_name(QIO_CHANNEL(listen_ioc),
> >> "migration-socket-listener");
> >
> > I still don't really like any of the changes in this file. We've now got
> > two sets of methods which connect to a remote host and two sets of methods
> > which accept incoming clients. I've got to think there's a better way to
> > refactor the existing code, such that we don't need two sets of methods
> > for the same actions
>
> I am open to suggestions, basically we want to be able to:
> - open one + n channels
> - be sure that we got the same id on both sides of the connection.
>
> You suggested on the previous iteration that I changed the FALSE in
> *HERE* for TRUE, but I was not able to:
> - make sure that we have opened n sockets before we continue with
> migration
> - making sure that we got same id numbers in both sides, that is doable,
> just add a new id field
> - right now I open a channel, and wait for the other side to open it
> before open the following one. I can do things in parallel, but
> locking is going to be "interesting".
>
> So, as said, I don't really care how we open the channels, I am totally
> open to suggestions. Looking at the current code, this is the best way
> that I have been able to think of.
I think the key problem in the current design is that you delay the opening
of the extra socket channels. To be able to remove most of this duplication,
I think you need to open all the channels at once right at the start.
IOW, in qmp_migrate() instead of calling tcp_start_outgoing_migration()
just once, use a loop to call it N times (where N == number of threads).
Now this method is asynchronous, and eventually triggers a call to
migration_channel_connect() when the connection actually succeeds.
You will need to change migration_channel_connect() so that it can be
called multiple times. migration_channel_connect() should count how
many channels have been opened, and only start the migration once all
of them are open.
The incoming side is a little different - in qemu_start_incoming_migration()
you only need call tcp_start_incoming_migration() once. In the
socket_accept_incoming_migration() method though, you need to change the
'return FALSE' to 'return TRUE', so that it continues to accept multiple
incoming clients. The socket_start_outgoing_migration()method needs to again
count the number of channels that have been opened so far, and only start
the actual migration once the right number are open.
By doing all this opening of channels upfront, you'll also make it much
easier to support the other migration protocols - in particular 'fd'
protocol needs to be extended so that libvirt can pass in multiple FDs
in the monitor command at once. The 'exec' protocol should also be
able to trivially support this by simply launching the command multiple
times.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work
2017-03-14 10:34 ` Daniel P. Berrange
@ 2017-03-14 12:32 ` Juan Quintela
0 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-14 12:32 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, amit.shah, dgilbert
"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Mar 13, 2017 at 05:58:06PM +0100, Juan Quintela wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> > On Mon, Mar 13, 2017 at 01:44:27PM +0100, Juan Quintela wrote:
>> >
>> > I still don't really like any of the changes in this file. We've now got
>> > two sets of methods which connect to a remote host and two sets of methods
>> > which accept incoming clients. I've got to think there's a better way to
>> > refactor the existing code, such that we don't need two sets of methods
>> > for the same actions
>>
>> I am open to suggestions, basically we want to be able to:
>> - open one + n channels
>> - be sure that we got the same id on both sides of the connection.
>>
>> You suggested on the previous iteration that I changed the FALSE in
>> *HERE* for TRUE, but I was not able to:
>> - make sure that we have opened n sockets before we continue with
>> migration
>> - making sure that we got same id numbers in both sides, that is doable,
>> just add a new id field
>> - right now I open a channel, and wait for the other side to open it
>> before open the following one. I can do things in parallel, but
>> locking is going to be "interesting".
>>
>> So, as said, I don't really care how we open the channels, I am totally
>> open to suggestions. Looking at the current code, this is the best way
>> that I have been able to think of.
>
> I think the key problem in the current design is that you delay the opening
> of the extra socket channels. To be able to remove most of this duplication,
> I think you need to open all the channels at once right at the start.
>
>
> IOW, in qmp_migrate() instead of calling tcp_start_outgoing_migration()
> just once, use a loop to call it N times (where N == number of threads).
>
> Now this method is asynchronous, and eventually triggers a call to
> migration_channel_connect() when the connection actually succeeds.
> You will need to change migration_channel_connect() so that it can be
> called multiple times. migration_channel_connect() should count how
> many channels have been opened, and only start the migration once all
> of them are open.
>
> The incoming side is a little different - in qemu_start_incoming_migration()
> you only need call tcp_start_incoming_migration() once. In the
> socket_accept_incoming_migration() method though, you need to change the
> 'return FALSE' to 'return TRUE', so that it continues to accept multiple
> incoming clients. The socket_start_outgoing_migration()method needs to again
> count the number of channels that have been opened so far, and only start
> the actual migration once the right number are open.
>
>
> By doing all this opening of channels upfront, you'll also make it much
> easier to support the other migration protocols - in particular 'fd'
> protocol needs to be extended so that libvirt can pass in multiple FDs
> in the monitor command at once. The 'exec' protocol should also be
> able to trivially support this by simply launching the command multiple
> times.
Ok. Thanks. Will look into this.
Later, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 10/16] migration: Create ram_multifd_page
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (8 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 09/16] migration: Start of multiple fd work Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 11/16] migration: Really use multiple pages at a time Juan Quintela
` (6 subsequent siblings)
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
The function still don't use multifd, but we have simplified
ram_save_page, xbzrle and RDMA stuff is gone. We have added a new
counter and a new flag for this type of pages.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
hmp.c | 2 +
include/migration/migration.h | 1 +
migration/migration.c | 1 +
migration/ram.c | 98 ++++++++++++++++++++++++++++++++++++++++++-
qapi-schema.json | 4 +-
5 files changed, 103 insertions(+), 3 deletions(-)
diff --git a/hmp.c b/hmp.c
index ab02773..1238d0f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -223,6 +223,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
info->ram->postcopy_requests);
}
+ monitor_printf(mon, "multifd: %" PRIu64 " pages\n",
+ info->ram->multifd);
}
if (info->has_disk) {
diff --git a/include/migration/migration.h b/include/migration/migration.h
index cbb049d..bd152c5 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -301,6 +301,7 @@ uint64_t xbzrle_mig_pages_transferred(void);
uint64_t xbzrle_mig_pages_overflow(void);
uint64_t xbzrle_mig_pages_cache_miss(void);
double xbzrle_mig_cache_miss_rate(void);
+uint64_t multifd_mig_pages_transferred(void);
void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
void ram_debug_dump_bitmap(unsigned long *todump, bool expected);
diff --git a/migration/migration.c b/migration/migration.c
index 5bbd688..a32e4ad 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -661,6 +661,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
info->ram->mbps = s->mbps;
info->ram->dirty_sync_count = s->dirty_sync_count;
info->ram->postcopy_requests = s->postcopy_requests;
+ info->ram->multifd = multifd_mig_pages_transferred();
if (s->state != MIGRATION_STATUS_COMPLETED) {
info->ram->remaining = ram_bytes_remaining();
diff --git a/migration/ram.c b/migration/ram.c
index 7833e6f..ccd7fe9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -61,6 +61,7 @@ static uint64_t bitmap_sync_count;
#define RAM_SAVE_FLAG_XBZRLE 0x40
/* 0x80 is reserved in migration.h start with 0x100 next */
#define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100
+#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
static uint8_t *ZERO_TARGET_PAGE;
@@ -141,6 +142,7 @@ typedef struct AccountingInfo {
uint64_t dup_pages;
uint64_t skipped_pages;
uint64_t norm_pages;
+ uint64_t multifd_pages;
uint64_t iterations;
uint64_t xbzrle_bytes;
uint64_t xbzrle_pages;
@@ -211,6 +213,11 @@ uint64_t xbzrle_mig_pages_overflow(void)
return acct_info.xbzrle_overflows;
}
+uint64_t multifd_mig_pages_transferred(void)
+{
+ return acct_info.multifd_pages;
+}
+
/* This is the last block that we have visited serching for dirty pages
*/
static RAMBlock *last_seen_block;
@@ -385,13 +392,18 @@ void migrate_compress_threads_create(void)
/* Multiple fd's */
struct MultiFDSendParams {
+ /* not changed */
int id;
QemuThread thread;
QIOChannel *c;
QemuSemaphore sem;
QemuSemaphore init;
QemuMutex mutex;
+ /* protected by param mutex */
bool quit;
+ uint8_t *address;
+ /* protected by multifd mutex */
+ bool done;
};
typedef struct MultiFDSendParams MultiFDSendParams;
@@ -399,6 +411,8 @@ struct {
MultiFDSendParams *params;
/* number o6 created threads */
int count;
+ QemuMutex mutex;
+ QemuSemaphore sem;
} *multifd_send_state;
static void terminate_multifd_send_threads(void)
@@ -441,11 +455,11 @@ void migrate_multifd_send_threads_join(void)
static void *multifd_send_thread(void *opaque)
{
MultiFDSendParams *p = opaque;
-
char start = 's';
qio_channel_write(p->c, &start, 1, &error_abort);
qemu_sem_post(&p->init);
+ qemu_sem_post(&multifd_send_state->sem);
while (true) {
qemu_mutex_lock(&p->mutex);
@@ -453,6 +467,15 @@ static void *multifd_send_thread(void *opaque)
qemu_mutex_unlock(&p->mutex);
break;
}
+ if (p->address) {
+ p->address = 0;
+ qemu_mutex_unlock(&p->mutex);
+ qemu_mutex_lock(&multifd_send_state->mutex);
+ p->done = true;
+ qemu_mutex_unlock(&multifd_send_state->mutex);
+ qemu_sem_post(&multifd_send_state->sem);
+ continue;
+ }
qemu_mutex_unlock(&p->mutex);
qemu_sem_wait(&p->sem);
}
@@ -471,6 +494,8 @@ int migrate_multifd_send_threads_create(void)
multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
multifd_send_state->count = 0;
+ qemu_mutex_init(&multifd_send_state->mutex);
+ qemu_sem_init(&multifd_send_state->sem, 0);
for (i = 0; i < thread_count; i++) {
char thread_name[15];
MultiFDSendParams *p = &multifd_send_state->params[i];
@@ -480,6 +505,8 @@ int migrate_multifd_send_threads_create(void)
qemu_sem_init(&p->init, 0);
p->quit = false;
p->id = i;
+ p->done = true;
+ p->address = 0;
p->c = socket_send_channel_create();
if (!p->c) {
error_report("Error creating a send channel");
@@ -495,6 +522,30 @@ int migrate_multifd_send_threads_create(void)
return 0;
}
+static int multifd_send_page(uint8_t *address)
+{
+ int i;
+ MultiFDSendParams *p = NULL; /* make happy gcc */
+
+ qemu_sem_wait(&multifd_send_state->sem);
+ qemu_mutex_lock(&multifd_send_state->mutex);
+ for (i = 0; i < multifd_send_state->count; i++) {
+ p = &multifd_send_state->params[i];
+
+ if (p->done) {
+ p->done = false;
+ break;
+ }
+ }
+ qemu_mutex_unlock(&multifd_send_state->mutex);
+ qemu_mutex_lock(&p->mutex);
+ p->address = address;
+ qemu_mutex_unlock(&p->mutex);
+ qemu_sem_post(&p->sem);
+
+ return 0;
+}
+
struct MultiFDRecvParams {
int id;
QemuThread thread;
@@ -1050,6 +1101,34 @@ static int ram_save_page(MigrationState *ms, QEMUFile *f, PageSearchStatus *pss,
return pages;
}
+static int ram_multifd_page(QEMUFile *f, PageSearchStatus *pss,
+ bool last_stage, uint64_t *bytes_transferred)
+{
+ int pages;
+ uint8_t *p;
+ RAMBlock *block = pss->block;
+ ram_addr_t offset = pss->offset;
+
+ p = block->host + offset;
+
+ if (block == last_sent_block) {
+ offset |= RAM_SAVE_FLAG_CONTINUE;
+ }
+ pages = save_zero_page(f, block, offset, p, bytes_transferred);
+ if (pages == -1) {
+ *bytes_transferred +=
+ save_page_header(f, block, offset | RAM_SAVE_FLAG_MULTIFD_PAGE);
+ qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
+ multifd_send_page(p);
+ *bytes_transferred += TARGET_PAGE_SIZE;
+ pages = 1;
+ acct_info.norm_pages++;
+ acct_info.multifd_pages++;
+ }
+
+ return pages;
+}
+
static int do_compress_ram_page(QEMUFile *f, RAMBlock *block,
ram_addr_t offset)
{
@@ -1495,6 +1574,8 @@ static int ram_save_target_page(MigrationState *ms, QEMUFile *f,
res = ram_save_compressed_page(ms, f, pss,
last_stage,
bytes_transferred);
+ } else if (migrate_use_multifd()) {
+ res = ram_multifd_page(f, pss, last_stage, bytes_transferred);
} else {
res = ram_save_page(ms, f, pss, last_stage,
bytes_transferred);
@@ -2765,6 +2846,10 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
if (!migrate_use_compression()) {
invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
}
+
+ if (!migrate_use_multifd()) {
+ invalid_flags |= RAM_SAVE_FLAG_MULTIFD_PAGE;
+ }
/* This RCU critical section can be very long running.
* When RCU reclaims in the code start to become numerous,
* it will be necessary to reduce the granularity of this
@@ -2789,13 +2874,17 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
if (flags & invalid_flags & RAM_SAVE_FLAG_COMPRESS_PAGE) {
error_report("Received an unexpected compressed page");
}
+ if (flags & invalid_flags & RAM_SAVE_FLAG_MULTIFD_PAGE) {
+ error_report("Received an unexpected multifd page");
+ }
ret = -EINVAL;
break;
}
if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
- RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
+ RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE |
+ RAM_SAVE_FLAG_MULTIFD_PAGE)) {
RAMBlock *block = ram_block_from_stream(f, flags);
host = host_from_ram_block_offset(block, addr);
@@ -2882,6 +2971,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
break;
}
break;
+
+ case RAM_SAVE_FLAG_MULTIFD_PAGE:
+ qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
+ break;
+
case RAM_SAVE_FLAG_EOS:
/* normal exit */
break;
diff --git a/qapi-schema.json b/qapi-schema.json
index 33a6267..0286b75 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -574,6 +574,7 @@
#
# @postcopy-requests: The number of page requests received from the destination
# (since 2.7)
+# @multifd: number of pages sent with multifd (since 2.9)
#
# Since: 0.14.0
##
@@ -582,7 +583,8 @@
'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
'mbps' : 'number', 'dirty-sync-count' : 'int',
- 'postcopy-requests' : 'int' } }
+ 'postcopy-requests' : 'int',
+ 'multifd' : 'int'} }
##
# @XBZRLECacheStats:
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 11/16] migration: Really use multiple pages at a time
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (9 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 10/16] migration: Create ram_multifd_page Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 12/16] migration: Send the fd number which we are going to use for this page Juan Quintela
` (5 subsequent siblings)
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
We now send several pages at a time each time that we wakeup a thread.
Signed-off-by: Juan Quintela <quintela@redhat.com>
--
Use iovec's insead of creating the equivalent.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 46 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index ccd7fe9..4914240 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -391,6 +391,13 @@ void migrate_compress_threads_create(void)
/* Multiple fd's */
+
+typedef struct {
+ int num;
+ int size;
+ struct iovec *iov;
+} multifd_pages_t;
+
struct MultiFDSendParams {
/* not changed */
int id;
@@ -401,7 +408,7 @@ struct MultiFDSendParams {
QemuMutex mutex;
/* protected by param mutex */
bool quit;
- uint8_t *address;
+ multifd_pages_t pages;
/* protected by multifd mutex */
bool done;
};
@@ -467,8 +474,8 @@ static void *multifd_send_thread(void *opaque)
qemu_mutex_unlock(&p->mutex);
break;
}
- if (p->address) {
- p->address = 0;
+ if (p->pages.num) {
+ p->pages.num = 0;
qemu_mutex_unlock(&p->mutex);
qemu_mutex_lock(&multifd_send_state->mutex);
p->done = true;
@@ -483,6 +490,13 @@ static void *multifd_send_thread(void *opaque)
return NULL;
}
+static void multifd_init_group(multifd_pages_t *pages)
+{
+ pages->num = 0;
+ pages->size = migrate_multifd_group();
+ pages->iov = g_malloc0(pages->size * sizeof(struct iovec));
+}
+
int migrate_multifd_send_threads_create(void)
{
int i, thread_count;
@@ -506,7 +520,7 @@ int migrate_multifd_send_threads_create(void)
p->quit = false;
p->id = i;
p->done = true;
- p->address = 0;
+ multifd_init_group(&p->pages);
p->c = socket_send_channel_create();
if (!p->c) {
error_report("Error creating a send channel");
@@ -524,8 +538,23 @@ int migrate_multifd_send_threads_create(void)
static int multifd_send_page(uint8_t *address)
{
- int i;
+ int i, j;
MultiFDSendParams *p = NULL; /* make happy gcc */
+ static multifd_pages_t pages;
+ static bool once;
+
+ if (!once) {
+ multifd_init_group(&pages);
+ once = true;
+ }
+
+ pages.iov[pages.num].iov_base = address;
+ pages.iov[pages.num].iov_len = TARGET_PAGE_SIZE;
+ pages.num++;
+
+ if (pages.num < (pages.size - 1)) {
+ return UINT16_MAX;
+ }
qemu_sem_wait(&multifd_send_state->sem);
qemu_mutex_lock(&multifd_send_state->mutex);
@@ -539,7 +568,12 @@ static int multifd_send_page(uint8_t *address)
}
qemu_mutex_unlock(&multifd_send_state->mutex);
qemu_mutex_lock(&p->mutex);
- p->address = address;
+ p->pages.num = pages.num;
+ for (j = 0; j < pages.size; j++) {
+ p->pages.iov[j].iov_base = pages.iov[j].iov_base;
+ p->pages.iov[j].iov_len = pages.iov[j].iov_len;
+ }
+ pages.num = 0;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 12/16] migration: Send the fd number which we are going to use for this page
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (10 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 11/16] migration: Really use multiple pages at a time Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side Juan Quintela
` (4 subsequent siblings)
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
We are still sending the page through the main channel, that would
change later in the series
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 4914240..6f5ca50 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -577,7 +577,7 @@ static int multifd_send_page(uint8_t *address)
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
- return 0;
+ return i;
}
struct MultiFDRecvParams {
@@ -1139,6 +1139,7 @@ static int ram_multifd_page(QEMUFile *f, PageSearchStatus *pss,
bool last_stage, uint64_t *bytes_transferred)
{
int pages;
+ uint16_t fd_num;
uint8_t *p;
RAMBlock *block = pss->block;
ram_addr_t offset = pss->offset;
@@ -1152,8 +1153,10 @@ static int ram_multifd_page(QEMUFile *f, PageSearchStatus *pss,
if (pages == -1) {
*bytes_transferred +=
save_page_header(f, block, offset | RAM_SAVE_FLAG_MULTIFD_PAGE);
+ fd_num = multifd_send_page(p);
+ qemu_put_be16(f, fd_num);
+ *bytes_transferred += 2; /* size of fd_num */
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
- multifd_send_page(p);
*bytes_transferred += TARGET_PAGE_SIZE;
pages = 1;
acct_info.norm_pages++;
@@ -2898,6 +2901,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
while (!postcopy_running && !ret && !(flags & RAM_SAVE_FLAG_EOS)) {
ram_addr_t addr, total_ram_bytes;
void *host = NULL;
+ uint16_t fd_num;
uint8_t ch;
addr = qemu_get_be64(f);
@@ -3007,6 +3011,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
break;
case RAM_SAVE_FLAG_MULTIFD_PAGE:
+ fd_num = qemu_get_be16(f);
+ if (fd_num != 0) {
+ /* this is yet an unused variable, changed later */
+ fd_num = fd_num;
+ }
qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
break;
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (11 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 12/16] migration: Send the fd number which we are going to use for this page Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-14 9:23 ` Paolo Bonzini
2017-03-13 12:44 ` [Qemu-devel] [PATCH 14/16] migration: Test new fd infrastructure Juan Quintela
` (3 subsequent siblings)
16 siblings, 1 reply; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
We make the locking and the transfer of information specific, even if we
are still receiving things through the main thread.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 60 insertions(+), 8 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 6f5ca50..3b1a2dc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -44,6 +44,7 @@
#include "exec/ram_addr.h"
#include "qemu/rcu_queue.h"
#include "migration/colo.h"
+#include "qemu/iov.h"
static int dirty_rate_high_cnt;
@@ -536,7 +537,7 @@ int migrate_multifd_send_threads_create(void)
return 0;
}
-static int multifd_send_page(uint8_t *address)
+static uint16_t multifd_send_page(uint8_t *address, bool last_page)
{
int i, j;
MultiFDSendParams *p = NULL; /* make happy gcc */
@@ -552,8 +553,10 @@ static int multifd_send_page(uint8_t *address)
pages.iov[pages.num].iov_len = TARGET_PAGE_SIZE;
pages.num++;
- if (pages.num < (pages.size - 1)) {
- return UINT16_MAX;
+ if (!last_page) {
+ if (pages.num < (pages.size - 1)) {
+ return UINT16_MAX;
+ }
}
qemu_sem_wait(&multifd_send_state->sem);
@@ -581,13 +584,18 @@ static int multifd_send_page(uint8_t *address)
}
struct MultiFDRecvParams {
+ /* not changed */
int id;
QemuThread thread;
QIOChannel *c;
QemuSemaphore init;
+ QemuSemaphore ready;
QemuSemaphore sem;
QemuMutex mutex;
+ /* proteced by param mutex */
bool quit;
+ multifd_pages_t pages;
+ bool done;
};
typedef struct MultiFDRecvParams MultiFDRecvParams;
@@ -641,6 +649,7 @@ static void *multifd_recv_thread(void *opaque)
qio_channel_read(p->c, &start, 1, &error_abort);
qemu_sem_post(&p->init);
+ qemu_sem_post(&p->ready);
while (true) {
qemu_mutex_lock(&p->mutex);
@@ -648,6 +657,13 @@ static void *multifd_recv_thread(void *opaque)
qemu_mutex_unlock(&p->mutex);
break;
}
+ if (p->pages.num) {
+ p->pages.num = 0;
+ p->done = true;
+ qemu_mutex_unlock(&p->mutex);
+ qemu_sem_post(&p->ready);
+ continue;
+ }
qemu_mutex_unlock(&p->mutex);
qemu_sem_wait(&p->sem);
}
@@ -672,8 +688,11 @@ int migrate_multifd_recv_threads_create(void)
qemu_mutex_init(&p->mutex);
qemu_sem_init(&p->sem, 0);
qemu_sem_init(&p->init, 0);
+ qemu_sem_init(&p->ready, 0);
p->quit = false;
p->id = i;
+ p->done = false;
+ multifd_init_group(&p->pages);
p->c = socket_recv_channel_create();
if (!p->c) {
@@ -690,6 +709,42 @@ int migrate_multifd_recv_threads_create(void)
return 0;
}
+static void multifd_recv_page(uint8_t *address, uint16_t fd_num)
+{
+ int thread_count;
+ MultiFDRecvParams *p;
+ static multifd_pages_t pages;
+ static bool once;
+
+ if (!once) {
+ multifd_init_group(&pages);
+ once = true;
+ }
+
+ pages.iov[pages.num].iov_base = address;
+ pages.iov[pages.num].iov_len = TARGET_PAGE_SIZE;
+ pages.num++;
+
+ if (fd_num == UINT16_MAX) {
+ return;
+ }
+
+ thread_count = migrate_multifd_threads();
+ assert(fd_num < thread_count);
+ p = &multifd_recv_state->params[fd_num];
+
+ qemu_sem_wait(&p->ready);
+
+ qemu_mutex_lock(&p->mutex);
+ p->done = false;
+ iov_copy(p->pages.iov, pages.num, pages.iov, pages.num, 0,
+ iov_size(pages.iov, pages.num));
+ p->pages.num = pages.num;
+ pages.num = 0;
+ qemu_mutex_unlock(&p->mutex);
+ qemu_sem_post(&p->sem);
+}
+
/**
* save_page_header: Write page header to wire
*
@@ -1153,7 +1208,7 @@ static int ram_multifd_page(QEMUFile *f, PageSearchStatus *pss,
if (pages == -1) {
*bytes_transferred +=
save_page_header(f, block, offset | RAM_SAVE_FLAG_MULTIFD_PAGE);
- fd_num = multifd_send_page(p);
+ fd_num = multifd_send_page(p, migration_dirty_pages == 1);
qemu_put_be16(f, fd_num);
*bytes_transferred += 2; /* size of fd_num */
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
@@ -3012,10 +3067,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
case RAM_SAVE_FLAG_MULTIFD_PAGE:
fd_num = qemu_get_be16(f);
- if (fd_num != 0) {
- /* this is yet an unused variable, changed later */
- fd_num = fd_num;
- }
+ multifd_recv_page(host, fd_num);
qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
break;
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-13 12:44 ` [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side Juan Quintela
@ 2017-03-14 9:23 ` Paolo Bonzini
2017-03-17 13:02 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2017-03-14 9:23 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: amit.shah, dgilbert
On 13/03/2017 13:44, Juan Quintela wrote:
> case RAM_SAVE_FLAG_MULTIFD_PAGE:
> fd_num = qemu_get_be16(f);
> - if (fd_num != 0) {
> - /* this is yet an unused variable, changed later */
> - fd_num = fd_num;
> - }
> + multifd_recv_page(host, fd_num);
> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> break;
I still believe this design is a mistake.
Paolo
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-14 9:23 ` Paolo Bonzini
@ 2017-03-17 13:02 ` Dr. David Alan Gilbert
2017-03-17 16:05 ` Paolo Bonzini
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-17 13:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Juan Quintela, qemu-devel
* Paolo Bonzini (pbonzini@redhat.com) wrote:
>
>
> On 13/03/2017 13:44, Juan Quintela wrote:
> > case RAM_SAVE_FLAG_MULTIFD_PAGE:
> > fd_num = qemu_get_be16(f);
> > - if (fd_num != 0) {
> > - /* this is yet an unused variable, changed later */
> > - fd_num = fd_num;
> > - }
> > + multifd_recv_page(host, fd_num);
> > qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > break;
>
> I still believe this design is a mistake.
Is it a use of a separate FD carrying all of the flags/addresses that
you object to?
Dave
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-17 13:02 ` Dr. David Alan Gilbert
@ 2017-03-17 16:05 ` Paolo Bonzini
2017-03-17 19:36 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2017-03-17 16:05 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, qemu-devel
On 17/03/2017 14:02, Dr. David Alan Gilbert wrote:
>>> case RAM_SAVE_FLAG_MULTIFD_PAGE:
>>> fd_num = qemu_get_be16(f);
>>> - if (fd_num != 0) {
>>> - /* this is yet an unused variable, changed later */
>>> - fd_num = fd_num;
>>> - }
>>> + multifd_recv_page(host, fd_num);
>>> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>>> break;
>> I still believe this design is a mistake.
> Is it a use of a separate FD carrying all of the flags/addresses that
> you object to?
Yes, it introduces a serialization point unnecessarily, and I don't
believe the rationale that Juan offered was strong enough.
This is certainly true on the receive side, but serialization is not
even necessary on the send side. Multiple threads can efficiently split
the work among themselves and visit the dirty bitmap without a central
distributor.
I need to study the code more to understand another issue. Say you have
a page that is sent to two different threads in two different
iterations, like
thread 1
iteration 1: pages 3, 7
thread 2
iteration 1: page 3
iteration 2: page 7
Does the code ensure that all threads wait at the end of an iteration?
Otherwise, thread 2 could process page 7 from iteration 2 before or
while thread 1 processes the same page from iteration 1.
Paolo
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-17 16:05 ` Paolo Bonzini
@ 2017-03-17 19:36 ` Dr. David Alan Gilbert
2017-03-20 11:15 ` Paolo Bonzini
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-17 19:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Juan Quintela, qemu-devel
* Paolo Bonzini (pbonzini@redhat.com) wrote:
>
>
> On 17/03/2017 14:02, Dr. David Alan Gilbert wrote:
> >>> case RAM_SAVE_FLAG_MULTIFD_PAGE:
> >>> fd_num = qemu_get_be16(f);
> >>> - if (fd_num != 0) {
> >>> - /* this is yet an unused variable, changed later */
> >>> - fd_num = fd_num;
> >>> - }
> >>> + multifd_recv_page(host, fd_num);
> >>> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> >>> break;
> >> I still believe this design is a mistake.
> > Is it a use of a separate FD carrying all of the flags/addresses that
> > you object to?
>
> Yes, it introduces a serialization point unnecessarily, and I don't
> believe the rationale that Juan offered was strong enough.
>
> This is certainly true on the receive side, but serialization is not
> even necessary on the send side.
Is there an easy way to benchmark it (without writing both) to figure
out if sending (word) (page) on one fd is less efficient than sending
two fd's with the pages and words separate?
> Multiple threads can efficiently split
> the work among themselves and visit the dirty bitmap without a central
> distributor.
I mostly agree; I kind of fancy the idea of having one per NUMA node;
but a central distributor might be a good idea anyway in the cases
where you find the heavy-writer all happens to be in the same area.
>
> I need to study the code more to understand another issue. Say you have
> a page that is sent to two different threads in two different
> iterations, like
>
> thread 1
> iteration 1: pages 3, 7
> thread 2
> iteration 1: page 3
> iteration 2: page 7
>
> Does the code ensure that all threads wait at the end of an iteration?
> Otherwise, thread 2 could process page 7 from iteration 2 before or
> while thread 1 processes the same page from iteration 1.
I think there's a sync at the end of each iteration on Juan's current code
that stops that.
Dave
>
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-17 19:36 ` Dr. David Alan Gilbert
@ 2017-03-20 11:15 ` Paolo Bonzini
2017-03-30 11:56 ` Juan Quintela
0 siblings, 1 reply; 46+ messages in thread
From: Paolo Bonzini @ 2017-03-20 11:15 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, qemu-devel
On 17/03/2017 20:36, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 17/03/2017 14:02, Dr. David Alan Gilbert wrote:
>>>>> case RAM_SAVE_FLAG_MULTIFD_PAGE:
>>>>> fd_num = qemu_get_be16(f);
>>>>> - if (fd_num != 0) {
>>>>> - /* this is yet an unused variable, changed later */
>>>>> - fd_num = fd_num;
>>>>> - }
>>>>> + multifd_recv_page(host, fd_num);
>>>>> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>>>>> break;
>>>> I still believe this design is a mistake.
>>> Is it a use of a separate FD carrying all of the flags/addresses that
>>> you object to?
>>
>> Yes, it introduces a serialization point unnecessarily, and I don't
>> believe the rationale that Juan offered was strong enough.
>>
>> This is certainly true on the receive side, but serialization is not
>> even necessary on the send side.
>
> Is there an easy way to benchmark it (without writing both) to figure
> out if sending (word) (page) on one fd is less efficient than sending
> two fd's with the pages and words separate?
I think it shouldn't be hard to write a version which keeps the central
distributor but puts the metadata in the auxiliary fds too.
But I think what matters is not efficiency, but rather being more
forward-proof. Besides liberty of changing implementation, Juan's
current code simply has no commands in auxiliary file descriptors, which
can be very limiting.
Paolo
>> Multiple threads can efficiently split
>> the work among themselves and visit the dirty bitmap without a central
>> distributor.
>
> I mostly agree; I kind of fancy the idea of having one per NUMA node;
> but a central distributor might be a good idea anyway in the cases
> where you find the heavy-writer all happens to be in the same area.
>
>>
>> I need to study the code more to understand another issue. Say you have
>> a page that is sent to two different threads in two different
>> iterations, like
>>
>> thread 1
>> iteration 1: pages 3, 7
>> thread 2
>> iteration 1: page 3
>> iteration 2: page 7
>>
>> Does the code ensure that all threads wait at the end of an iteration?
>> Otherwise, thread 2 could process page 7 from iteration 2 before or
>> while thread 1 processes the same page from iteration 1.
>
> I think there's a sync at the end of each iteration on Juan's current code
> that stops that.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side
2017-03-20 11:15 ` Paolo Bonzini
@ 2017-03-30 11:56 ` Juan Quintela
0 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-30 11:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Dr. David Alan Gilbert, qemu-devel
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 17/03/2017 20:36, Dr. David Alan Gilbert wrote:
>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>> On 17/03/2017 14:02, Dr. David Alan Gilbert wrote:
>>>>>> case RAM_SAVE_FLAG_MULTIFD_PAGE:
>>>>>> fd_num = qemu_get_be16(f);
>>>>>> - if (fd_num != 0) {
>>>>>> - /* this is yet an unused variable, changed later */
>>>>>> - fd_num = fd_num;
>>>>>> - }
>>>>>> + multifd_recv_page(host, fd_num);
>>>>>> qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>>>>>> break;
>>>>> I still believe this design is a mistake.
>>>> Is it a use of a separate FD carrying all of the flags/addresses that
>>>> you object to?
>>>
>>> Yes, it introduces a serialization point unnecessarily, and I don't
>>> believe the rationale that Juan offered was strong enough.
>>>
>>> This is certainly true on the receive side, but serialization is not
>>> even necessary on the send side.
>>
>> Is there an easy way to benchmark it (without writing both) to figure
>> out if sending (word) (page) on one fd is less efficient than sending
>> two fd's with the pages and words separate?
>
> I think it shouldn't be hard to write a version which keeps the central
> distributor but puts the metadata in the auxiliary fds too.
That is not difficult to do (famous last words).
I will try to test both approachs for next version, thanks.
>
> But I think what matters is not efficiency, but rather being more
> forward-proof. Besides liberty of changing implementation, Juan's
> current code simply has no commands in auxiliary file descriptors, which
> can be very limiting.
>
> Paolo
>
>>> Multiple threads can efficiently split
>>> the work among themselves and visit the dirty bitmap without a central
>>> distributor.
>>
>> I mostly agree; I kind of fancy the idea of having one per NUMA node;
>> but a central distributor might be a good idea anyway in the cases
>> where you find the heavy-writer all happens to be in the same area.
>>
>>>
>>> I need to study the code more to understand another issue. Say you have
>>> a page that is sent to two different threads in two different
>>> iterations, like
>>>
>>> thread 1
>>> iteration 1: pages 3, 7
>>> thread 2
>>> iteration 1: page 3
>>> iteration 2: page 7
>>>
>>> Does the code ensure that all threads wait at the end of an iteration?
>>> Otherwise, thread 2 could process page 7 from iteration 2 before or
>>> while thread 1 processes the same page from iteration 1.
>>
>> I think there's a sync at the end of each iteration on Juan's current code
>> that stops that.
This can't happen by design. We sync all threads at the end of each migration.
Later, Juan.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH 14/16] migration: Test new fd infrastructure
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (12 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 13/16] migration: Create thread infrastructure for multifd recv side Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 15/16] migration: Transfer pages over new channels Juan Quintela
` (2 subsequent siblings)
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
We just send the address through the alternate channels and test that it
is ok.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/migration/ram.c b/migration/ram.c
index 3b1a2dc..32cc678 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -476,8 +476,26 @@ static void *multifd_send_thread(void *opaque)
break;
}
if (p->pages.num) {
+ int i;
+ int num;
+
+ num = p->pages.num;
p->pages.num = 0;
qemu_mutex_unlock(&p->mutex);
+
+ for (i = 0; i < num; i++) {
+ if (qio_channel_write(p->c,
+ (const char *)&p->pages.iov[i].iov_base,
+ sizeof(uint8_t *), &error_abort)
+ != sizeof(uint8_t *)) {
+ MigrationState *s = migrate_get_current();
+
+ migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+ terminate_multifd_send_threads();
+ return NULL;
+ }
+ }
qemu_mutex_lock(&multifd_send_state->mutex);
p->done = true;
qemu_mutex_unlock(&multifd_send_state->mutex);
@@ -645,6 +663,7 @@ void migrate_multifd_recv_threads_join(void)
static void *multifd_recv_thread(void *opaque)
{
MultiFDRecvParams *p = opaque;
+ uint8_t *recv_address;
char start;
qio_channel_read(p->c, &start, 1, &error_abort);
@@ -658,7 +677,38 @@ static void *multifd_recv_thread(void *opaque)
break;
}
if (p->pages.num) {
+ int i;
+ int num;
+
+ num = p->pages.num;
p->pages.num = 0;
+
+ for (i = 0; i < num; i++) {
+ if (qio_channel_read(p->c,
+ (char *)&recv_address,
+ sizeof(uint8_t *), &error_abort)
+ != sizeof(uint8_t *)) {
+ MigrationState *s = migrate_get_current();
+
+ migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+ terminate_multifd_recv_threads();
+ return NULL;
+ }
+ if (recv_address != p->pages.iov[i].iov_base) {
+ MigrationState *s = migrate_get_current();
+
+ printf("We received %p what we were expecting %p (%d)\n",
+ recv_address,
+ p->pages.iov[i].iov_base, i);
+
+ migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+ terminate_multifd_recv_threads();
+ return NULL;
+ }
+ }
+
p->done = true;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->ready);
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 15/16] migration: Transfer pages over new channels
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (13 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 14/16] migration: Test new fd infrastructure Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-13 12:44 ` [Qemu-devel] [PATCH 16/16] migration: Flush receive queue Juan Quintela
2017-03-14 10:21 ` [Qemu-devel] [PATCH 00/16] Multifd v4 Dr. David Alan Gilbert
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
We switch for sending the page number to send real pages.
Signed-off-by: Juan Quintela <quintela@redhat.com>
--
Remove the HACK bit, now we have the function that calculates the size
of a page exported.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/migration.c | 14 ++++++++----
migration/ram.c | 59 +++++++++++++++++----------------------------------
2 files changed, 29 insertions(+), 44 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index a32e4ad..18d6955 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1967,7 +1967,8 @@ static void *migration_thread(void *opaque)
/* Used by the bandwidth calcs, updated later */
int64_t initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
- int64_t initial_bytes = 0;
+ int64_t qemu_file_bytes = 0;
+ int64_t multifd_pages = 0;
int64_t max_size = 0;
int64_t start_time = initial_time;
int64_t end_time;
@@ -2051,9 +2052,13 @@ static void *migration_thread(void *opaque)
}
current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
if (current_time >= initial_time + BUFFER_DELAY) {
- uint64_t transferred_bytes = qemu_ftell(s->to_dst_file) -
- initial_bytes;
uint64_t time_spent = current_time - initial_time;
+ uint64_t qemu_file_bytes_now = qemu_ftell(s->to_dst_file);
+ uint64_t multifd_pages_now = multifd_mig_pages_transferred();
+ uint64_t transferred_bytes =
+ (qemu_file_bytes_now - qemu_file_bytes) +
+ (multifd_pages_now - multifd_pages) *
+ (1ul << qemu_target_page_bits());
double bandwidth = (double)transferred_bytes / time_spent;
max_size = bandwidth * s->parameters.downtime_limit;
@@ -2070,7 +2075,8 @@ static void *migration_thread(void *opaque)
qemu_file_reset_rate_limit(s->to_dst_file);
initial_time = current_time;
- initial_bytes = qemu_ftell(s->to_dst_file);
+ qemu_file_bytes = qemu_file_bytes_now;
+ multifd_pages = multifd_pages_now;
}
if (qemu_file_rate_limit(s->to_dst_file)) {
/* usleep expects microseconds */
diff --git a/migration/ram.c b/migration/ram.c
index 32cc678..e213a49 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -476,25 +476,21 @@ static void *multifd_send_thread(void *opaque)
break;
}
if (p->pages.num) {
- int i;
int num;
num = p->pages.num;
p->pages.num = 0;
qemu_mutex_unlock(&p->mutex);
- for (i = 0; i < num; i++) {
- if (qio_channel_write(p->c,
- (const char *)&p->pages.iov[i].iov_base,
- sizeof(uint8_t *), &error_abort)
- != sizeof(uint8_t *)) {
- MigrationState *s = migrate_get_current();
+ if (qio_channel_writev_all(p->c, p->pages.iov,
+ num, &error_abort)
+ != num * TARGET_PAGE_SIZE) {
+ MigrationState *s = migrate_get_current();
- migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
- MIGRATION_STATUS_FAILED);
- terminate_multifd_send_threads();
- return NULL;
- }
+ migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+ terminate_multifd_send_threads();
+ return NULL;
}
qemu_mutex_lock(&multifd_send_state->mutex);
p->done = true;
@@ -663,7 +659,6 @@ void migrate_multifd_recv_threads_join(void)
static void *multifd_recv_thread(void *opaque)
{
MultiFDRecvParams *p = opaque;
- uint8_t *recv_address;
char start;
qio_channel_read(p->c, &start, 1, &error_abort);
@@ -677,38 +672,21 @@ static void *multifd_recv_thread(void *opaque)
break;
}
if (p->pages.num) {
- int i;
int num;
num = p->pages.num;
p->pages.num = 0;
- for (i = 0; i < num; i++) {
- if (qio_channel_read(p->c,
- (char *)&recv_address,
- sizeof(uint8_t *), &error_abort)
- != sizeof(uint8_t *)) {
- MigrationState *s = migrate_get_current();
+ if (qio_channel_readv_all(p->c, p->pages.iov,
+ num, &error_abort)
+ != num * TARGET_PAGE_SIZE) {
+ MigrationState *s = migrate_get_current();
- migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
- MIGRATION_STATUS_FAILED);
- terminate_multifd_recv_threads();
- return NULL;
- }
- if (recv_address != p->pages.iov[i].iov_base) {
- MigrationState *s = migrate_get_current();
-
- printf("We received %p what we were expecting %p (%d)\n",
- recv_address,
- p->pages.iov[i].iov_base, i);
-
- migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
- MIGRATION_STATUS_FAILED);
- terminate_multifd_recv_threads();
- return NULL;
- }
+ migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_FAILED);
+ terminate_multifd_recv_threads();
+ return NULL;
}
-
p->done = true;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->ready);
@@ -1260,8 +1238,10 @@ static int ram_multifd_page(QEMUFile *f, PageSearchStatus *pss,
save_page_header(f, block, offset | RAM_SAVE_FLAG_MULTIFD_PAGE);
fd_num = multifd_send_page(p, migration_dirty_pages == 1);
qemu_put_be16(f, fd_num);
+ if (fd_num != UINT16_MAX) {
+ qemu_fflush(f);
+ }
*bytes_transferred += 2; /* size of fd_num */
- qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
*bytes_transferred += TARGET_PAGE_SIZE;
pages = 1;
acct_info.norm_pages++;
@@ -3118,7 +3098,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
case RAM_SAVE_FLAG_MULTIFD_PAGE:
fd_num = qemu_get_be16(f);
multifd_recv_page(host, fd_num);
- qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
break;
case RAM_SAVE_FLAG_EOS:
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH 16/16] migration: Flush receive queue
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (14 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 15/16] migration: Transfer pages over new channels Juan Quintela
@ 2017-03-13 12:44 ` Juan Quintela
2017-03-14 10:21 ` [Qemu-devel] [PATCH 00/16] Multifd v4 Dr. David Alan Gilbert
16 siblings, 0 replies; 46+ messages in thread
From: Juan Quintela @ 2017-03-13 12:44 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, dgilbert
Each time that we sync the bitmap, it is a possiblity that we receive
a page that is being processed by a different thread. We fix this
problem just making sure that we wait for all receiving threads to
finish its work before we procedeed with the next stage.
We are low on page flags, so we use a combination that is not valid to
emit that message: MULTIFD_PAGE and COMPRESSED.
I tried to make a migration command for it, but it don't work because
we sync the bitmap sometimes when we have already sent the beggining
of the section, so I just added a new page flag.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
include/migration/migration.h | 1 +
migration/ram.c | 57 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bd152c5..86023ff 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -278,6 +278,7 @@ int migrate_multifd_send_threads_create(void);
void migrate_multifd_send_threads_join(void);
int migrate_multifd_recv_threads_create(void);
void migrate_multifd_recv_threads_join(void);
+void qemu_savevm_send_multifd_flush(QEMUFile *f);
void migrate_compress_threads_create(void);
void migrate_compress_threads_join(void);
diff --git a/migration/ram.c b/migration/ram.c
index e213a49..a93c4ae 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -64,6 +64,13 @@ static uint64_t bitmap_sync_count;
#define RAM_SAVE_FLAG_COMPRESS_PAGE 0x100
#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
+/* We are getting low on pages flags, so we start using combinations
+ When we need to flush a page, we sent it as
+ RAM_SAVE_FLAG_MULTIFD_PAGE | RAM_SAVE_FLAG_COMPRESS_PAGE
+ We don't allow that combination
+*/
+
+
static uint8_t *ZERO_TARGET_PAGE;
static inline bool is_zero_range(uint8_t *p, uint64_t size)
@@ -392,6 +399,9 @@ void migrate_compress_threads_create(void)
/* Multiple fd's */
+/* Indicates if we have synced the bitmap and we need to assure that
+ target has processeed all previous pages */
+bool multifd_needs_flush;
typedef struct {
int num;
@@ -605,9 +615,11 @@ struct MultiFDRecvParams {
QemuSemaphore init;
QemuSemaphore ready;
QemuSemaphore sem;
+ QemuCond cond_sync;
QemuMutex mutex;
/* proteced by param mutex */
bool quit;
+ bool sync;
multifd_pages_t pages;
bool done;
};
@@ -648,6 +660,7 @@ void migrate_multifd_recv_threads_join(void)
qemu_mutex_destroy(&p->mutex);
qemu_sem_destroy(&p->sem);
qemu_sem_destroy(&p->init);
+ qemu_cond_destroy(&p->cond_sync);
socket_send_channel_destroy(multifd_recv_state->params[i].c);
}
g_free(multifd_recv_state->params);
@@ -688,6 +701,10 @@ static void *multifd_recv_thread(void *opaque)
return NULL;
}
p->done = true;
+ if (p->sync) {
+ qemu_cond_signal(&p->cond_sync);
+ p->sync = false;
+ }
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->ready);
continue;
@@ -717,9 +734,11 @@ int migrate_multifd_recv_threads_create(void)
qemu_sem_init(&p->sem, 0);
qemu_sem_init(&p->init, 0);
qemu_sem_init(&p->ready, 0);
+ qemu_cond_init(&p->cond_sync);
p->quit = false;
p->id = i;
p->done = false;
+ p->sync = false;
multifd_init_group(&p->pages);
p->c = socket_recv_channel_create();
@@ -773,6 +792,27 @@ static void multifd_recv_page(uint8_t *address, uint16_t fd_num)
qemu_sem_post(&p->sem);
}
+static int multifd_flush(void)
+{
+ int i, thread_count;
+
+ if (!migrate_use_multifd()) {
+ return 0;
+ }
+ thread_count = migrate_multifd_threads();
+ for (i = 0; i < thread_count; i++) {
+ MultiFDRecvParams *p = &multifd_recv_state->params[i];
+
+ qemu_mutex_lock(&p->mutex);
+ while (!p->done) {
+ p->sync = true;
+ qemu_cond_wait(&p->cond_sync, &p->mutex);
+ }
+ qemu_mutex_unlock(&p->mutex);
+ }
+ return 0;
+}
+
/**
* save_page_header: Write page header to wire
*
@@ -789,6 +829,12 @@ static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
{
size_t size, len;
+ if (multifd_needs_flush &&
+ (offset & RAM_SAVE_FLAG_MULTIFD_PAGE)) {
+ offset |= RAM_SAVE_FLAG_COMPRESS;
+ multifd_needs_flush = false;
+ }
+
qemu_put_be64(f, offset);
size = 8;
@@ -2526,6 +2572,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
if (!migration_in_postcopy(migrate_get_current())) {
migration_bitmap_sync();
+ if (migrate_use_multifd()) {
+ multifd_needs_flush = true;
+ }
}
ram_control_before_iterate(f, RAM_CONTROL_FINISH);
@@ -2567,6 +2616,9 @@ static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
qemu_mutex_lock_iothread();
rcu_read_lock();
migration_bitmap_sync();
+ if (migrate_use_multifd()) {
+ multifd_needs_flush = true;
+ }
rcu_read_unlock();
qemu_mutex_unlock_iothread();
remaining_size = ram_save_remaining() * TARGET_PAGE_SIZE;
@@ -3005,6 +3057,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
break;
}
+ if ((flags & (RAM_SAVE_FLAG_MULTIFD_PAGE | RAM_SAVE_FLAG_COMPRESS))
+ == (RAM_SAVE_FLAG_MULTIFD_PAGE | RAM_SAVE_FLAG_COMPRESS)) {
+ multifd_flush();
+ flags = flags & ~RAM_SAVE_FLAG_COMPRESS;
+ }
if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE |
RAM_SAVE_FLAG_MULTIFD_PAGE)) {
--
2.9.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-13 12:44 [Qemu-devel] [PATCH 00/16] Multifd v4 Juan Quintela
` (15 preceding siblings ...)
2017-03-13 12:44 ` [Qemu-devel] [PATCH 16/16] migration: Flush receive queue Juan Quintela
@ 2017-03-14 10:21 ` Dr. David Alan Gilbert
2017-03-14 10:26 ` Daniel P. Berrange
2017-03-14 11:47 ` Daniel P. Berrange
16 siblings, 2 replies; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 10:21 UTC (permalink / raw)
To: Juan Quintela; +Cc: qemu-devel
* Juan Quintela (quintela@redhat.com) wrote:
> Hi
>
> This is the 4th version of multifd. Changes:
> - XBZRLE don't need to be checked for
> - Documentation and defaults are consistent
> - split socketArgs
> - use iovec instead of creating something similar.
> - We use now the exported size of target page (another HACK removal)
> - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> was already taken.
> What they do is the same that the without _all() function, but if it
> returns due to blocking it redo the call.
> - it is checkpatch.pl clean now.
>
> Please comment, Juan.
High level things,
a) I think you probably need to do some bandwidth measurements to show
that multifd is managing to have some benefit - it would be good
for the cover letter.
b) By my count I think this is actually v5 (And I think you're missing
the -v to git)
Dave
>
>
>
> [v3]
>
> - comments for previous verion addressed
> - lots of bugs fixed
> - remove DPRINTF from ram.c
>
> - add multifd-group parameter, it gives how many pages we sent each
> time to the worker threads. I am open to better names.
> - Better flush support.
> - with migration_set_speed 2G it is able to migrate "stress -vm 2
> -vm-bytes 512M" over loopback.
>
> Please review.
>
> Thanks, Juan.
>
> [v2]
>
> This is a version against current code. It is based on top of QIO
> work. It improves the thread synchronization and fixes the problem
> when we could have two threads handing the same page.
>
> Please comment, Juan.
>
>
> Juan Quintela (16):
> qio: create new qio_channel_write_all
> qio: create new qio_channel_read_all
> migration: Test for disabled features on reception
> migration: Don't create decompression threads if not enabled
> migration: Add multifd capability
> migration: Create x-multifd-threads parameter
> migration: Create x-multifd-group parameter
> migration: Create multifd migration threads
> migration: Start of multiple fd work
> migration: Create ram_multifd_page
> migration: Really use multiple pages at a time
> migration: Send the fd number which we are going to use for this page
> migration: Create thread infrastructure for multifd recv side
> migration: Test new fd infrastructure
> migration: Transfer pages over new channels
> migration: Flush receive queue
>
> hmp.c | 18 ++
> include/io/channel.h | 46 ++++
> include/migration/migration.h | 17 ++
> io/channel.c | 76 ++++++
> migration/migration.c | 85 ++++++-
> migration/qemu-file-channel.c | 29 +--
> migration/ram.c | 522 +++++++++++++++++++++++++++++++++++++++++-
> migration/socket.c | 67 +++++-
> qapi-schema.json | 30 ++-
> 9 files changed, 848 insertions(+), 42 deletions(-)
>
> --
> 2.9.3
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 10:21 ` [Qemu-devel] [PATCH 00/16] Multifd v4 Dr. David Alan Gilbert
@ 2017-03-14 10:26 ` Daniel P. Berrange
2017-03-14 11:40 ` Dr. David Alan Gilbert
2017-03-14 11:47 ` Daniel P. Berrange
1 sibling, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-14 10:26 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, qemu-devel
On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > Hi
> >
> > This is the 4th version of multifd. Changes:
> > - XBZRLE don't need to be checked for
> > - Documentation and defaults are consistent
> > - split socketArgs
> > - use iovec instead of creating something similar.
> > - We use now the exported size of target page (another HACK removal)
> > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > was already taken.
> > What they do is the same that the without _all() function, but if it
> > returns due to blocking it redo the call.
> > - it is checkpatch.pl clean now.
> >
> > Please comment, Juan.
>
> High level things,
> a) I think you probably need to do some bandwidth measurements to show
> that multifd is managing to have some benefit - it would be good
> for the cover letter.
multi-fd will certainly benefit encrypted migration, since we'll be able
to burn multiple CPUs for AES instead of bottlenecking on one CPU, and
thus able to take greater advantage of networks with > 1-GigE bandwidth.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 10:26 ` Daniel P. Berrange
@ 2017-03-14 11:40 ` Dr. David Alan Gilbert
2017-03-14 11:45 ` Daniel P. Berrange
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 11:40 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Juan Quintela, qemu-devel
* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> > > Hi
> > >
> > > This is the 4th version of multifd. Changes:
> > > - XBZRLE don't need to be checked for
> > > - Documentation and defaults are consistent
> > > - split socketArgs
> > > - use iovec instead of creating something similar.
> > > - We use now the exported size of target page (another HACK removal)
> > > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > > was already taken.
> > > What they do is the same that the without _all() function, but if it
> > > returns due to blocking it redo the call.
> > > - it is checkpatch.pl clean now.
> > >
> > > Please comment, Juan.
> >
> > High level things,
> > a) I think you probably need to do some bandwidth measurements to show
> > that multifd is managing to have some benefit - it would be good
> > for the cover letter.
>
> multi-fd will certainly benefit encrypted migration, since we'll be able
> to burn multiple CPUs for AES instead of bottlenecking on one CPU, and
> thus able to take greater advantage of networks with > 1-GigE bandwidth.
Yes, that's one I really want to see. It might be odd using lots of fd's
just to do that, but probably the easiest way.
Dave
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 11:40 ` Dr. David Alan Gilbert
@ 2017-03-14 11:45 ` Daniel P. Berrange
0 siblings, 0 replies; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-14 11:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, qemu-devel
On Tue, Mar 14, 2017 at 11:40:20AM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> > > * Juan Quintela (quintela@redhat.com) wrote:
> > > > Hi
> > > >
> > > > This is the 4th version of multifd. Changes:
> > > > - XBZRLE don't need to be checked for
> > > > - Documentation and defaults are consistent
> > > > - split socketArgs
> > > > - use iovec instead of creating something similar.
> > > > - We use now the exported size of target page (another HACK removal)
> > > > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > > > was already taken.
> > > > What they do is the same that the without _all() function, but if it
> > > > returns due to blocking it redo the call.
> > > > - it is checkpatch.pl clean now.
> > > >
> > > > Please comment, Juan.
> > >
> > > High level things,
> > > a) I think you probably need to do some bandwidth measurements to show
> > > that multifd is managing to have some benefit - it would be good
> > > for the cover letter.
> >
> > multi-fd will certainly benefit encrypted migration, since we'll be able
> > to burn multiple CPUs for AES instead of bottlenecking on one CPU, and
> > thus able to take greater advantage of networks with > 1-GigE bandwidth.
>
> Yes, that's one I really want to see. It might be odd using lots of fd's
> just to do that, but probably the easiest way.
In theory you could have multiple threads doing encryption, all writing to
a single FD, but the AFAICT the GNUTLS library doesn't make this possible
as it encapsulates both encryption + i/o behind a single API call. Even if
we called the same API call from multiple threads, I'm pretty sure it will
serialize the encryption with internal locking, so you would not gain
anything. So using multiple distinct TLS connections is the only viable
option.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 10:21 ` [Qemu-devel] [PATCH 00/16] Multifd v4 Dr. David Alan Gilbert
2017-03-14 10:26 ` Daniel P. Berrange
@ 2017-03-14 11:47 ` Daniel P. Berrange
2017-03-14 12:22 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-14 11:47 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, qemu-devel
On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > Hi
> >
> > This is the 4th version of multifd. Changes:
> > - XBZRLE don't need to be checked for
> > - Documentation and defaults are consistent
> > - split socketArgs
> > - use iovec instead of creating something similar.
> > - We use now the exported size of target page (another HACK removal)
> > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > was already taken.
> > What they do is the same that the without _all() function, but if it
> > returns due to blocking it redo the call.
> > - it is checkpatch.pl clean now.
> >
> > Please comment, Juan.
>
> High level things,
> a) I think you probably need to do some bandwidth measurements to show
> that multifd is managing to have some benefit - it would be good
> for the cover letter.
Presumably this would be a building block to solving the latency problems
with post-copy, by reserving one channel for use transferring out of band
pages required by target host page faults.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 11:47 ` Daniel P. Berrange
@ 2017-03-14 12:22 ` Dr. David Alan Gilbert
2017-03-14 12:34 ` Daniel P. Berrange
0 siblings, 1 reply; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 12:22 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Juan Quintela, qemu-devel
* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> > > Hi
> > >
> > > This is the 4th version of multifd. Changes:
> > > - XBZRLE don't need to be checked for
> > > - Documentation and defaults are consistent
> > > - split socketArgs
> > > - use iovec instead of creating something similar.
> > > - We use now the exported size of target page (another HACK removal)
> > > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > > was already taken.
> > > What they do is the same that the without _all() function, but if it
> > > returns due to blocking it redo the call.
> > > - it is checkpatch.pl clean now.
> > >
> > > Please comment, Juan.
> >
> > High level things,
> > a) I think you probably need to do some bandwidth measurements to show
> > that multifd is managing to have some benefit - it would be good
> > for the cover letter.
>
> Presumably this would be a building block to solving the latency problems
> with post-copy, by reserving one channel for use transferring out of band
> pages required by target host page faults.
Right, it's on my list to look at; there's some interesting questions about
the way in which the main fd carrying the headers interacts, and also what
happens to pages immediately after the requested page; for example, lets
say we're currently streaming at address 'S' and a postcopy request (P) comes in;
so what we currently have on one FD is:
S,S+1....S+n,P,P+1,P+2,P+n
Note that when a request comes in we flip location so we start sending background
pages from P+1 on the assumption that they'll be wanted soon.
with 3 FDs this would go initially as:
S S+3 P+1 P+4
S+1 S+4 P+2 ..
S+2 P P+3 ..
now if we had a spare FD for postcopy we'd do:
S S+3 P+1 P+4
S+1 S+4 P+2 ..
S+2 S+5 P+3 ..
- P - -
So 'P' got there quickly - but P+1 is stuck behind the S's; is that what we want?
An interesting alternative would be to switch which fd we keep free:
S S+3 - - -
S+1 S+4 P+2 P+4
S+2 S+5 P+3 P+5
- P P+1 P+6
So depending on your buffering P+1 might also now be pretty fast; but that's
starting to get into heuristics about guessing how much you should put on
your previously low-queue'd fd.
Dave
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 12:22 ` Dr. David Alan Gilbert
@ 2017-03-14 12:34 ` Daniel P. Berrange
2017-03-14 16:23 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 46+ messages in thread
From: Daniel P. Berrange @ 2017-03-14 12:34 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: Juan Quintela, qemu-devel
On Tue, Mar 14, 2017 at 12:22:23PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> > > * Juan Quintela (quintela@redhat.com) wrote:
> > > > Hi
> > > >
> > > > This is the 4th version of multifd. Changes:
> > > > - XBZRLE don't need to be checked for
> > > > - Documentation and defaults are consistent
> > > > - split socketArgs
> > > > - use iovec instead of creating something similar.
> > > > - We use now the exported size of target page (another HACK removal)
> > > > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > > > was already taken.
> > > > What they do is the same that the without _all() function, but if it
> > > > returns due to blocking it redo the call.
> > > > - it is checkpatch.pl clean now.
> > > >
> > > > Please comment, Juan.
> > >
> > > High level things,
> > > a) I think you probably need to do some bandwidth measurements to show
> > > that multifd is managing to have some benefit - it would be good
> > > for the cover letter.
> >
> > Presumably this would be a building block to solving the latency problems
> > with post-copy, by reserving one channel for use transferring out of band
> > pages required by target host page faults.
>
> Right, it's on my list to look at; there's some interesting questions about
> the way in which the main fd carrying the headers interacts, and also what
> happens to pages immediately after the requested page; for example, lets
> say we're currently streaming at address 'S' and a postcopy request (P) comes in;
> so what we currently have on one FD is:
>
> S,S+1....S+n,P,P+1,P+2,P+n
>
> Note that when a request comes in we flip location so we start sending background
> pages from P+1 on the assumption that they'll be wanted soon.
>
> with 3 FDs this would go initially as:
> S S+3 P+1 P+4
> S+1 S+4 P+2 ..
> S+2 P P+3 ..
>
> now if we had a spare FD for postcopy we'd do:
> S S+3 P+1 P+4
> S+1 S+4 P+2 ..
> S+2 S+5 P+3 ..
> - P - -
>
> So 'P' got there quickly - but P+1 is stuck behind the S's; is that what we want?
> An interesting alternative would be to switch which fd we keep free:
> S S+3 - - -
> S+1 S+4 P+2 P+4
> S+2 S+5 P+3 P+5
> - P P+1 P+6
>
> So depending on your buffering P+1 might also now be pretty fast; but that's
> starting to get into heuristics about guessing how much you should put on
> your previously low-queue'd fd.
Ah, I see, so you're essentially trying todo read-ahead when post-copy
faults. It becomes even more fun when you have multiple page faults
coming in, (quite likely with multi-vCPU guests), as you have P, Q, R, S
come in, all of which want servicing quickly. So if you queue up too
many P+n pages for read-ahead, you'd delay Q, R & S
S S+3 - - -
S+1 S+4 P+2 P+4 Q R ...
S+2 S+5 P+3 P+5 Q+1 R+1 ...
- P P+1 P+6 Q+2 ... ...
this tends to argue for overcommitting threads vs cpus. eg even if QEMU
is confined to only use 2 host CPUs, it would be worth having 4 migration
threads. They would contend for CPU time for AES encryption, but you
would reduce chance of getting stuck behind large send-buffers.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH 00/16] Multifd v4
2017-03-14 12:34 ` Daniel P. Berrange
@ 2017-03-14 16:23 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 46+ messages in thread
From: Dr. David Alan Gilbert @ 2017-03-14 16:23 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Juan Quintela, qemu-devel
* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Tue, Mar 14, 2017 at 12:22:23PM +0000, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > On Tue, Mar 14, 2017 at 10:21:43AM +0000, Dr. David Alan Gilbert wrote:
> > > > * Juan Quintela (quintela@redhat.com) wrote:
> > > > > Hi
> > > > >
> > > > > This is the 4th version of multifd. Changes:
> > > > > - XBZRLE don't need to be checked for
> > > > > - Documentation and defaults are consistent
> > > > > - split socketArgs
> > > > > - use iovec instead of creating something similar.
> > > > > - We use now the exported size of target page (another HACK removal)
> > > > > - created qio_chanel_{wirtev,readv}_all functions. the _full() name
> > > > > was already taken.
> > > > > What they do is the same that the without _all() function, but if it
> > > > > returns due to blocking it redo the call.
> > > > > - it is checkpatch.pl clean now.
> > > > >
> > > > > Please comment, Juan.
> > > >
> > > > High level things,
> > > > a) I think you probably need to do some bandwidth measurements to show
> > > > that multifd is managing to have some benefit - it would be good
> > > > for the cover letter.
> > >
> > > Presumably this would be a building block to solving the latency problems
> > > with post-copy, by reserving one channel for use transferring out of band
> > > pages required by target host page faults.
> >
> > Right, it's on my list to look at; there's some interesting questions about
> > the way in which the main fd carrying the headers interacts, and also what
> > happens to pages immediately after the requested page; for example, lets
> > say we're currently streaming at address 'S' and a postcopy request (P) comes in;
> > so what we currently have on one FD is:
> >
> > S,S+1....S+n,P,P+1,P+2,P+n
> >
> > Note that when a request comes in we flip location so we start sending background
> > pages from P+1 on the assumption that they'll be wanted soon.
> >
> > with 3 FDs this would go initially as:
> > S S+3 P+1 P+4
> > S+1 S+4 P+2 ..
> > S+2 P P+3 ..
> >
> > now if we had a spare FD for postcopy we'd do:
> > S S+3 P+1 P+4
> > S+1 S+4 P+2 ..
> > S+2 S+5 P+3 ..
> > - P - -
> >
> > So 'P' got there quickly - but P+1 is stuck behind the S's; is that what we want?
> > An interesting alternative would be to switch which fd we keep free:
> > S S+3 - - -
> > S+1 S+4 P+2 P+4
> > S+2 S+5 P+3 P+5
> > - P P+1 P+6
> >
> > So depending on your buffering P+1 might also now be pretty fast; but that's
> > starting to get into heuristics about guessing how much you should put on
> > your previously low-queue'd fd.
>
> Ah, I see, so you're essentially trying todo read-ahead when post-copy
> faults. It becomes even more fun when you have multiple page faults
> coming in, (quite likely with multi-vCPU guests), as you have P, Q, R, S
> come in, all of which want servicing quickly. So if you queue up too
> many P+n pages for read-ahead, you'd delay Q, R & S
>
> S S+3 - - -
> S+1 S+4 P+2 P+4 Q R ...
> S+2 S+5 P+3 P+5 Q+1 R+1 ...
> - P P+1 P+6 Q+2 ... ...
>
> this tends to argue for overcommitting threads vs cpus. eg even if QEMU
> is confined to only use 2 host CPUs, it would be worth having 4 migration
> threads. They would contend for CPU time for AES encryption, but you
> would reduce chance of getting stuck behind large send-buffers.
Possibly although it becomes very heuristicy; and then I'm not sure what
happens when you find you've got AES offload hardware.
I also worry again about the fd carrying the headers, if the destination
gets bottlenecked reading pages off the other fd's it might not get to the
postcopy page.
So you can bottleneck on any of network bandwidth, source CPU bandwidth
or destination CPU bandwidth (which I think is where the current bottleneck
on one fd tends to be with no encryption/compression).
I think there's a syscall where you can ask how much is buffered in a socket,
of course that can only tell you about the sender, so really you do want to be
setup so that the source is trying to send no faster than the destination can
read it.
Dave
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 46+ messages in thread