* [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv, Send}Params
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
@ 2022-08-02 6:38 ` Juan Quintela
2022-08-11 8:10 ` [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv,Send}Params Leonardo Brás
2022-08-02 6:38 ` [PATCH v7 02/12] multifd: Create page_count fields into both MultiFD{Recv, Send}Params Juan Quintela
` (10 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
We were calling qemu_target_page_size() left and right.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd.h | 4 ++++
migration/multifd-zlib.c | 14 ++++++--------
migration/multifd-zstd.c | 12 +++++-------
migration/multifd.c | 18 ++++++++----------
4 files changed, 23 insertions(+), 25 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index 519f498643..86fb9982b3 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -80,6 +80,8 @@ typedef struct {
bool registered_yank;
/* packet allocated len */
uint32_t packet_len;
+ /* guest page size */
+ uint32_t page_size;
/* multifd flags for sending ram */
int write_flags;
@@ -143,6 +145,8 @@ typedef struct {
QIOChannel *c;
/* packet allocated len */
uint32_t packet_len;
+ /* guest page size */
+ uint32_t page_size;
/* syncs main thread and channels */
QemuSemaphore sem_sync;
diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
index 18213a9513..37770248e1 100644
--- a/migration/multifd-zlib.c
+++ b/migration/multifd-zlib.c
@@ -116,7 +116,6 @@ static void zlib_send_cleanup(MultiFDSendParams *p, Error **errp)
static int zlib_send_prepare(MultiFDSendParams *p, Error **errp)
{
struct zlib_data *z = p->data;
- size_t page_size = qemu_target_page_size();
z_stream *zs = &z->zs;
uint32_t out_size = 0;
int ret;
@@ -135,8 +134,8 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error **errp)
* with compression. zlib does not guarantee that this is safe,
* therefore copy the page before calling deflate().
*/
- memcpy(z->buf, p->pages->block->host + p->normal[i], page_size);
- zs->avail_in = page_size;
+ memcpy(z->buf, p->pages->block->host + p->normal[i], p->page_size);
+ zs->avail_in = p->page_size;
zs->next_in = z->buf;
zs->avail_out = available;
@@ -242,12 +241,11 @@ static void zlib_recv_cleanup(MultiFDRecvParams *p)
static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
{
struct zlib_data *z = p->data;
- size_t page_size = qemu_target_page_size();
z_stream *zs = &z->zs;
uint32_t in_size = p->next_packet_size;
/* we measure the change of total_out */
uint32_t out_size = zs->total_out;
- uint32_t expected_size = p->normal_num * page_size;
+ uint32_t expected_size = p->normal_num * p->page_size;
uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
int ret;
int i;
@@ -274,7 +272,7 @@ static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
flush = Z_SYNC_FLUSH;
}
- zs->avail_out = page_size;
+ zs->avail_out = p->page_size;
zs->next_out = p->host + p->normal[i];
/*
@@ -288,8 +286,8 @@ static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
do {
ret = inflate(zs, flush);
} while (ret == Z_OK && zs->avail_in
- && (zs->total_out - start) < page_size);
- if (ret == Z_OK && (zs->total_out - start) < page_size) {
+ && (zs->total_out - start) < p->page_size);
+ if (ret == Z_OK && (zs->total_out - start) < p->page_size) {
error_setg(errp, "multifd %u: inflate generated too few output",
p->id);
return -1;
diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c
index d788d309f2..f4a8e1ed1f 100644
--- a/migration/multifd-zstd.c
+++ b/migration/multifd-zstd.c
@@ -113,7 +113,6 @@ static void zstd_send_cleanup(MultiFDSendParams *p, Error **errp)
static int zstd_send_prepare(MultiFDSendParams *p, Error **errp)
{
struct zstd_data *z = p->data;
- size_t page_size = qemu_target_page_size();
int ret;
uint32_t i;
@@ -128,7 +127,7 @@ static int zstd_send_prepare(MultiFDSendParams *p, Error **errp)
flush = ZSTD_e_flush;
}
z->in.src = p->pages->block->host + p->normal[i];
- z->in.size = page_size;
+ z->in.size = p->page_size;
z->in.pos = 0;
/*
@@ -241,8 +240,7 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
{
uint32_t in_size = p->next_packet_size;
uint32_t out_size = 0;
- size_t page_size = qemu_target_page_size();
- uint32_t expected_size = p->normal_num * page_size;
+ uint32_t expected_size = p->normal_num * p->page_size;
uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
struct zstd_data *z = p->data;
int ret;
@@ -265,7 +263,7 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
for (i = 0; i < p->normal_num; i++) {
z->out.dst = p->host + p->normal[i];
- z->out.size = page_size;
+ z->out.size = p->page_size;
z->out.pos = 0;
/*
@@ -279,8 +277,8 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
do {
ret = ZSTD_decompressStream(z->zds, &z->out, &z->in);
} while (ret > 0 && (z->in.size - z->in.pos > 0)
- && (z->out.pos < page_size));
- if (ret > 0 && (z->out.pos < page_size)) {
+ && (z->out.pos < p->page_size));
+ if (ret > 0 && (z->out.pos < p->page_size)) {
error_setg(errp, "multifd %u: decompressStream buffer too small",
p->id);
return -1;
diff --git a/migration/multifd.c b/migration/multifd.c
index 586ddc9d65..d2070c9cee 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -87,15 +87,14 @@ static void nocomp_send_cleanup(MultiFDSendParams *p, Error **errp)
static int nocomp_send_prepare(MultiFDSendParams *p, Error **errp)
{
MultiFDPages_t *pages = p->pages;
- size_t page_size = qemu_target_page_size();
for (int i = 0; i < p->normal_num; i++) {
p->iov[p->iovs_num].iov_base = pages->block->host + p->normal[i];
- p->iov[p->iovs_num].iov_len = page_size;
+ p->iov[p->iovs_num].iov_len = p->page_size;
p->iovs_num++;
}
- p->next_packet_size = p->normal_num * page_size;
+ p->next_packet_size = p->normal_num * p->page_size;
p->flags |= MULTIFD_FLAG_NOCOMP;
return 0;
}
@@ -139,7 +138,6 @@ static void nocomp_recv_cleanup(MultiFDRecvParams *p)
static int nocomp_recv_pages(MultiFDRecvParams *p, Error **errp)
{
uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
- size_t page_size = qemu_target_page_size();
if (flags != MULTIFD_FLAG_NOCOMP) {
error_setg(errp, "multifd %u: flags received %x flags expected %x",
@@ -148,7 +146,7 @@ static int nocomp_recv_pages(MultiFDRecvParams *p, Error **errp)
}
for (int i = 0; i < p->normal_num; i++) {
p->iov[i].iov_base = p->host + p->normal[i];
- p->iov[i].iov_len = page_size;
+ p->iov[i].iov_len = p->page_size;
}
return qio_channel_readv_all(p->c, p->iov, p->normal_num, errp);
}
@@ -281,8 +279,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
{
MultiFDPacket_t *packet = p->packet;
- size_t page_size = qemu_target_page_size();
- uint32_t page_count = MULTIFD_PACKET_SIZE / page_size;
+ uint32_t page_count = MULTIFD_PACKET_SIZE / p->page_size;
RAMBlock *block;
int i;
@@ -344,7 +341,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
for (i = 0; i < p->normal_num; i++) {
uint64_t offset = be64_to_cpu(packet->offset[i]);
- if (offset > (block->used_length - page_size)) {
+ if (offset > (block->used_length - p->page_size)) {
error_setg(errp, "multifd: offset too long %" PRIu64
" (max " RAM_ADDR_FMT ")",
offset, block->used_length);
@@ -433,8 +430,7 @@ static int multifd_send_pages(QEMUFile *f)
p->packet_num = multifd_send_state->packet_num++;
multifd_send_state->pages = p->pages;
p->pages = pages;
- transferred = ((uint64_t) pages->num) * qemu_target_page_size()
- + p->packet_len;
+ transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
qemu_file_acct_rate_limit(f, transferred);
ram_counters.multifd_bytes += transferred;
ram_counters.transferred += transferred;
@@ -939,6 +935,7 @@ int multifd_save_setup(Error **errp)
/* We need one extra place for the packet header */
p->iov = g_new0(struct iovec, page_count + 1);
p->normal = g_new0(ram_addr_t, page_count);
+ p->page_size = qemu_target_page_size();
if (migrate_use_zero_copy_send()) {
p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
@@ -1186,6 +1183,7 @@ int multifd_load_setup(Error **errp)
p->name = g_strdup_printf("multifdrecv_%d", i);
p->iov = g_new0(struct iovec, page_count);
p->normal = g_new0(ram_addr_t, page_count);
+ p->page_size = qemu_target_page_size();
}
for (i = 0; i < thread_count; i++) {
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv,Send}Params
2022-08-02 6:38 ` [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv, Send}Params Juan Quintela
@ 2022-08-11 8:10 ` Leonardo Brás
2022-08-13 15:41 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 8:10 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
Hello Juan,
On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
> We were calling qemu_target_page_size() left and right.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
IMHO looks a good idea to bring that info inside the multifd parameters.
> ---
> migration/multifd.h | 4 ++++
> migration/multifd-zlib.c | 14 ++++++--------
> migration/multifd-zstd.c | 12 +++++-------
> migration/multifd.c | 18 ++++++++----------
> 4 files changed, 23 insertions(+), 25 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 519f498643..86fb9982b3 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -80,6 +80,8 @@ typedef struct {
> bool registered_yank;
> /* packet allocated len */
> uint32_t packet_len;
> + /* guest page size */
> + uint32_t page_size;
> /* multifd flags for sending ram */
> int write_flags;
>
> @@ -143,6 +145,8 @@ typedef struct {
> QIOChannel *c;
> /* packet allocated len */
> uint32_t packet_len;
> + /* guest page size */
> + uint32_t page_size;
>
> /* syncs main thread and channels */
> QemuSemaphore sem_sync;
> diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
> index 18213a9513..37770248e1 100644
> --- a/migration/multifd-zlib.c
> +++ b/migration/multifd-zlib.c
> @@ -116,7 +116,6 @@ static void zlib_send_cleanup(MultiFDSendParams *p, Error **errp)
> static int zlib_send_prepare(MultiFDSendParams *p, Error **errp)
> {
> struct zlib_data *z = p->data;
> - size_t page_size = qemu_target_page_size();
> z_stream *zs = &z->zs;
> uint32_t out_size = 0;
> int ret;
> @@ -135,8 +134,8 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error **errp)
> * with compression. zlib does not guarantee that this is safe,
> * therefore copy the page before calling deflate().
> */
> - memcpy(z->buf, p->pages->block->host + p->normal[i], page_size);
> - zs->avail_in = page_size;
> + memcpy(z->buf, p->pages->block->host + p->normal[i], p->page_size);
> + zs->avail_in = p->page_size;
> zs->next_in = z->buf;
>
> zs->avail_out = available;
> @@ -242,12 +241,11 @@ static void zlib_recv_cleanup(MultiFDRecvParams *p)
> static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
> {
> struct zlib_data *z = p->data;
> - size_t page_size = qemu_target_page_size();
> z_stream *zs = &z->zs;
> uint32_t in_size = p->next_packet_size;
> /* we measure the change of total_out */
> uint32_t out_size = zs->total_out;
> - uint32_t expected_size = p->normal_num * page_size;
> + uint32_t expected_size = p->normal_num * p->page_size;
> uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
> int ret;
> int i;
> @@ -274,7 +272,7 @@ static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
> flush = Z_SYNC_FLUSH;
> }
>
> - zs->avail_out = page_size;
> + zs->avail_out = p->page_size;
> zs->next_out = p->host + p->normal[i];
>
> /*
> @@ -288,8 +286,8 @@ static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
> do {
> ret = inflate(zs, flush);
> } while (ret == Z_OK && zs->avail_in
> - && (zs->total_out - start) < page_size);
> - if (ret == Z_OK && (zs->total_out - start) < page_size) {
> + && (zs->total_out - start) < p->page_size);
> + if (ret == Z_OK && (zs->total_out - start) < p->page_size) {
> error_setg(errp, "multifd %u: inflate generated too few output",
> p->id);
> return -1;
> diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c
> index d788d309f2..f4a8e1ed1f 100644
> --- a/migration/multifd-zstd.c
> +++ b/migration/multifd-zstd.c
> @@ -113,7 +113,6 @@ static void zstd_send_cleanup(MultiFDSendParams *p, Error **errp)
> static int zstd_send_prepare(MultiFDSendParams *p, Error **errp)
> {
> struct zstd_data *z = p->data;
> - size_t page_size = qemu_target_page_size();
> int ret;
> uint32_t i;
>
> @@ -128,7 +127,7 @@ static int zstd_send_prepare(MultiFDSendParams *p, Error **errp)
> flush = ZSTD_e_flush;
> }
> z->in.src = p->pages->block->host + p->normal[i];
> - z->in.size = page_size;
> + z->in.size = p->page_size;
> z->in.pos = 0;
>
> /*
> @@ -241,8 +240,7 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
> {
> uint32_t in_size = p->next_packet_size;
> uint32_t out_size = 0;
> - size_t page_size = qemu_target_page_size();
> - uint32_t expected_size = p->normal_num * page_size;
> + uint32_t expected_size = p->normal_num * p->page_size;
> uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
> struct zstd_data *z = p->data;
> int ret;
> @@ -265,7 +263,7 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
>
> for (i = 0; i < p->normal_num; i++) {
> z->out.dst = p->host + p->normal[i];
> - z->out.size = page_size;
> + z->out.size = p->page_size;
> z->out.pos = 0;
>
> /*
> @@ -279,8 +277,8 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
> do {
> ret = ZSTD_decompressStream(z->zds, &z->out, &z->in);
> } while (ret > 0 && (z->in.size - z->in.pos > 0)
> - && (z->out.pos < page_size));
> - if (ret > 0 && (z->out.pos < page_size)) {
> + && (z->out.pos < p->page_size));
> + if (ret > 0 && (z->out.pos < p->page_size)) {
> error_setg(errp, "multifd %u: decompressStream buffer too small",
> p->id);
> return -1;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 586ddc9d65..d2070c9cee 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -87,15 +87,14 @@ static void nocomp_send_cleanup(MultiFDSendParams *p, Error **errp)
> static int nocomp_send_prepare(MultiFDSendParams *p, Error **errp)
> {
> MultiFDPages_t *pages = p->pages;
> - size_t page_size = qemu_target_page_size();
>
> for (int i = 0; i < p->normal_num; i++) {
> p->iov[p->iovs_num].iov_base = pages->block->host + p->normal[i];
> - p->iov[p->iovs_num].iov_len = page_size;
> + p->iov[p->iovs_num].iov_len = p->page_size;
> p->iovs_num++;
> }
>
> - p->next_packet_size = p->normal_num * page_size;
> + p->next_packet_size = p->normal_num * p->page_size;
> p->flags |= MULTIFD_FLAG_NOCOMP;
> return 0;
> }
> @@ -139,7 +138,6 @@ static void nocomp_recv_cleanup(MultiFDRecvParams *p)
> static int nocomp_recv_pages(MultiFDRecvParams *p, Error **errp)
> {
> uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
> - size_t page_size = qemu_target_page_size();
>
> if (flags != MULTIFD_FLAG_NOCOMP) {
> error_setg(errp, "multifd %u: flags received %x flags expected %x",
> @@ -148,7 +146,7 @@ static int nocomp_recv_pages(MultiFDRecvParams *p, Error **errp)
> }
> for (int i = 0; i < p->normal_num; i++) {
> p->iov[i].iov_base = p->host + p->normal[i];
> - p->iov[i].iov_len = page_size;
> + p->iov[i].iov_len = p->page_size;
> }
> return qio_channel_readv_all(p->c, p->iov, p->normal_num, errp);
> }
> @@ -281,8 +279,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
> static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> {
> MultiFDPacket_t *packet = p->packet;
> - size_t page_size = qemu_target_page_size();
> - uint32_t page_count = MULTIFD_PACKET_SIZE / page_size;
> + uint32_t page_count = MULTIFD_PACKET_SIZE / p->page_size;
> RAMBlock *block;
> int i;
>
> @@ -344,7 +341,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> for (i = 0; i < p->normal_num; i++) {
> uint64_t offset = be64_to_cpu(packet->offset[i]);
>
> - if (offset > (block->used_length - page_size)) {
> + if (offset > (block->used_length - p->page_size)) {
> error_setg(errp, "multifd: offset too long %" PRIu64
> " (max " RAM_ADDR_FMT ")",
> offset, block->used_length);
> @@ -433,8 +430,7 @@ static int multifd_send_pages(QEMUFile *f)
> p->packet_num = multifd_send_state->packet_num++;
> multifd_send_state->pages = p->pages;
> p->pages = pages;
> - transferred = ((uint64_t) pages->num) * qemu_target_page_size()
> - + p->packet_len;
> + transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
> qemu_file_acct_rate_limit(f, transferred);
> ram_counters.multifd_bytes += transferred;
> ram_counters.transferred += transferred;
> @@ -939,6 +935,7 @@ int multifd_save_setup(Error **errp)
> /* We need one extra place for the packet header */
> p->iov = g_new0(struct iovec, page_count + 1);
> p->normal = g_new0(ram_addr_t, page_count);
> + p->page_size = qemu_target_page_size();
>
> if (migrate_use_zero_copy_send()) {
> p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
> @@ -1186,6 +1183,7 @@ int multifd_load_setup(Error **errp)
> p->name = g_strdup_printf("multifdrecv_%d", i);
> p->iov = g_new0(struct iovec, page_count);
> p->normal = g_new0(ram_addr_t, page_count);
> + p->page_size = qemu_target_page_size();
> }
>
> for (i = 0; i < thread_count; i++) {
IIUC this info should never change after assigned, and is the same on every
multifd channel param.
I wonder if it would be interesting to have a common area for this kind of info,
which could be referenced by every multifd channel parameter.
Or maybe too much trouble?
Anyway, FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
Best regards,
Leo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv,Send}Params
2022-08-11 8:10 ` [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv,Send}Params Leonardo Brás
@ 2022-08-13 15:41 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-13 15:41 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> Hello Juan,
>
> On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
>> We were calling qemu_target_page_size() left and right.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> IMHO looks a good idea to bring that info inside the multifd parameters.
Thanks.
[...]
> IIUC this info should never change after assigned, and is the same on every
> multifd channel param.
registered_yank? Perhaps, I have to look.
packet_len, page_size, page_count and write_flags: They never really change.
But on the other hand, So we are "wasting" 16bytes per channel.
> I wonder if it would be interesting to have a common area for this kind of info,
> which could be referenced by every multifd channel parameter.
> Or maybe too much trouble?
Will take a look on the future, the bigger problem that I can think of
is that we are already passing the MultiFD{Send,Recv}Params to each
function, so having them globaly will means to have global variable or
adding a pointer (8bytes) to each params, so not sure it is a good idea
with current amount of constants.
> Anyway, FWIW:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 02/12] multifd: Create page_count fields into both MultiFD{Recv, Send}Params
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
2022-08-02 6:38 ` [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv, Send}Params Juan Quintela
@ 2022-08-02 6:38 ` Juan Quintela
2022-08-11 8:10 ` [PATCH v7 02/12] multifd: Create page_count fields into both MultiFD{Recv,Send}Params Leonardo Brás
2022-08-02 6:38 ` [PATCH v7 03/12] migration: Export ram_transferred_ram() Juan Quintela
` (9 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
We were recalculating it left and right. We plan to change that
values on next patches.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd.h | 4 ++++
migration/multifd.c | 7 ++++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index 86fb9982b3..e2802a9ce2 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -82,6 +82,8 @@ typedef struct {
uint32_t packet_len;
/* guest page size */
uint32_t page_size;
+ /* number of pages in a full packet */
+ uint32_t page_count;
/* multifd flags for sending ram */
int write_flags;
@@ -147,6 +149,8 @@ typedef struct {
uint32_t packet_len;
/* guest page size */
uint32_t page_size;
+ /* number of pages in a full packet */
+ uint32_t page_count;
/* syncs main thread and channels */
QemuSemaphore sem_sync;
diff --git a/migration/multifd.c b/migration/multifd.c
index d2070c9cee..aa3808a6f4 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -279,7 +279,6 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
{
MultiFDPacket_t *packet = p->packet;
- uint32_t page_count = MULTIFD_PACKET_SIZE / p->page_size;
RAMBlock *block;
int i;
@@ -306,10 +305,10 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
* If we received a packet that is 100 times bigger than expected
* just stop migration. It is a magic number.
*/
- if (packet->pages_alloc > page_count) {
+ if (packet->pages_alloc > p->page_count) {
error_setg(errp, "multifd: received packet "
"with size %u and expected a size of %u",
- packet->pages_alloc, page_count) ;
+ packet->pages_alloc, p->page_count) ;
return -1;
}
@@ -936,6 +935,7 @@ int multifd_save_setup(Error **errp)
p->iov = g_new0(struct iovec, page_count + 1);
p->normal = g_new0(ram_addr_t, page_count);
p->page_size = qemu_target_page_size();
+ p->page_count = page_count;
if (migrate_use_zero_copy_send()) {
p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
@@ -1183,6 +1183,7 @@ int multifd_load_setup(Error **errp)
p->name = g_strdup_printf("multifdrecv_%d", i);
p->iov = g_new0(struct iovec, page_count);
p->normal = g_new0(ram_addr_t, page_count);
+ p->page_count = page_count;
p->page_size = qemu_target_page_size();
}
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 02/12] multifd: Create page_count fields into both MultiFD{Recv,Send}Params
2022-08-02 6:38 ` [PATCH v7 02/12] multifd: Create page_count fields into both MultiFD{Recv, Send}Params Juan Quintela
@ 2022-08-11 8:10 ` Leonardo Brás
0 siblings, 0 replies; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 8:10 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
> We were recalculating it left and right. We plan to change that
> values on next patches.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/multifd.h | 4 ++++
> migration/multifd.c | 7 ++++---
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 86fb9982b3..e2802a9ce2 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -82,6 +82,8 @@ typedef struct {
> uint32_t packet_len;
> /* guest page size */
> uint32_t page_size;
> + /* number of pages in a full packet */
> + uint32_t page_count;
> /* multifd flags for sending ram */
> int write_flags;
>
> @@ -147,6 +149,8 @@ typedef struct {
> uint32_t packet_len;
> /* guest page size */
> uint32_t page_size;
> + /* number of pages in a full packet */
> + uint32_t page_count;
>
> /* syncs main thread and channels */
> QemuSemaphore sem_sync;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index d2070c9cee..aa3808a6f4 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -279,7 +279,6 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
> static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> {
> MultiFDPacket_t *packet = p->packet;
> - uint32_t page_count = MULTIFD_PACKET_SIZE / p->page_size;
> RAMBlock *block;
> int i;
>
> @@ -306,10 +305,10 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> * If we received a packet that is 100 times bigger than expected
> * just stop migration. It is a magic number.
> */
> - if (packet->pages_alloc > page_count) {
> + if (packet->pages_alloc > p->page_count) {
> error_setg(errp, "multifd: received packet "
> "with size %u and expected a size of %u",
> - packet->pages_alloc, page_count) ;
> + packet->pages_alloc, p->page_count) ;
> return -1;
> }
>
> @@ -936,6 +935,7 @@ int multifd_save_setup(Error **errp)
> p->iov = g_new0(struct iovec, page_count + 1);
> p->normal = g_new0(ram_addr_t, page_count);
> p->page_size = qemu_target_page_size();
> + p->page_count = page_count;
>
> if (migrate_use_zero_copy_send()) {
> p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
> @@ -1183,6 +1183,7 @@ int multifd_load_setup(Error **errp)
> p->name = g_strdup_printf("multifdrecv_%d", i);
> p->iov = g_new0(struct iovec, page_count);
> p->normal = g_new0(ram_addr_t, page_count);
> + p->page_count = page_count;
> p->page_size = qemu_target_page_size();
> }
>
Same comment as Patch [1/12] here.
FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
Best regards,
Leo
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 03/12] migration: Export ram_transferred_ram()
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
2022-08-02 6:38 ` [PATCH v7 01/12] multifd: Create page_size fields into both MultiFD{Recv, Send}Params Juan Quintela
2022-08-02 6:38 ` [PATCH v7 02/12] multifd: Create page_count fields into both MultiFD{Recv, Send}Params Juan Quintela
@ 2022-08-02 6:38 ` Juan Quintela
2022-08-11 8:11 ` Leonardo Brás
2022-08-02 6:38 ` [PATCH v7 04/12] multifd: Count the number of bytes sent correctly Juan Quintela
` (8 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost,
David Edmondson
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.h | 2 ++
migration/ram.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/migration/ram.h b/migration/ram.h
index c7af65ac74..e844966f69 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -65,6 +65,8 @@ int ram_load_postcopy(QEMUFile *f, int channel);
void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
+void ram_transferred_add(uint64_t bytes);
+
int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
bool ramblock_recv_bitmap_test_byte_offset(RAMBlock *rb, uint64_t byte_offset);
void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
diff --git a/migration/ram.c b/migration/ram.c
index b94669ba5d..85d89d61ac 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -422,7 +422,7 @@ uint64_t ram_bytes_remaining(void)
MigrationStats ram_counters;
-static void ram_transferred_add(uint64_t bytes)
+void ram_transferred_add(uint64_t bytes)
{
if (runstate_is_running()) {
ram_counters.precopy_bytes += bytes;
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 03/12] migration: Export ram_transferred_ram()
2022-08-02 6:38 ` [PATCH v7 03/12] migration: Export ram_transferred_ram() Juan Quintela
@ 2022-08-11 8:11 ` Leonardo Brás
2022-08-13 15:36 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 8:11 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost, David Edmondson
On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Is this doubled Signed-off-by intentional?
> ---
> migration/ram.h | 2 ++
> migration/ram.c | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.h b/migration/ram.h
> index c7af65ac74..e844966f69 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -65,6 +65,8 @@ int ram_load_postcopy(QEMUFile *f, int channel);
>
> void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>
> +void ram_transferred_add(uint64_t bytes);
> +
> int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
> bool ramblock_recv_bitmap_test_byte_offset(RAMBlock *rb, uint64_t byte_offset);
> void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
> diff --git a/migration/ram.c b/migration/ram.c
> index b94669ba5d..85d89d61ac 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -422,7 +422,7 @@ uint64_t ram_bytes_remaining(void)
>
> MigrationStats ram_counters;
>
> -static void ram_transferred_add(uint64_t bytes)
> +void ram_transferred_add(uint64_t bytes)
> {
> if (runstate_is_running()) {
> ram_counters.precopy_bytes += bytes;
Other than that, FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
Best regards,
Leo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 03/12] migration: Export ram_transferred_ram()
2022-08-11 8:11 ` Leonardo Brás
@ 2022-08-13 15:36 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-13 15:36 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost, David Edmondson
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Is this doubled Signed-off-by intentional?
It is .git/hooks/prepare-commit-msg for you.
Adding --if-exits doNothing
And we will see how it breaks in (other) very subtle ways.
>
> Other than that, FWIW:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 04/12] multifd: Count the number of bytes sent correctly
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (2 preceding siblings ...)
2022-08-02 6:38 ` [PATCH v7 03/12] migration: Export ram_transferred_ram() Juan Quintela
@ 2022-08-02 6:38 ` Juan Quintela
2022-08-11 8:11 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer Juan Quintela
` (7 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:38 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
Current code asumes that all pages are whole. That is not true for
example for compression already. Fix it for creating a new field
->sent_bytes that includes it.
All ram_counters are used only from the migration thread, so we have
two options:
- put a mutex and fill everything when we sent it (not only
ram_counters, also qemu_file->xfer_bytes).
- Create a local variable that implements how much has been sent
through each channel. And when we push another packet, we "add" the
previous stats.
I choose two due to less changes overall. On the previous code we
increase transferred and then we sent. Current code goes the other
way around. It sents the data, and after the fact, it updates the
counters. Notice that each channel can have a maximum of half a
megabyte of data without counting, so it is not very important.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd.h | 2 ++
migration/multifd.c | 14 ++++++--------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index e2802a9ce2..36f899c56f 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -102,6 +102,8 @@ typedef struct {
uint32_t flags;
/* global number of generated multifd packets */
uint64_t packet_num;
+ /* How many bytes have we sent on the last packet */
+ uint64_t sent_bytes;
/* thread has work to do */
int pending_job;
/* array of pages to sent.
diff --git a/migration/multifd.c b/migration/multifd.c
index aa3808a6f4..e25b529235 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -394,7 +394,6 @@ static int multifd_send_pages(QEMUFile *f)
static int next_channel;
MultiFDSendParams *p = NULL; /* make happy gcc */
MultiFDPages_t *pages = multifd_send_state->pages;
- uint64_t transferred;
if (qatomic_read(&multifd_send_state->exiting)) {
return -1;
@@ -429,10 +428,10 @@ static int multifd_send_pages(QEMUFile *f)
p->packet_num = multifd_send_state->packet_num++;
multifd_send_state->pages = p->pages;
p->pages = pages;
- transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
- qemu_file_acct_rate_limit(f, transferred);
- ram_counters.multifd_bytes += transferred;
- ram_counters.transferred += transferred;
+ ram_transferred_add(p->sent_bytes);
+ ram_counters.multifd_bytes += p->sent_bytes;
+ qemu_file_acct_rate_limit(f, p->sent_bytes);
+ p->sent_bytes = 0;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
@@ -605,9 +604,6 @@ int multifd_send_sync_main(QEMUFile *f)
p->packet_num = multifd_send_state->packet_num++;
p->flags |= MULTIFD_FLAG_SYNC;
p->pending_job++;
- qemu_file_acct_rate_limit(f, p->packet_len);
- ram_counters.multifd_bytes += p->packet_len;
- ram_counters.transferred += p->packet_len;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
@@ -714,6 +710,8 @@ static void *multifd_send_thread(void *opaque)
}
qemu_mutex_lock(&p->mutex);
+ p->sent_bytes += p->packet_len;;
+ p->sent_bytes += p->next_packet_size;
p->pending_job--;
qemu_mutex_unlock(&p->mutex);
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 04/12] multifd: Count the number of bytes sent correctly
2022-08-02 6:38 ` [PATCH v7 04/12] multifd: Count the number of bytes sent correctly Juan Quintela
@ 2022-08-11 8:11 ` Leonardo Brás
2022-08-19 9:35 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 8:11 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
> Current code asumes that all pages are whole. That is not true for
> example for compression already. Fix it for creating a new field
> ->sent_bytes that includes it.
>
> All ram_counters are used only from the migration thread, so we have
> two options:
> - put a mutex and fill everything when we sent it (not only
> ram_counters, also qemu_file->xfer_bytes).
> - Create a local variable that implements how much has been sent
> through each channel. And when we push another packet, we "add" the
> previous stats.
>
> I choose two due to less changes overall. On the previous code we
> increase transferred and then we sent. Current code goes the other
> way around. It sents the data, and after the fact, it updates the
> counters. Notice that each channel can have a maximum of half a
> megabyte of data without counting, so it is not very important.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/multifd.h | 2 ++
> migration/multifd.c | 14 ++++++--------
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index e2802a9ce2..36f899c56f 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -102,6 +102,8 @@ typedef struct {
> uint32_t flags;
> /* global number of generated multifd packets */
> uint64_t packet_num;
> + /* How many bytes have we sent on the last packet */
> + uint64_t sent_bytes;
> /* thread has work to do */
> int pending_job;
> /* array of pages to sent.
> diff --git a/migration/multifd.c b/migration/multifd.c
> index aa3808a6f4..e25b529235 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -394,7 +394,6 @@ static int multifd_send_pages(QEMUFile *f)
> static int next_channel;
> MultiFDSendParams *p = NULL; /* make happy gcc */
> MultiFDPages_t *pages = multifd_send_state->pages;
> - uint64_t transferred;
>
> if (qatomic_read(&multifd_send_state->exiting)) {
> return -1;
> @@ -429,10 +428,10 @@ static int multifd_send_pages(QEMUFile *f)
> p->packet_num = multifd_send_state->packet_num++;
> multifd_send_state->pages = p->pages;
> p->pages = pages;
> - transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
> - qemu_file_acct_rate_limit(f, transferred);
> - ram_counters.multifd_bytes += transferred;
> - ram_counters.transferred += transferred;
> + ram_transferred_add(p->sent_bytes);
> + ram_counters.multifd_bytes += p->sent_bytes;
I'm worndering if we could avoid having this last line by having
ram_transferred_add() to include:
if (migrate_use_multifd()) {
ram_counters.multifd_bytes += bytes;
}
But I am not sure if other usages from ram_transferred_add() could interfere.
> + qemu_file_acct_rate_limit(f, p->sent_bytes);
> + p->sent_bytes = 0;
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_post(&p->sem);
>
> @@ -605,9 +604,6 @@ int multifd_send_sync_main(QEMUFile *f)
> p->packet_num = multifd_send_state->packet_num++;
> p->flags |= MULTIFD_FLAG_SYNC;
> p->pending_job++;
> - qemu_file_acct_rate_limit(f, p->packet_len);
> - ram_counters.multifd_bytes += p->packet_len;
> - ram_counters.transferred += p->packet_len;
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_post(&p->sem);
>
> @@ -714,6 +710,8 @@ static void *multifd_send_thread(void *opaque)
> }
>
> qemu_mutex_lock(&p->mutex);
> + p->sent_bytes += p->packet_len;;
Double semicolon.
> + p->sent_bytes += p->next_packet_size;
> p->pending_job--;
> qemu_mutex_unlock(&p->mutex);
>
IIUC, it changes how rate-limiting and ram counters perceive how many bytes have
been sent, by counting actual bytes instead of page multiples. This should
reflect what have been actually sent (in terms of rate limiting).
I'm wondering if having the ram_counters.transferred to reflect acutal bytes,
instead of the number of pages * pagesize will cause any user (or management
code) to be confuse in any way.
Other than that:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
Best regards,
Leo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 04/12] multifd: Count the number of bytes sent correctly
2022-08-11 8:11 ` Leonardo Brás
@ 2022-08-19 9:35 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-19 9:35 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:38 +0200, Juan Quintela wrote:
>> Current code asumes that all pages are whole. That is not true for
>> example for compression already. Fix it for creating a new field
>> ->sent_bytes that includes it.
>>
>> All ram_counters are used only from the migration thread, so we have
>> two options:
>> - put a mutex and fill everything when we sent it (not only
>> ram_counters, also qemu_file->xfer_bytes).
>> - Create a local variable that implements how much has been sent
>> through each channel. And when we push another packet, we "add" the
>> previous stats.
>>
>> I choose two due to less changes overall. On the previous code we
>> increase transferred and then we sent. Current code goes the other
>> way around. It sents the data, and after the fact, it updates the
>> counters. Notice that each channel can have a maximum of half a
>> megabyte of data without counting, so it is not very important.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/multifd.h | 2 ++
>> migration/multifd.c | 14 ++++++--------
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index e2802a9ce2..36f899c56f 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -102,6 +102,8 @@ typedef struct {
>> uint32_t flags;
>> /* global number of generated multifd packets */
>> uint64_t packet_num;
>> + /* How many bytes have we sent on the last packet */
>> + uint64_t sent_bytes;
>> /* thread has work to do */
>> int pending_job;
>> /* array of pages to sent.
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index aa3808a6f4..e25b529235 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -394,7 +394,6 @@ static int multifd_send_pages(QEMUFile *f)
>> static int next_channel;
>> MultiFDSendParams *p = NULL; /* make happy gcc */
>> MultiFDPages_t *pages = multifd_send_state->pages;
>> - uint64_t transferred;
>>
>> if (qatomic_read(&multifd_send_state->exiting)) {
>> return -1;
>> @@ -429,10 +428,10 @@ static int multifd_send_pages(QEMUFile *f)
>> p->packet_num = multifd_send_state->packet_num++;
>> multifd_send_state->pages = p->pages;
>> p->pages = pages;
>> - transferred = ((uint64_t) pages->num) * p->page_size + p->packet_len;
>> - qemu_file_acct_rate_limit(f, transferred);
>> - ram_counters.multifd_bytes += transferred;
>> - ram_counters.transferred += transferred;
>> + ram_transferred_add(p->sent_bytes);
>> + ram_counters.multifd_bytes += p->sent_bytes;
>
> I'm worndering if we could avoid having this last line by having
> ram_transferred_add() to include:
>
> if (migrate_use_multifd()) {
> ram_counters.multifd_bytes += bytes;
> }
>
> But I am not sure if other usages from ram_transferred_add() could interfere.
I preffer not to, because ram_addr_ram() is also used for non multifd code.
> Double semicolon.
Fixed, thanks.
>> + p->sent_bytes += p->next_packet_size;
>> p->pending_job--;
>> qemu_mutex_unlock(&p->mutex);
>>
>
> IIUC, it changes how rate-limiting and ram counters perceive how many bytes have
> been sent, by counting actual bytes instead of page multiples. This should
> reflect what have been actually sent (in terms of rate limiting).
>
> I'm wondering if having the ram_counters.transferred to reflect acutal bytes,
> instead of the number of pages * pagesize will cause any user (or management
> code) to be confuse in any way.
It shouldn't, because we already have things that don't sent that data
as multiples:
- any compression code
- xbzrle
so I think we are right here.
> Other than that:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (3 preceding siblings ...)
2022-08-02 6:38 ` [PATCH v7 04/12] multifd: Count the number of bytes sent correctly Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-08-11 8:11 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 06/12] multifd: Make flags field thread local Juan Quintela
` (6 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
We are going to create a new function for multifd latest in the series.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 85d89d61ac..499d9b2a90 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -310,6 +310,9 @@ typedef struct {
bool preempted;
} PostcopyPreemptState;
+typedef struct RAMState RAMState;
+typedef struct PageSearchStatus PageSearchStatus;
+
/* State of RAM for migration */
struct RAMState {
/* QEMUFile used for this migration */
@@ -372,8 +375,9 @@ struct RAMState {
* is enabled.
*/
unsigned int postcopy_channel;
+
+ int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
};
-typedef struct RAMState RAMState;
static RAMState *ram_state;
@@ -2255,14 +2259,14 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
}
/**
- * ram_save_target_page: save one target page
+ * ram_save_target_page_legacy: save one target page
*
* Returns the number of pages written
*
* @rs: current RAM state
* @pss: data about the page we want to send
*/
-static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
+static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
{
RAMBlock *block = pss->block;
ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
@@ -2469,7 +2473,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss)
/* Check the pages is dirty and if it is send it */
if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
- tmppages = ram_save_target_page(rs, pss);
+ tmppages = rs->ram_save_target_page(rs, pss);
if (tmppages < 0) {
return tmppages;
}
@@ -3223,6 +3227,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
ram_control_before_iterate(f, RAM_CONTROL_SETUP);
ram_control_after_iterate(f, RAM_CONTROL_SETUP);
+ (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
ret = multifd_send_sync_main(f);
if (ret < 0) {
return ret;
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer
2022-08-02 6:39 ` [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer Juan Quintela
@ 2022-08-11 8:11 ` Leonardo Brás
2022-08-19 9:51 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 8:11 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> We are going to create a new function for multifd latest in the series.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Double Signed-off-by again.
> ---
> migration/ram.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 85d89d61ac..499d9b2a90 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -310,6 +310,9 @@ typedef struct {
> bool preempted;
> } PostcopyPreemptState;
>
> +typedef struct RAMState RAMState;
> +typedef struct PageSearchStatus PageSearchStatus;
> +
> /* State of RAM for migration */
> struct RAMState {
> /* QEMUFile used for this migration */
> @@ -372,8 +375,9 @@ struct RAMState {
> * is enabled.
> */
> unsigned int postcopy_channel;
> +
> + int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> };
> -typedef struct RAMState RAMState;
>
> static RAMState *ram_state;
>
> @@ -2255,14 +2259,14 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
> }
>
> /**
> - * ram_save_target_page: save one target page
> + * ram_save_target_page_legacy: save one target page
> *
> * Returns the number of pages written
> *
> * @rs: current RAM state
> * @pss: data about the page we want to send
> */
> -static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
> +static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
> {
> RAMBlock *block = pss->block;
> ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
> @@ -2469,7 +2473,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss)
>
> /* Check the pages is dirty and if it is send it */
> if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> - tmppages = ram_save_target_page(rs, pss);
> + tmppages = rs->ram_save_target_page(rs, pss);
> if (tmppages < 0) {
> return tmppages;
> }
> @@ -3223,6 +3227,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> ram_control_before_iterate(f, RAM_CONTROL_SETUP);
> ram_control_after_iterate(f, RAM_CONTROL_SETUP);
>
> + (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
> ret = multifd_send_sync_main(f);
> if (ret < 0) {
> return ret;
So, IIUC:
- Rename ram_save_target_page -> ram_save_target_page_legacy
- Add a function pointer to RAMState (or a callback)
- Assign function pointer = ram_save_target_page_legacy at setup
- Replace ram_save_target_page() by indirect function call using above pointer.
I could see no issue in this, so I belive it works fine.
The only thing that concerns me is the name RAMState.
IMHO, a struct named RAMState is supposed to just reflect the state of ram (or
according to this struct's comments, the state of RAM for migration. Having a
function pointer here that saves a page seems counterintuitive, since it does
not reflect the state of RAM.
Maybe we could rename the struct, or even better, create another struct that
could look something like this:
struct RAMMigration {
RAMState state;
int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
/* Other callbacks or further info.*/
}
What do you think about it?
Best regards,
Leo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer
2022-08-11 8:11 ` Leonardo Brás
@ 2022-08-19 9:51 ` Juan Quintela
2022-08-20 7:14 ` Leonardo Bras Soares Passos
0 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-19 9:51 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> We are going to create a new function for multifd latest in the series.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Double Signed-off-by again.
>
>> ---
>> migration/ram.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 85d89d61ac..499d9b2a90 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -310,6 +310,9 @@ typedef struct {
>> bool preempted;
>> } PostcopyPreemptState;
>>
>> +typedef struct RAMState RAMState;
>> +typedef struct PageSearchStatus PageSearchStatus;
>> +
>> /* State of RAM for migration */
>> struct RAMState {
>> /* QEMUFile used for this migration */
>> @@ -372,8 +375,9 @@ struct RAMState {
>> * is enabled.
>> */
>> unsigned int postcopy_channel;
>> +
>> + int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
>> };
>> -typedef struct RAMState RAMState;
>>
>> static RAMState *ram_state;
>>
>> @@ -2255,14 +2259,14 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
>> }
>>
>> /**
>> - * ram_save_target_page: save one target page
>> + * ram_save_target_page_legacy: save one target page
>> *
>> * Returns the number of pages written
>> *
>> * @rs: current RAM state
>> * @pss: data about the page we want to send
>> */
>> -static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
>> +static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>> {
>> RAMBlock *block = pss->block;
>> ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
>> @@ -2469,7 +2473,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss)
>>
>> /* Check the pages is dirty and if it is send it */
>> if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
>> - tmppages = ram_save_target_page(rs, pss);
>> + tmppages = rs->ram_save_target_page(rs, pss);
>> if (tmppages < 0) {
>> return tmppages;
>> }
>> @@ -3223,6 +3227,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>> ram_control_before_iterate(f, RAM_CONTROL_SETUP);
>> ram_control_after_iterate(f, RAM_CONTROL_SETUP);
>>
>> + (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
>> ret = multifd_send_sync_main(f);
>> if (ret < 0) {
>> return ret;
>
>
> So, IIUC:
> - Rename ram_save_target_page -> ram_save_target_page_legacy
> - Add a function pointer to RAMState (or a callback)
> - Assign function pointer = ram_save_target_page_legacy at setup
> - Replace ram_save_target_page() by indirect function call using above pointer.
>
> I could see no issue in this, so I belive it works fine.
>
> The only thing that concerns me is the name RAMState.
Every device state is setup in RAMState.
> IMHO, a struct named RAMState is supposed to just reflect the state of ram (or
> according to this struct's comments, the state of RAM for migration. Having a
> function pointer here that saves a page seems counterintuitive, since it does
> not reflect the state of RAM.
The big problem for adding another struct is that we would have to
change all the callers, or yet another global variable. Both are bad
idea in my humble opinion.
> Maybe we could rename the struct, or even better, create another struct that
> could look something like this:
>
> struct RAMMigration {
> RAMState state;
> int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> /* Other callbacks or further info.*/
> }
>
> What do you think about it?
Really this depends on configuration. What is setup for qemu
migration. I think this is the easiest way to do it, we can add a new
struct, but it gets everything much more complicated:
- the value that we receive in ram_save_setup() is a RAMState
- We would have to change all the callers form
* ram_save_iterate()
* ram_find_and_save_block()
* ram_save_host_page()
So I think it is quite a bit of churn for not a lot of gain.
Later, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer
2022-08-19 9:51 ` Juan Quintela
@ 2022-08-20 7:14 ` Leonardo Bras Soares Passos
2022-08-22 21:35 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Bras Soares Passos @ 2022-08-20 7:14 UTC (permalink / raw)
To: Juan Quintela
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
On Fri, Aug 19, 2022 at 6:52 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> We are going to create a new function for multifd latest in the series.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > Double Signed-off-by again.
> >
> >> ---
> >> migration/ram.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 85d89d61ac..499d9b2a90 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -310,6 +310,9 @@ typedef struct {
> >> bool preempted;
> >> } PostcopyPreemptState;
> >>
> >> +typedef struct RAMState RAMState;
> >> +typedef struct PageSearchStatus PageSearchStatus;
> >> +
> >> /* State of RAM for migration */
> >> struct RAMState {
> >> /* QEMUFile used for this migration */
> >> @@ -372,8 +375,9 @@ struct RAMState {
> >> * is enabled.
> >> */
> >> unsigned int postcopy_channel;
> >> +
> >> + int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> >> };
> >> -typedef struct RAMState RAMState;
> >>
> >> static RAMState *ram_state;
> >>
> >> @@ -2255,14 +2259,14 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
> >> }
> >>
> >> /**
> >> - * ram_save_target_page: save one target page
> >> + * ram_save_target_page_legacy: save one target page
> >> *
> >> * Returns the number of pages written
> >> *
> >> * @rs: current RAM state
> >> * @pss: data about the page we want to send
> >> */
> >> -static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
> >> +static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
> >> {
> >> RAMBlock *block = pss->block;
> >> ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
> >> @@ -2469,7 +2473,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss)
> >>
> >> /* Check the pages is dirty and if it is send it */
> >> if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> >> - tmppages = ram_save_target_page(rs, pss);
> >> + tmppages = rs->ram_save_target_page(rs, pss);
> >> if (tmppages < 0) {
> >> return tmppages;
> >> }
> >> @@ -3223,6 +3227,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> >> ram_control_before_iterate(f, RAM_CONTROL_SETUP);
> >> ram_control_after_iterate(f, RAM_CONTROL_SETUP);
> >>
> >> + (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
> >> ret = multifd_send_sync_main(f);
> >> if (ret < 0) {
> >> return ret;
> >
> >
> > So, IIUC:
> > - Rename ram_save_target_page -> ram_save_target_page_legacy
> > - Add a function pointer to RAMState (or a callback)
> > - Assign function pointer = ram_save_target_page_legacy at setup
> > - Replace ram_save_target_page() by indirect function call using above pointer.
> >
> > I could see no issue in this, so I belive it works fine.
> >
> > The only thing that concerns me is the name RAMState.
>
> Every device state is setup in RAMState.
>
> > IMHO, a struct named RAMState is supposed to just reflect the state of ram (or
> > according to this struct's comments, the state of RAM for migration. Having a
> > function pointer here that saves a page seems counterintuitive, since it does
> > not reflect the state of RAM.
>
> The big problem for adding another struct is that we would have to
> change all the callers, or yet another global variable. Both are bad
> idea in my humble opinion.
>
> > Maybe we could rename the struct, or even better, create another struct that
> > could look something like this:
> >
> > struct RAMMigration {
> > RAMState state;
> > int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
> > /* Other callbacks or further info.*/
> > }
> >
> > What do you think about it?
>
> Really this depends on configuration. What is setup for qemu
> migration. I think this is the easiest way to do it, we can add a new
> struct, but it gets everything much more complicated:
>
> - the value that we receive in ram_save_setup() is a RAMState
> - We would have to change all the callers form
> * ram_save_iterate()
> * ram_find_and_save_block()
> * ram_save_host_page()
Maybe RAMState could be part of a bigger struct, and we could use
something like a container_of().
So whenever you want to use it, it would be available.
What about that?
>
> So I think it is quite a bit of churn for not a lot of gain.
>
> Later, Juan.
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer
2022-08-20 7:14 ` Leonardo Bras Soares Passos
@ 2022-08-22 21:35 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-22 21:35 UTC (permalink / raw)
To: Leonardo Bras Soares Passos
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Bras Soares Passos <leobras@redhat.com> wrote:
> On Fri, Aug 19, 2022 at 6:52 AM Juan Quintela <quintela@redhat.com> wrote:
>>
>> - the value that we receive in ram_save_setup() is a RAMState
>> - We would have to change all the callers form
>> * ram_save_iterate()
>> * ram_find_and_save_block()
>> * ram_save_host_page()
>
> Maybe RAMState could be part of a bigger struct, and we could use
> something like a container_of().
> So whenever you want to use it, it would be available.
>
> What about that?
New struct it is:
typedef struct {
int (*ram_save_target_page)(RAMState *rs, PageSearchStatus *pss);
} MigrationOps;
And go from there.
Later, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 06/12] multifd: Make flags field thread local
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (4 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 05/12] migration: Make ram_save_target_page() a pointer Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-08-11 9:04 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held Juan Quintela
` (5 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
Use of flags with respect to locking was incensistant. For the
sending side:
- it was set to 0 with mutex held on the multifd channel.
- MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
- Everything else was done without the mutex held on the multifd channel.
On the reception side, it is not used on the migration thread, only on
the multifd channels threads.
So we move it to the multifd channels thread only variables, and we
introduce a new bool sync_needed on the send side to pass that information.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd.h | 10 ++++++----
migration/multifd.c | 23 +++++++++++++----------
2 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index 36f899c56f..a67cefc0a2 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -98,12 +98,12 @@ typedef struct {
bool running;
/* should this thread finish */
bool quit;
- /* multifd flags for each packet */
- uint32_t flags;
/* global number of generated multifd packets */
uint64_t packet_num;
/* How many bytes have we sent on the last packet */
uint64_t sent_bytes;
+ /* Do we need to do an iteration sync */
+ bool sync_needed;
/* thread has work to do */
int pending_job;
/* array of pages to sent.
@@ -117,6 +117,8 @@ typedef struct {
/* pointer to the packet */
MultiFDPacket_t *packet;
+ /* multifd flags for each packet */
+ uint32_t flags;
/* size of the next packet that contains pages */
uint32_t next_packet_size;
/* packets sent through this channel */
@@ -163,8 +165,6 @@ typedef struct {
bool running;
/* should this thread finish */
bool quit;
- /* multifd flags for each packet */
- uint32_t flags;
/* global number of generated multifd packets */
uint64_t packet_num;
@@ -172,6 +172,8 @@ typedef struct {
/* pointer to the packet */
MultiFDPacket_t *packet;
+ /* multifd flags for each packet */
+ uint32_t flags;
/* size of the next packet that contains pages */
uint32_t next_packet_size;
/* packets sent through this channel */
diff --git a/migration/multifd.c b/migration/multifd.c
index e25b529235..09a40a9135 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -602,7 +602,7 @@ int multifd_send_sync_main(QEMUFile *f)
}
p->packet_num = multifd_send_state->packet_num++;
- p->flags |= MULTIFD_FLAG_SYNC;
+ p->sync_needed = true;
p->pending_job++;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
@@ -658,7 +658,11 @@ static void *multifd_send_thread(void *opaque)
if (p->pending_job) {
uint64_t packet_num = p->packet_num;
- uint32_t flags = p->flags;
+ p->flags = 0;
+ if (p->sync_needed) {
+ p->flags |= MULTIFD_FLAG_SYNC;
+ p->sync_needed = false;
+ }
p->normal_num = 0;
if (use_zero_copy_send) {
@@ -680,14 +684,13 @@ static void *multifd_send_thread(void *opaque)
}
}
multifd_send_fill_packet(p);
- p->flags = 0;
p->num_packets++;
p->total_normal_pages += p->normal_num;
p->pages->num = 0;
p->pages->block = NULL;
qemu_mutex_unlock(&p->mutex);
- trace_multifd_send(p->id, packet_num, p->normal_num, flags,
+ trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
p->next_packet_size);
if (use_zero_copy_send) {
@@ -715,7 +718,7 @@ static void *multifd_send_thread(void *opaque)
p->pending_job--;
qemu_mutex_unlock(&p->mutex);
- if (flags & MULTIFD_FLAG_SYNC) {
+ if (p->flags & MULTIFD_FLAG_SYNC) {
qemu_sem_post(&p->sem_sync);
}
qemu_sem_post(&multifd_send_state->channels_ready);
@@ -1090,7 +1093,7 @@ static void *multifd_recv_thread(void *opaque)
rcu_register_thread();
while (true) {
- uint32_t flags;
+ bool sync_needed = false;
if (p->quit) {
break;
@@ -1112,11 +1115,11 @@ static void *multifd_recv_thread(void *opaque)
break;
}
- flags = p->flags;
+ trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->flags,
+ p->next_packet_size);
+ sync_needed = p->flags & MULTIFD_FLAG_SYNC;
/* recv methods don't know how to handle the SYNC flag */
p->flags &= ~MULTIFD_FLAG_SYNC;
- trace_multifd_recv(p->id, p->packet_num, p->normal_num, flags,
- p->next_packet_size);
p->num_packets++;
p->total_normal_pages += p->normal_num;
qemu_mutex_unlock(&p->mutex);
@@ -1128,7 +1131,7 @@ static void *multifd_recv_thread(void *opaque)
}
}
- if (flags & MULTIFD_FLAG_SYNC) {
+ if (sync_needed) {
qemu_sem_post(&multifd_recv_state->sem_sync);
qemu_sem_wait(&p->sem_sync);
}
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 06/12] multifd: Make flags field thread local
2022-08-02 6:39 ` [PATCH v7 06/12] multifd: Make flags field thread local Juan Quintela
@ 2022-08-11 9:04 ` Leonardo Brás
2022-08-19 10:03 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 9:04 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> Use of flags with respect to locking was incensistant. For the
> sending side:
> - it was set to 0 with mutex held on the multifd channel.
> - MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
> - Everything else was done without the mutex held on the multifd channel.
>
> On the reception side, it is not used on the migration thread, only on
> the multifd channels threads.
>
> So we move it to the multifd channels thread only variables, and we
> introduce a new bool sync_needed on the send side to pass that information.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/multifd.h | 10 ++++++----
> migration/multifd.c | 23 +++++++++++++----------
> 2 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 36f899c56f..a67cefc0a2 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -98,12 +98,12 @@ typedef struct {
Just noticed having no name in 'typedef struct' line makes it harder to
understand what is going on.
MultiFDSendParams
> bool running;
> /* should this thread finish */
> bool quit;
> - /* multifd flags for each packet */
> - uint32_t flags;
> /* global number of generated multifd packets */
> uint64_t packet_num;
> /* How many bytes have we sent on the last packet */
> uint64_t sent_bytes;
> + /* Do we need to do an iteration sync */
> + bool sync_needed;
> /* thread has work to do */
> int pending_job;
> /* array of pages to sent.
> @@ -117,6 +117,8 @@ typedef struct {
>
> /* pointer to the packet */
> MultiFDPacket_t *packet;
> + /* multifd flags for each packet */
> + uint32_t flags;
> /* size of the next packet that contains pages */
> uint32_t next_packet_size;
> /* packets sent through this channel */
MultiFDRecvParams
> @@ -163,8 +165,6 @@ typedef struct {
> bool running;
> /* should this thread finish */
> bool quit;
> - /* multifd flags for each packet */
> - uint32_t flags;
> /* global number of generated multifd packets */
> uint64_t packet_num;
>
> @@ -172,6 +172,8 @@ typedef struct {
>
> /* pointer to the packet */
> MultiFDPacket_t *packet;
> + /* multifd flags for each packet */
> + uint32_t flags;
> /* size of the next packet that contains pages */
> uint32_t next_packet_size;
> /* packets sent through this channel */
So, IIUC, the struct member flags got moved down (same struct) to an area
described as thread-local, meaning it does not need locking.
Interesting, I haven't noticed this different areas in the same struct.
> diff --git a/migration/multifd.c b/migration/multifd.c
> index e25b529235..09a40a9135 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -602,7 +602,7 @@ int multifd_send_sync_main(QEMUFile *f)
> }
>
> p->packet_num = multifd_send_state->packet_num++;
> - p->flags |= MULTIFD_FLAG_SYNC;
> + p->sync_needed = true;
> p->pending_job++;
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_post(&p->sem);
> @@ -658,7 +658,11 @@ static void *multifd_send_thread(void *opaque)
>
> if (p->pending_job) {
> uint64_t packet_num = p->packet_num;
> - uint32_t flags = p->flags;
> + p->flags = 0;
> + if (p->sync_needed) {
> + p->flags |= MULTIFD_FLAG_SYNC;
> + p->sync_needed = false;
> + }
Any particular reason why doing p->flags = 0, then p->flags |= MULTIFD_FLAG_SYNC
?
[1] Couldn't it be done without the |= , since it's already being set to zero
before? (becoming "p->flags = MULTIFD_FLAG_SYNC" )
> p->normal_num = 0;
>
> if (use_zero_copy_send) {
> @@ -680,14 +684,13 @@ static void *multifd_send_thread(void *opaque)
> }
> }
> multifd_send_fill_packet(p);
> - p->flags = 0;
> p->num_packets++;
> p->total_normal_pages += p->normal_num;
> p->pages->num = 0;
> p->pages->block = NULL;
> qemu_mutex_unlock(&p->mutex);
>
> - trace_multifd_send(p->id, packet_num, p->normal_num, flags,
> + trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> p->next_packet_size);
>
> if (use_zero_copy_send) {
> @@ -715,7 +718,7 @@ static void *multifd_send_thread(void *opaque)
> p->pending_job--;
> qemu_mutex_unlock(&p->mutex);
>
> - if (flags & MULTIFD_FLAG_SYNC) {
> + if (p->flags & MULTIFD_FLAG_SYNC) {
> qemu_sem_post(&p->sem_sync);
> }
> qemu_sem_post(&multifd_send_state->channels_ready);
IIUC it uses p->sync_needed to keep the sync info, instead of the previous flags
local var, and thus it can set p->flags = 0 earlier. Seems to not change any
behavior AFAICS.
> @@ -1090,7 +1093,7 @@ static void *multifd_recv_thread(void *opaque)
> rcu_register_thread();
>
> while (true) {
> - uint32_t flags;
> + bool sync_needed = false;
>
> if (p->quit) {
> break;
> @@ -1112,11 +1115,11 @@ static void *multifd_recv_thread(void *opaque)
> break;
> }
>
> - flags = p->flags;
> + trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->flags,
> + p->next_packet_size);
> + sync_needed = p->flags & MULTIFD_FLAG_SYNC;
> /* recv methods don't know how to handle the SYNC flag */
> p->flags &= ~MULTIFD_FLAG_SYNC;
> - trace_multifd_recv(p->id, p->packet_num, p->normal_num, flags,
> - p->next_packet_size);
> p->num_packets++;
> p->total_normal_pages += p->normal_num;
> qemu_mutex_unlock(&p->mutex);
> @@ -1128,7 +1131,7 @@ static void *multifd_recv_thread(void *opaque)
> }
> }
>
> - if (flags & MULTIFD_FLAG_SYNC) {
> + if (sync_needed) {
> qemu_sem_post(&multifd_recv_state->sem_sync);
> qemu_sem_wait(&p->sem_sync);
> }
Ok, IIUC this part should have the same behavior as before, but using a bool
instead of an u32.
Other than question [1], LGTM.
FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 06/12] multifd: Make flags field thread local
2022-08-11 9:04 ` Leonardo Brás
@ 2022-08-19 10:03 ` Juan Quintela
2022-08-20 7:24 ` Leonardo Bras Soares Passos
0 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-19 10:03 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> Use of flags with respect to locking was incensistant. For the
>> sending side:
>> - it was set to 0 with mutex held on the multifd channel.
>> - MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
>> - Everything else was done without the mutex held on the multifd channel.
>>
>> On the reception side, it is not used on the migration thread, only on
>> the multifd channels threads.
>>
>> So we move it to the multifd channels thread only variables, and we
>> introduce a new bool sync_needed on the send side to pass that information.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/multifd.h | 10 ++++++----
>> migration/multifd.c | 23 +++++++++++++----------
>> 2 files changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index 36f899c56f..a67cefc0a2 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -98,12 +98,12 @@ typedef struct {
>
> Just noticed having no name in 'typedef struct' line makes it harder to
> understand what is going on.
It is common idiom in QEMU. The principal reason is that if you don't
want anyone to use "struct MultiFDSendParams" but MultiFDSendParams, the
best way to achieve that is to do it this way.
>> @@ -172,6 +172,8 @@ typedef struct {
>>
>> /* pointer to the packet */
>> MultiFDPacket_t *packet;
>> + /* multifd flags for each packet */
>> + uint32_t flags;
>> /* size of the next packet that contains pages */
>> uint32_t next_packet_size;
>> /* packets sent through this channel */
>
> So, IIUC, the struct member flags got moved down (same struct) to an area
> described as thread-local, meaning it does not need locking.
>
> Interesting, I haven't noticed this different areas in the same struct.
It has changed in the last two weeks or so in upstream (it has been on
this patchset for several months.)
>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index e25b529235..09a40a9135 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -602,7 +602,7 @@ int multifd_send_sync_main(QEMUFile *f)
>> }
>>
>> p->packet_num = multifd_send_state->packet_num++;
>> - p->flags |= MULTIFD_FLAG_SYNC;
>> + p->sync_needed = true;
>> p->pending_job++;
>> qemu_mutex_unlock(&p->mutex);
>> qemu_sem_post(&p->sem);
>> @@ -658,7 +658,11 @@ static void *multifd_send_thread(void *opaque)
>>
>> if (p->pending_job) {
>> uint64_t packet_num = p->packet_num;
>> - uint32_t flags = p->flags;
>> + p->flags = 0;
>> + if (p->sync_needed) {
>> + p->flags |= MULTIFD_FLAG_SYNC;
>> + p->sync_needed = false;
>> + }
>
> Any particular reason why doing p->flags = 0, then p->flags |= MULTIFD_FLAG_SYNC
> ?
It is a bitmap field, and if there is anything on the future, we need to
set it. I agree that when there is only one flag, it seems "weird".
> [1] Couldn't it be done without the |= , since it's already being set to zero
> before? (becoming "p->flags = MULTIFD_FLAG_SYNC" )
As said, easier to modify later, and also easier if we want to setup a
flag by default.
I agree that it is a matter of style/taste.
>> p->normal_num = 0;
>>
>> if (use_zero_copy_send) {
>> @@ -680,14 +684,13 @@ static void *multifd_send_thread(void *opaque)
>> }
>> }
>> multifd_send_fill_packet(p);
>> - p->flags = 0;
>> p->num_packets++;
>> p->total_normal_pages += p->normal_num;
>> p->pages->num = 0;
>> p->pages->block = NULL;
>> qemu_mutex_unlock(&p->mutex);
>>
>> - trace_multifd_send(p->id, packet_num, p->normal_num, flags,
>> + trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
>> p->next_packet_size);
>>
>> if (use_zero_copy_send) {
>> @@ -715,7 +718,7 @@ static void *multifd_send_thread(void *opaque)
>> p->pending_job--;
>> qemu_mutex_unlock(&p->mutex);
>>
>> - if (flags & MULTIFD_FLAG_SYNC) {
>> + if (p->flags & MULTIFD_FLAG_SYNC) {
>> qemu_sem_post(&p->sem_sync);
>> }
>> qemu_sem_post(&multifd_send_state->channels_ready);
>
> IIUC it uses p->sync_needed to keep the sync info, instead of the previous flags
> local var, and thus it can set p->flags = 0 earlier. Seems to not change any
> behavior AFAICS.
The protection of the global flags was being wrong. That is the reason
that I decided to change it to the sync_needed.
The problem was that at some point we were still sending a packet (that
shouldn't have the SYNC flag enabled), but we received a
multifd_main_sync() and it got enabled anyways. The easier way that I
found te fix it was this way.
Problem was difficult to detect, that is the reason that I change it
this way.
>> - if (flags & MULTIFD_FLAG_SYNC) {
>> + if (sync_needed) {
>> qemu_sem_post(&multifd_recv_state->sem_sync);
>> qemu_sem_wait(&p->sem_sync);
>> }
>
> Ok, IIUC this part should have the same behavior as before, but using a bool
> instead of an u32.
I changed it to make sure that we only checked the flags at the
beggining of the function, with the lock taken.
>
> FWIW:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 06/12] multifd: Make flags field thread local
2022-08-19 10:03 ` Juan Quintela
@ 2022-08-20 7:24 ` Leonardo Bras Soares Passos
2022-08-23 13:00 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Bras Soares Passos @ 2022-08-20 7:24 UTC (permalink / raw)
To: Juan Quintela
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
On Fri, Aug 19, 2022 at 7:03 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> Use of flags with respect to locking was incensistant. For the
> >> sending side:
> >> - it was set to 0 with mutex held on the multifd channel.
> >> - MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
> >> - Everything else was done without the mutex held on the multifd channel.
> >>
> >> On the reception side, it is not used on the migration thread, only on
> >> the multifd channels threads.
> >>
> >> So we move it to the multifd channels thread only variables, and we
> >> introduce a new bool sync_needed on the send side to pass that information.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >> migration/multifd.h | 10 ++++++----
> >> migration/multifd.c | 23 +++++++++++++----------
> >> 2 files changed, 19 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/migration/multifd.h b/migration/multifd.h
> >> index 36f899c56f..a67cefc0a2 100644
> >> --- a/migration/multifd.h
> >> +++ b/migration/multifd.h
> >> @@ -98,12 +98,12 @@ typedef struct {
> >
> > Just noticed having no name in 'typedef struct' line makes it harder to
> > understand what is going on.
>
> It is common idiom in QEMU. The principal reason is that if you don't
> want anyone to use "struct MultiFDSendParams" but MultiFDSendParams, the
> best way to achieve that is to do it this way.
I agree, but a comment after the typedef could help reviewing. Something like
typedef struct { /* MultiFDSendParams */
...
} MultiFDSendParams
Becomes this in diff:
diff --git a/migration/multifd.h b/migration/multifd.h
index 134e6a7f19..93bb3a7f4a 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -90,6 +90,7 @@ typedef struct { /* MultiFDSendParams */
[...]
>
> >> @@ -172,6 +172,8 @@ typedef struct {
> >>
> >> /* pointer to the packet */
> >> MultiFDPacket_t *packet;
> >> + /* multifd flags for each packet */
> >> + uint32_t flags;
> >> /* size of the next packet that contains pages */
> >> uint32_t next_packet_size;
> >> /* packets sent through this channel */
> >
> > So, IIUC, the struct member flags got moved down (same struct) to an area
> > described as thread-local, meaning it does not need locking.
> >
> > Interesting, I haven't noticed this different areas in the same struct.
>
> It has changed in the last two weeks or so in upstream (it has been on
> this patchset for several months.)
Nice :)
>
>
> >
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index e25b529235..09a40a9135 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -602,7 +602,7 @@ int multifd_send_sync_main(QEMUFile *f)
> >> }
> >>
> >> p->packet_num = multifd_send_state->packet_num++;
> >> - p->flags |= MULTIFD_FLAG_SYNC;
> >> + p->sync_needed = true;
> >> p->pending_job++;
> >> qemu_mutex_unlock(&p->mutex);
> >> qemu_sem_post(&p->sem);
> >> @@ -658,7 +658,11 @@ static void *multifd_send_thread(void *opaque)
> >>
> >> if (p->pending_job) {
> >> uint64_t packet_num = p->packet_num;
> >> - uint32_t flags = p->flags;
> >> + p->flags = 0;
> >> + if (p->sync_needed) {
> >> + p->flags |= MULTIFD_FLAG_SYNC;
> >> + p->sync_needed = false;
> >> + }
> >
> > Any particular reason why doing p->flags = 0, then p->flags |= MULTIFD_FLAG_SYNC
> > ?
>
> It is a bitmap field, and if there is anything on the future, we need to
> set it. I agree that when there is only one flag, it seems "weird".
>
> > [1] Couldn't it be done without the |= , since it's already being set to zero
> > before? (becoming "p->flags = MULTIFD_FLAG_SYNC" )
>
> As said, easier to modify later, and also easier if we want to setup a
> flag by default.
Yeah, I agree. It makes sense now.
Thanks
>
> I agree that it is a matter of style/taste.
>
> >> p->normal_num = 0;
> >>
> >> if (use_zero_copy_send) {
> >> @@ -680,14 +684,13 @@ static void *multifd_send_thread(void *opaque)
> >> }
> >> }
> >> multifd_send_fill_packet(p);
> >> - p->flags = 0;
> >> p->num_packets++;
> >> p->total_normal_pages += p->normal_num;
> >> p->pages->num = 0;
> >> p->pages->block = NULL;
> >> qemu_mutex_unlock(&p->mutex);
> >>
> >> - trace_multifd_send(p->id, packet_num, p->normal_num, flags,
> >> + trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> >> p->next_packet_size);
> >>
> >> if (use_zero_copy_send) {
> >> @@ -715,7 +718,7 @@ static void *multifd_send_thread(void *opaque)
> >> p->pending_job--;
> >> qemu_mutex_unlock(&p->mutex);
> >>
> >> - if (flags & MULTIFD_FLAG_SYNC) {
> >> + if (p->flags & MULTIFD_FLAG_SYNC) {
> >> qemu_sem_post(&p->sem_sync);
> >> }
> >> qemu_sem_post(&multifd_send_state->channels_ready);
> >
> > IIUC it uses p->sync_needed to keep the sync info, instead of the previous flags
> > local var, and thus it can set p->flags = 0 earlier. Seems to not change any
> > behavior AFAICS.
>
> The protection of the global flags was being wrong. That is the reason
> that I decided to change it to the sync_needed.
>
> The problem was that at some point we were still sending a packet (that
> shouldn't have the SYNC flag enabled), but we received a
> multifd_main_sync() and it got enabled anyways. The easier way that I
> found te fix it was this way.
>
> Problem was difficult to detect, that is the reason that I change it
> this way.
Oh, I see.
>
> >> - if (flags & MULTIFD_FLAG_SYNC) {
> >> + if (sync_needed) {
> >> qemu_sem_post(&multifd_recv_state->sem_sync);
> >> qemu_sem_wait(&p->sem_sync);
> >> }
> >
> > Ok, IIUC this part should have the same behavior as before, but using a bool
> > instead of an u32.
>
> I changed it to make sure that we only checked the flags at the
> beggining of the function, with the lock taken.
Thanks for sharing!
Best regards,
Leo
>
> >
> > FWIW:
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
>
> Thanks, Juan.
>
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 06/12] multifd: Make flags field thread local
2022-08-20 7:24 ` Leonardo Bras Soares Passos
@ 2022-08-23 13:00 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-23 13:00 UTC (permalink / raw)
To: Leonardo Bras Soares Passos
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Bras Soares Passos <leobras@redhat.com> wrote:
> On Fri, Aug 19, 2022 at 7:03 AM Juan Quintela <quintela@redhat.com> wrote:
>>
>> Leonardo Brás <leobras@redhat.com> wrote:
>> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> >> Use of flags with respect to locking was incensistant. For the
>> >> sending side:
>> >> - it was set to 0 with mutex held on the multifd channel.
>> >> - MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
>> >> - Everything else was done without the mutex held on the multifd channel.
>> >>
>> >> On the reception side, it is not used on the migration thread, only on
>> >> the multifd channels threads.
>> >>
>> >> So we move it to the multifd channels thread only variables, and we
>> >> introduce a new bool sync_needed on the send side to pass that information.
>> >>
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >> migration/multifd.h | 10 ++++++----
>> >> migration/multifd.c | 23 +++++++++++++----------
>> >> 2 files changed, 19 insertions(+), 14 deletions(-)
>> >>
>> >> diff --git a/migration/multifd.h b/migration/multifd.h
>> >> index 36f899c56f..a67cefc0a2 100644
>> >> --- a/migration/multifd.h
>> >> +++ b/migration/multifd.h
>> >> @@ -98,12 +98,12 @@ typedef struct {
>> >
>> > Just noticed having no name in 'typedef struct' line makes it harder to
>> > understand what is going on.
>>
>> It is common idiom in QEMU. The principal reason is that if you don't
>> want anyone to use "struct MultiFDSendParams" but MultiFDSendParams, the
>> best way to achieve that is to do it this way.
>
> I agree, but a comment after the typedef could help reviewing. Something like
>
> typedef struct { /* MultiFDSendParams */
> ...
> } MultiFDSendParams
You have a point here. Not putting a comment, putting the real thing.
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (5 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 06/12] multifd: Make flags field thread local Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-08-11 9:16 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page Juan Quintela
` (4 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
We do the send_prepare() and the fill of the head packet without the
mutex held. It will help a lot for compression and later in the
series for zero pages.
Notice that we can use p->pages without holding p->mutex because
p->pending_job == 1.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd.h | 2 ++
migration/multifd.c | 11 ++++++-----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index a67cefc0a2..cd389d18d2 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -109,7 +109,9 @@ typedef struct {
/* array of pages to sent.
* The owner of 'pages' depends of 'pending_job' value:
* pending_job == 0 -> migration_thread can use it.
+ * No need for mutex lock.
* pending_job != 0 -> multifd_channel can use it.
+ * No need for mutex lock.
*/
MultiFDPages_t *pages;
diff --git a/migration/multifd.c b/migration/multifd.c
index 09a40a9135..68fc9f8e88 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -663,6 +663,8 @@ static void *multifd_send_thread(void *opaque)
p->flags |= MULTIFD_FLAG_SYNC;
p->sync_needed = false;
}
+ qemu_mutex_unlock(&p->mutex);
+
p->normal_num = 0;
if (use_zero_copy_send) {
@@ -684,11 +686,6 @@ static void *multifd_send_thread(void *opaque)
}
}
multifd_send_fill_packet(p);
- p->num_packets++;
- p->total_normal_pages += p->normal_num;
- p->pages->num = 0;
- p->pages->block = NULL;
- qemu_mutex_unlock(&p->mutex);
trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
p->next_packet_size);
@@ -713,6 +710,10 @@ static void *multifd_send_thread(void *opaque)
}
qemu_mutex_lock(&p->mutex);
+ p->num_packets++;
+ p->total_normal_pages += p->normal_num;
+ p->pages->num = 0;
+ p->pages->block = NULL;
p->sent_bytes += p->packet_len;;
p->sent_bytes += p->next_packet_size;
p->pending_job--;
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held
2022-08-02 6:39 ` [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held Juan Quintela
@ 2022-08-11 9:16 ` Leonardo Brás
2022-08-19 11:32 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 9:16 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> We do the send_prepare() and the fill of the head packet without the
> mutex held. It will help a lot for compression and later in the
> series for zero pages.
>
> Notice that we can use p->pages without holding p->mutex because
> p->pending_job == 1.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/multifd.h | 2 ++
> migration/multifd.c | 11 ++++++-----
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index a67cefc0a2..cd389d18d2 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -109,7 +109,9 @@ typedef struct {
> /* array of pages to sent.
> * The owner of 'pages' depends of 'pending_job' value:
> * pending_job == 0 -> migration_thread can use it.
> + * No need for mutex lock.
> * pending_job != 0 -> multifd_channel can use it.
> + * No need for mutex lock.
> */
> MultiFDPages_t *pages;
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 09a40a9135..68fc9f8e88 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -663,6 +663,8 @@ static void *multifd_send_thread(void *opaque)
> p->flags |= MULTIFD_FLAG_SYNC;
> p->sync_needed = false;
> }
> + qemu_mutex_unlock(&p->mutex);
> +
If it unlocks here, we will have unprotected:
for (int i = 0; i < p->pages->num; i++) {
p->normal[p->normal_num] = p->pages->offset[i];
p->normal_num++;
}
And p->pages seems to be in the mutex-protected area.
Should it be ok?
Also, under that we have:
if (p->normal_num) {
ret = multifd_send_state->ops->send_prepare(p, &local_err);
if (ret != 0) {
qemu_mutex_unlock(&p->mutex);
break;
}
}
Calling mutex_unlock() here, even though the unlock already happened before,
could cause any issue?
> p->normal_num = 0;
>
> if (use_zero_copy_send) {
> @@ -684,11 +686,6 @@ static void *multifd_send_thread(void *opaque)
> }
> }
> multifd_send_fill_packet(p);
> - p->num_packets++;
> - p->total_normal_pages += p->normal_num;
> - p->pages->num = 0;
> - p->pages->block = NULL;
> - qemu_mutex_unlock(&p->mutex);
>
> trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> p->next_packet_size);
> @@ -713,6 +710,10 @@ static void *multifd_send_thread(void *opaque)
> }
>
> qemu_mutex_lock(&p->mutex);
> + p->num_packets++;
> + p->total_normal_pages += p->normal_num;
> + p->pages->num = 0;
> + p->pages->block = NULL;
> p->sent_bytes += p->packet_len;;
> p->sent_bytes += p->next_packet_size;
> p->pending_job--;
Not used in the interval, this part seems ok.
Best regards,
Leo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held
2022-08-11 9:16 ` Leonardo Brás
@ 2022-08-19 11:32 ` Juan Quintela
2022-08-20 7:27 ` Leonardo Bras Soares Passos
0 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-19 11:32 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> We do the send_prepare() and the fill of the head packet without the
>> mutex held. It will help a lot for compression and later in the
>> series for zero pages.
>>
>> Notice that we can use p->pages without holding p->mutex because
>> p->pending_job == 1.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/multifd.h | 2 ++
>> migration/multifd.c | 11 ++++++-----
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index a67cefc0a2..cd389d18d2 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -109,7 +109,9 @@ typedef struct {
>> /* array of pages to sent.
>> * The owner of 'pages' depends of 'pending_job' value:
>> * pending_job == 0 -> migration_thread can use it.
>> + * No need for mutex lock.
>> * pending_job != 0 -> multifd_channel can use it.
>> + * No need for mutex lock.
>> */
>> MultiFDPages_t *pages;
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 09a40a9135..68fc9f8e88 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -663,6 +663,8 @@ static void *multifd_send_thread(void *opaque)
>> p->flags |= MULTIFD_FLAG_SYNC;
>> p->sync_needed = false;
>> }
>> + qemu_mutex_unlock(&p->mutex);
>> +
>
> If it unlocks here, we will have unprotected:
> for (int i = 0; i < p->pages->num; i++) {
> p->normal[p->normal_num] = p->pages->offset[i];
> p->normal_num++;
> }
>
> And p->pages seems to be in the mutex-protected area.
> Should it be ok?
From the documentation:
/* array of pages to sent.
* The owner of 'pages' depends of 'pending_job' value:
* pending_job == 0 -> migration_thread can use it.
* No need for mutex lock.
* pending_job != 0 -> multifd_channel can use it.
* No need for mutex lock.
*/
MultiFDPages_t *pages;
So, it is right.
> Also, under that we have:
> if (p->normal_num) {
> ret = multifd_send_state->ops->send_prepare(p, &local_err);
> if (ret != 0) {
> qemu_mutex_unlock(&p->mutex);
> break;
> }
> }
>
> Calling mutex_unlock() here, even though the unlock already happened before,
> could cause any issue?
Good catch. Never got an error there.
Removing that bit.
> Best regards,
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held
2022-08-19 11:32 ` Juan Quintela
@ 2022-08-20 7:27 ` Leonardo Bras Soares Passos
0 siblings, 0 replies; 43+ messages in thread
From: Leonardo Bras Soares Passos @ 2022-08-20 7:27 UTC (permalink / raw)
To: Juan Quintela
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
On Fri, Aug 19, 2022 at 8:32 AM Juan Quintela <quintela@redhat.com> wrote:
>
> Leonardo Brás <leobras@redhat.com> wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> We do the send_prepare() and the fill of the head packet without the
> >> mutex held. It will help a lot for compression and later in the
> >> series for zero pages.
> >>
> >> Notice that we can use p->pages without holding p->mutex because
> >> p->pending_job == 1.
> >>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >> migration/multifd.h | 2 ++
> >> migration/multifd.c | 11 ++++++-----
> >> 2 files changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/migration/multifd.h b/migration/multifd.h
> >> index a67cefc0a2..cd389d18d2 100644
> >> --- a/migration/multifd.h
> >> +++ b/migration/multifd.h
> >> @@ -109,7 +109,9 @@ typedef struct {
> >> /* array of pages to sent.
> >> * The owner of 'pages' depends of 'pending_job' value:
> >> * pending_job == 0 -> migration_thread can use it.
> >> + * No need for mutex lock.
> >> * pending_job != 0 -> multifd_channel can use it.
> >> + * No need for mutex lock.
> >> */
> >> MultiFDPages_t *pages;
> >>
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index 09a40a9135..68fc9f8e88 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -663,6 +663,8 @@ static void *multifd_send_thread(void *opaque)
> >> p->flags |= MULTIFD_FLAG_SYNC;
> >> p->sync_needed = false;
> >> }
> >> + qemu_mutex_unlock(&p->mutex);
> >> +
> >
> > If it unlocks here, we will have unprotected:
> > for (int i = 0; i < p->pages->num; i++) {
> > p->normal[p->normal_num] = p->pages->offset[i];
> > p->normal_num++;
> > }
> >
> > And p->pages seems to be in the mutex-protected area.
> > Should it be ok?
>
> From the documentation:
>
> /* array of pages to sent.
> * The owner of 'pages' depends of 'pending_job' value:
> * pending_job == 0 -> migration_thread can use it.
> * No need for mutex lock.
> * pending_job != 0 -> multifd_channel can use it.
> * No need for mutex lock.
> */
> MultiFDPages_t *pages;
>
> So, it is right.
Oh, right. I missed that part earlier .
>
> > Also, under that we have:
> > if (p->normal_num) {
> > ret = multifd_send_state->ops->send_prepare(p, &local_err);
> > if (ret != 0) {
> > qemu_mutex_unlock(&p->mutex);
> > break;
> > }
> > }
> >
> > Calling mutex_unlock() here, even though the unlock already happened before,
> > could cause any issue?
>
> Good catch. Never got an error there.
>
> Removing that bit.
Thanks!
Best regards,
Leo
>
> > Best regards,
>
>
> Thanks, Juan.
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (6 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 07/12] multifd: Prepare to send a packet without the mutex held Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-08-11 9:29 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 09/12] migration: Export ram_release_page() Juan Quintela
` (3 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
We have to enable it by default until we introduce the new code.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
Change it to a capability. As capabilities are off by default, have
to change MULTIFD_ZERO_PAGE to MAIN_ZERO_PAGE, so it is false for
default, and true for older versions.
---
qapi/migration.json | 8 +++++++-
migration/migration.h | 1 +
hw/core/machine.c | 1 +
migration/migration.c | 16 +++++++++++++++-
4 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 81185d4311..dc981236ff 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -472,12 +472,18 @@
# Requires that QEMU be permitted to use locked memory
# for guest RAM pages.
# (since 7.1)
+#
# @postcopy-preempt: If enabled, the migration process will allow postcopy
# requests to preempt precopy stream, so postcopy requests
# will be handled faster. This is a performance feature and
# should not affect the correctness of postcopy migration.
# (since 7.1)
#
+# @main-zero-page: If enabled, the detection of zero pages will be
+# done on the main thread. Otherwise it is done on
+# the multifd threads.
+# (since 7.1)
+#
# Features:
# @unstable: Members @x-colo and @x-ignore-shared are experimental.
#
@@ -492,7 +498,7 @@
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
{ 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
'validate-uuid', 'background-snapshot',
- 'zero-copy-send', 'postcopy-preempt'] }
+ 'zero-copy-send', 'postcopy-preempt', 'main-zero-page'] }
##
# @MigrationCapabilityStatus:
diff --git a/migration/migration.h b/migration/migration.h
index cdad8aceaa..58b245b138 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -415,6 +415,7 @@ int migrate_multifd_channels(void);
MultiFDCompression migrate_multifd_compression(void);
int migrate_multifd_zlib_level(void);
int migrate_multifd_zstd_level(void);
+bool migrate_use_main_zero_page(void);
#ifdef CONFIG_LINUX
bool migrate_use_zero_copy_send(void);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a673302cce..2624b75ab4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -43,6 +43,7 @@
GlobalProperty hw_compat_7_0[] = {
{ "arm-gicv3-common", "force-8-bit-prio", "on" },
{ "nvme-ns", "eui64-default", "on"},
+ { "migration", "main-zero-page", "true" },
};
const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0);
diff --git a/migration/migration.c b/migration/migration.c
index e03f698a3c..ce3e5cc0cd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -164,7 +164,8 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
MIGRATION_CAPABILITY_XBZRLE,
MIGRATION_CAPABILITY_X_COLO,
MIGRATION_CAPABILITY_VALIDATE_UUID,
- MIGRATION_CAPABILITY_ZERO_COPY_SEND);
+ MIGRATION_CAPABILITY_ZERO_COPY_SEND,
+ MIGRATION_CAPABILITY_MAIN_ZERO_PAGE);
/* When we add fault tolerance, we could have several
migrations at once. For now we don't need to add
@@ -2592,6 +2593,17 @@ bool migrate_use_multifd(void)
return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD];
}
+bool migrate_use_main_zero_page(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ // We will enable this when we add the right code.
+ // return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
+ return true;
+}
+
bool migrate_pause_before_switchover(void)
{
MigrationState *s;
@@ -4406,6 +4418,8 @@ static Property migration_properties[] = {
DEFINE_PROP_MIG_CAP("x-zero-copy-send",
MIGRATION_CAPABILITY_ZERO_COPY_SEND),
#endif
+ DEFINE_PROP_MIG_CAP("main-zero-page",
+ MIGRATION_CAPABILITY_MAIN_ZERO_PAGE),
DEFINE_PROP_END_OF_LIST(),
};
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page
2022-08-02 6:39 ` [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page Juan Quintela
@ 2022-08-11 9:29 ` Leonardo Brás
2022-08-19 11:36 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 9:29 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> We have to enable it by default until we introduce the new code.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> Change it to a capability. As capabilities are off by default, have
> to change MULTIFD_ZERO_PAGE to MAIN_ZERO_PAGE, so it is false for
> default, and true for older versions.
IIUC, the idea of a capability is to introduce some new features to the code,
and let users enable or disable it.
If it introduce a new capability, is not very intuitive to think that it will be
always true for older versions, and false for new ones.
I would suggest adding it as MULTIFD_ZERO_PAGE, and let it disabled for now.
When the full feature gets introduced, the capability could be enabled by
default, if desired.
What do you think?
Best regards,
Leo
> ---
> qapi/migration.json | 8 +++++++-
> migration/migration.h | 1 +
> hw/core/machine.c | 1 +
> migration/migration.c | 16 +++++++++++++++-
> 4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 81185d4311..dc981236ff 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -472,12 +472,18 @@
> # Requires that QEMU be permitted to use locked memory
> # for guest RAM pages.
> # (since 7.1)
> +#
> # @postcopy-preempt: If enabled, the migration process will allow postcopy
> # requests to preempt precopy stream, so postcopy requests
> # will be handled faster. This is a performance feature and
> # should not affect the correctness of postcopy migration.
> # (since 7.1)
> #
> +# @main-zero-page: If enabled, the detection of zero pages will be
> +# done on the main thread. Otherwise it is done on
> +# the multifd threads.
> +# (since 7.1)
> +#
> # Features:
> # @unstable: Members @x-colo and @x-ignore-shared are experimental.
> #
> @@ -492,7 +498,7 @@
> 'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
> 'validate-uuid', 'background-snapshot',
> - 'zero-copy-send', 'postcopy-preempt'] }
> + 'zero-copy-send', 'postcopy-preempt', 'main-zero-page'] }
>
> ##
> # @MigrationCapabilityStatus:
> diff --git a/migration/migration.h b/migration/migration.h
> index cdad8aceaa..58b245b138 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -415,6 +415,7 @@ int migrate_multifd_channels(void);
> MultiFDCompression migrate_multifd_compression(void);
> int migrate_multifd_zlib_level(void);
> int migrate_multifd_zstd_level(void);
> +bool migrate_use_main_zero_page(void);
>
> #ifdef CONFIG_LINUX
> bool migrate_use_zero_copy_send(void);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a673302cce..2624b75ab4 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -43,6 +43,7 @@
> GlobalProperty hw_compat_7_0[] = {
> { "arm-gicv3-common", "force-8-bit-prio", "on" },
> { "nvme-ns", "eui64-default", "on"},
> + { "migration", "main-zero-page", "true" },
> };
> const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0);
>
> diff --git a/migration/migration.c b/migration/migration.c
> index e03f698a3c..ce3e5cc0cd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -164,7 +164,8 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
> MIGRATION_CAPABILITY_XBZRLE,
> MIGRATION_CAPABILITY_X_COLO,
> MIGRATION_CAPABILITY_VALIDATE_UUID,
> - MIGRATION_CAPABILITY_ZERO_COPY_SEND);
> + MIGRATION_CAPABILITY_ZERO_COPY_SEND,
> + MIGRATION_CAPABILITY_MAIN_ZERO_PAGE);
>
> /* When we add fault tolerance, we could have several
> migrations at once. For now we don't need to add
> @@ -2592,6 +2593,17 @@ bool migrate_use_multifd(void)
> return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD];
> }
>
> +bool migrate_use_main_zero_page(void)
> +{
> + MigrationState *s;
> +
> + s = migrate_get_current();
> +
> + // We will enable this when we add the right code.
> + // return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
> + return true;
> +}
> +
> bool migrate_pause_before_switchover(void)
> {
> MigrationState *s;
> @@ -4406,6 +4418,8 @@ static Property migration_properties[] = {
> DEFINE_PROP_MIG_CAP("x-zero-copy-send",
> MIGRATION_CAPABILITY_ZERO_COPY_SEND),
> #endif
> + DEFINE_PROP_MIG_CAP("main-zero-page",
> + MIGRATION_CAPABILITY_MAIN_ZERO_PAGE),
>
> DEFINE_PROP_END_OF_LIST(),
> };
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page
2022-08-11 9:29 ` Leonardo Brás
@ 2022-08-19 11:36 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-19 11:36 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> We have to enable it by default until we introduce the new code.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> ---
>>
>> Change it to a capability. As capabilities are off by default, have
>> to change MULTIFD_ZERO_PAGE to MAIN_ZERO_PAGE, so it is false for
>> default, and true for older versions.
>
> IIUC, the idea of a capability is to introduce some new features to the code,
> and let users enable or disable it.
All capabilities are false by default.
If we change the capability to be true by default, we need to teach
libvirt new tricks.
> If it introduce a new capability, is not very intuitive to think that it will be
> always true for older versions, and false for new ones.
It don't need to be intuitive, it just need to be documented correctly.
I think that is done, no?
> I would suggest adding it as MULTIFD_ZERO_PAGE, and let it disabled for now.
> When the full feature gets introduced, the capability could be enabled by
> default, if desired.
I have it that way before it was a capability.
but the info migrate_capabilities
showed everything false and this one true, wondering _why_ this one is
true.
So I decided to rename it, and make it true by default.
> What do you think?
I preffer it this way, why?
Because at some point in the future, we will remove the code that
implements the capability and the capability. So the idea is that we
don't want to use it for old code.
Later, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 09/12] migration: Export ram_release_page()
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (7 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-08-11 9:31 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 10/12] multifd: Support for zero pages transmission Juan Quintela
` (2 subsequent siblings)
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.h | 1 +
migration/ram.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/migration/ram.h b/migration/ram.h
index e844966f69..038d52f49f 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -66,6 +66,7 @@ int ram_load_postcopy(QEMUFile *f, int channel);
void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
void ram_transferred_add(uint64_t bytes);
+void ram_release_page(const char *rbname, uint64_t offset);
int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
bool ramblock_recv_bitmap_test_byte_offset(RAMBlock *rb, uint64_t byte_offset);
diff --git a/migration/ram.c b/migration/ram.c
index 499d9b2a90..291ba5c0ed 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1238,7 +1238,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
}
}
-static void ram_release_page(const char *rbname, uint64_t offset)
+void ram_release_page(const char *rbname, uint64_t offset)
{
if (!migrate_release_ram() || !migration_in_postcopy()) {
return;
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 09/12] migration: Export ram_release_page()
2022-08-02 6:39 ` [PATCH v7 09/12] migration: Export ram_release_page() Juan Quintela
@ 2022-08-11 9:31 ` Leonardo Brás
0 siblings, 0 replies; 43+ messages in thread
From: Leonardo Brás @ 2022-08-11 9:31 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/ram.h | 1 +
> migration/ram.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.h b/migration/ram.h
> index e844966f69..038d52f49f 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -66,6 +66,7 @@ int ram_load_postcopy(QEMUFile *f, int channel);
> void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>
> void ram_transferred_add(uint64_t bytes);
> +void ram_release_page(const char *rbname, uint64_t offset);
>
> int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
> bool ramblock_recv_bitmap_test_byte_offset(RAMBlock *rb, uint64_t byte_offset);
> diff --git a/migration/ram.c b/migration/ram.c
> index 499d9b2a90..291ba5c0ed 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1238,7 +1238,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
> }
> }
>
> -static void ram_release_page(const char *rbname, uint64_t offset)
> +void ram_release_page(const char *rbname, uint64_t offset)
> {
> if (!migrate_release_ram() || !migration_in_postcopy()) {
> return;
LGTM. FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 10/12] multifd: Support for zero pages transmission
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (8 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 09/12] migration: Export ram_release_page() Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-09-02 13:27 ` Leonardo Brás
2022-10-25 9:10 ` chuang xu
2022-08-02 6:39 ` [PATCH v7 11/12] multifd: Zero " Juan Quintela
2022-08-02 6:39 ` [PATCH v7 12/12] So we use multifd to transmit zero pages Juan Quintela
11 siblings, 2 replies; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
This patch adds counters and similar. Logic will be added on the
following patch.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
Added counters for duplicated/non duplicated pages.
Removed reviewed by from David.
Add total_zero_pages
---
migration/multifd.h | 17 ++++++++++++++++-
migration/multifd.c | 36 +++++++++++++++++++++++++++++-------
migration/ram.c | 2 --
migration/trace-events | 8 ++++----
4 files changed, 49 insertions(+), 14 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index cd389d18d2..a1b852200d 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -47,7 +47,10 @@ typedef struct {
/* size of the next packet that contains pages */
uint32_t next_packet_size;
uint64_t packet_num;
- uint64_t unused[4]; /* Reserved for future use */
+ /* zero pages */
+ uint32_t zero_pages;
+ uint32_t unused32[1]; /* Reserved for future use */
+ uint64_t unused64[3]; /* Reserved for future use */
char ramblock[256];
uint64_t offset[];
} __attribute__((packed)) MultiFDPacket_t;
@@ -127,6 +130,8 @@ typedef struct {
uint64_t num_packets;
/* non zero pages sent through this channel */
uint64_t total_normal_pages;
+ /* zero pages sent through this channel */
+ uint64_t total_zero_pages;
/* buffers to send */
struct iovec *iov;
/* number of iovs used */
@@ -135,6 +140,10 @@ typedef struct {
ram_addr_t *normal;
/* num of non zero pages */
uint32_t normal_num;
+ /* Pages that are zero */
+ ram_addr_t *zero;
+ /* num of zero pages */
+ uint32_t zero_num;
/* used for compression methods */
void *data;
} MultiFDSendParams;
@@ -184,12 +193,18 @@ typedef struct {
uint8_t *host;
/* non zero pages recv through this channel */
uint64_t total_normal_pages;
+ /* zero pages recv through this channel */
+ uint64_t total_zero_pages;
/* buffers to recv */
struct iovec *iov;
/* Pages that are not zero */
ram_addr_t *normal;
/* num of non zero pages */
uint32_t normal_num;
+ /* Pages that are zero */
+ ram_addr_t *zero;
+ /* num of zero pages */
+ uint32_t zero_num;
/* used for de-compression methods */
void *data;
} MultiFDRecvParams;
diff --git a/migration/multifd.c b/migration/multifd.c
index 68fc9f8e88..4473d9f834 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -263,6 +263,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
packet->normal_pages = cpu_to_be32(p->normal_num);
packet->next_packet_size = cpu_to_be32(p->next_packet_size);
packet->packet_num = cpu_to_be64(p->packet_num);
+ packet->zero_pages = cpu_to_be32(p->zero_num);
if (p->pages->block) {
strncpy(packet->ramblock, p->pages->block->idstr, 256);
@@ -323,7 +324,15 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
p->next_packet_size = be32_to_cpu(packet->next_packet_size);
p->packet_num = be64_to_cpu(packet->packet_num);
- if (p->normal_num == 0) {
+ p->zero_num = be32_to_cpu(packet->zero_pages);
+ if (p->zero_num > packet->pages_alloc - p->normal_num) {
+ error_setg(errp, "multifd: received packet "
+ "with %u zero pages and expected maximum pages are %u",
+ p->zero_num, packet->pages_alloc - p->normal_num) ;
+ return -1;
+ }
+
+ if (p->normal_num == 0 && p->zero_num == 0) {
return 0;
}
@@ -432,6 +441,8 @@ static int multifd_send_pages(QEMUFile *f)
ram_counters.multifd_bytes += p->sent_bytes;
qemu_file_acct_rate_limit(f, p->sent_bytes);
p->sent_bytes = 0;
+ ram_counters.normal += p->normal_num;
+ ram_counters.duplicate += p->zero_num;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
@@ -545,6 +556,8 @@ void multifd_save_cleanup(void)
p->iov = NULL;
g_free(p->normal);
p->normal = NULL;
+ g_free(p->zero);
+ p->zero = NULL;
multifd_send_state->ops->send_cleanup(p, &local_err);
if (local_err) {
migrate_set_error(migrate_get_current(), local_err);
@@ -666,6 +679,7 @@ static void *multifd_send_thread(void *opaque)
qemu_mutex_unlock(&p->mutex);
p->normal_num = 0;
+ p->zero_num = 0;
if (use_zero_copy_send) {
p->iovs_num = 0;
@@ -687,8 +701,8 @@ static void *multifd_send_thread(void *opaque)
}
multifd_send_fill_packet(p);
- trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
- p->next_packet_size);
+ trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
+ p->flags, p->next_packet_size);
if (use_zero_copy_send) {
/* Send header first, without zerocopy */
@@ -712,6 +726,7 @@ static void *multifd_send_thread(void *opaque)
qemu_mutex_lock(&p->mutex);
p->num_packets++;
p->total_normal_pages += p->normal_num;
+ p->total_zero_pages += p->zero_num;
p->pages->num = 0;
p->pages->block = NULL;
p->sent_bytes += p->packet_len;;
@@ -753,7 +768,8 @@ out:
qemu_mutex_unlock(&p->mutex);
rcu_unregister_thread();
- trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
+ trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages,
+ p->total_zero_pages);
return NULL;
}
@@ -938,6 +954,7 @@ int multifd_save_setup(Error **errp)
p->normal = g_new0(ram_addr_t, page_count);
p->page_size = qemu_target_page_size();
p->page_count = page_count;
+ p->zero = g_new0(ram_addr_t, page_count);
if (migrate_use_zero_copy_send()) {
p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
@@ -1046,6 +1063,8 @@ int multifd_load_cleanup(Error **errp)
p->iov = NULL;
g_free(p->normal);
p->normal = NULL;
+ g_free(p->zero);
+ p->zero = NULL;
multifd_recv_state->ops->recv_cleanup(p);
}
qemu_sem_destroy(&multifd_recv_state->sem_sync);
@@ -1116,13 +1135,14 @@ static void *multifd_recv_thread(void *opaque)
break;
}
- trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->flags,
- p->next_packet_size);
+ trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
+ p->flags, p->next_packet_size);
sync_needed = p->flags & MULTIFD_FLAG_SYNC;
/* recv methods don't know how to handle the SYNC flag */
p->flags &= ~MULTIFD_FLAG_SYNC;
p->num_packets++;
p->total_normal_pages += p->normal_num;
+ p->total_normal_pages += p->zero_num;
qemu_mutex_unlock(&p->mutex);
if (p->normal_num) {
@@ -1147,7 +1167,8 @@ static void *multifd_recv_thread(void *opaque)
qemu_mutex_unlock(&p->mutex);
rcu_unregister_thread();
- trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages);
+ trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages,
+ p->total_zero_pages);
return NULL;
}
@@ -1187,6 +1208,7 @@ int multifd_load_setup(Error **errp)
p->normal = g_new0(ram_addr_t, page_count);
p->page_count = page_count;
p->page_size = qemu_target_page_size();
+ p->zero = g_new0(ram_addr_t, page_count);
}
for (i = 0; i < thread_count; i++) {
diff --git a/migration/ram.c b/migration/ram.c
index 291ba5c0ed..2af70f517a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1412,8 +1412,6 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
if (multifd_queue_page(rs->f, block, offset) < 0) {
return -1;
}
- ram_counters.normal++;
-
return 1;
}
diff --git a/migration/trace-events b/migration/trace-events
index a34afe7b85..d34aec177c 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -120,21 +120,21 @@ postcopy_preempt_reset_channel(void) ""
# multifd.c
multifd_new_send_channel_async(uint8_t id) "channel %u"
-multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " pages %u flags 0x%x next packet size %u"
+multifd_recv(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t zero, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
multifd_recv_new_channel(uint8_t id) "channel %u"
multifd_recv_sync_main(long packet_num) "packet num %ld"
multifd_recv_sync_main_signal(uint8_t id) "channel %u"
multifd_recv_sync_main_wait(uint8_t id) "channel %u"
multifd_recv_terminate_threads(bool error) "error %d"
-multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %u packets %" PRIu64 " pages %" PRIu64
+multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64 " zero pages %" PRIu64
multifd_recv_thread_start(uint8_t id) "%u"
-multifd_send(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u flags 0x%x next packet size %u"
+multifd_send(uint8_t id, uint64_t packet_num, uint32_t normalpages, uint32_t zero_pages, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
multifd_send_error(uint8_t id) "channel %u"
multifd_send_sync_main(long packet_num) "packet num %ld"
multifd_send_sync_main_signal(uint8_t id) "channel %u"
multifd_send_sync_main_wait(uint8_t id) "channel %u"
multifd_send_terminate_threads(bool error) "error %d"
-multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64
+multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64 " zero pages %" PRIu64
multifd_send_thread_start(uint8_t id) "%u"
multifd_tls_outgoing_handshake_start(void *ioc, void *tioc, const char *hostname) "ioc=%p tioc=%p hostname=%s"
multifd_tls_outgoing_handshake_error(void *ioc, const char *err) "ioc=%p err=%s"
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 10/12] multifd: Support for zero pages transmission
2022-08-02 6:39 ` [PATCH v7 10/12] multifd: Support for zero pages transmission Juan Quintela
@ 2022-09-02 13:27 ` Leonardo Brás
2022-11-14 12:09 ` Juan Quintela
2022-10-25 9:10 ` chuang xu
1 sibling, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-09-02 13:27 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> This patch adds counters and similar. Logic will be added on the
> following patch.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> Added counters for duplicated/non duplicated pages.
> Removed reviewed by from David.
> Add total_zero_pages
> ---
> migration/multifd.h | 17 ++++++++++++++++-
> migration/multifd.c | 36 +++++++++++++++++++++++++++++-------
> migration/ram.c | 2 --
> migration/trace-events | 8 ++++----
> 4 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index cd389d18d2..a1b852200d 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -47,7 +47,10 @@ typedef struct {
> /* size of the next packet that contains pages */
> uint32_t next_packet_size;
> uint64_t packet_num;
> - uint64_t unused[4]; /* Reserved for future use */
> + /* zero pages */
> + uint32_t zero_pages;
> + uint32_t unused32[1]; /* Reserved for future use */
> + uint64_t unused64[3]; /* Reserved for future use */
> char ramblock[256];
> uint64_t offset[];
> } __attribute__((packed)) MultiFDPacket_t;
> @@ -127,6 +130,8 @@ typedef struct {
> uint64_t num_packets;
> /* non zero pages sent through this channel */
> uint64_t total_normal_pages;
> + /* zero pages sent through this channel */
> + uint64_t total_zero_pages;
> /* buffers to send */
> struct iovec *iov;
> /* number of iovs used */
> @@ -135,6 +140,10 @@ typedef struct {
> ram_addr_t *normal;
> /* num of non zero pages */
> uint32_t normal_num;
> + /* Pages that are zero */
> + ram_addr_t *zero;
> + /* num of zero pages */
> + uint32_t zero_num;
More of an organization viewpoint:
I can't see total_zero_pages, zero[] and zero_num as Multifd "Parameters".
But OTOH there are other data like this in the struct for keeping migration
status, so not an issue.
> /* used for compression methods */
> void *data;
> } MultiFDSendParams;
> @@ -184,12 +193,18 @@ typedef struct {
> uint8_t *host;
> /* non zero pages recv through this channel */
> uint64_t total_normal_pages;
> + /* zero pages recv through this channel */
> + uint64_t total_zero_pages;
> /* buffers to recv */
> struct iovec *iov;
> /* Pages that are not zero */
> ram_addr_t *normal;
> /* num of non zero pages */
> uint32_t normal_num;
> + /* Pages that are zero */
> + ram_addr_t *zero;
> + /* num of zero pages */
> + uint32_t zero_num;
> /* used for de-compression methods */
> void *data;
> } MultiFDRecvParams;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 68fc9f8e88..4473d9f834 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -263,6 +263,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
> packet->normal_pages = cpu_to_be32(p->normal_num);
> packet->next_packet_size = cpu_to_be32(p->next_packet_size);
> packet->packet_num = cpu_to_be64(p->packet_num);
> + packet->zero_pages = cpu_to_be32(p->zero_num);
>
> if (p->pages->block) {
> strncpy(packet->ramblock, p->pages->block->idstr, 256);
> @@ -323,7 +324,15 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> p->next_packet_size = be32_to_cpu(packet->next_packet_size);
> p->packet_num = be64_to_cpu(packet->packet_num);
>
> - if (p->normal_num == 0) {
> + p->zero_num = be32_to_cpu(packet->zero_pages);
> + if (p->zero_num > packet->pages_alloc - p->normal_num) {
> + error_setg(errp, "multifd: received packet "
> + "with %u zero pages and expected maximum pages are %u",
> + p->zero_num, packet->pages_alloc - p->normal_num) ;
> + return -1;
> + }
> +
> + if (p->normal_num == 0 && p->zero_num == 0) {
> return 0;
> }
>
> @@ -432,6 +441,8 @@ static int multifd_send_pages(QEMUFile *f)
> ram_counters.multifd_bytes += p->sent_bytes;
> qemu_file_acct_rate_limit(f, p->sent_bytes);
> p->sent_bytes = 0;
> + ram_counters.normal += p->normal_num;
> + ram_counters.duplicate += p->zero_num;
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_post(&p->sem);
>
> @@ -545,6 +556,8 @@ void multifd_save_cleanup(void)
> p->iov = NULL;
> g_free(p->normal);
> p->normal = NULL;
> + g_free(p->zero);
> + p->zero = NULL;
> multifd_send_state->ops->send_cleanup(p, &local_err);
> if (local_err) {
> migrate_set_error(migrate_get_current(), local_err);
> @@ -666,6 +679,7 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_unlock(&p->mutex);
>
> p->normal_num = 0;
> + p->zero_num = 0;
>
> if (use_zero_copy_send) {
> p->iovs_num = 0;
> @@ -687,8 +701,8 @@ static void *multifd_send_thread(void *opaque)
> }
> multifd_send_fill_packet(p);
>
> - trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> - p->next_packet_size);
> + trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
> + p->flags, p->next_packet_size);
>
> if (use_zero_copy_send) {
> /* Send header first, without zerocopy */
> @@ -712,6 +726,7 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_lock(&p->mutex);
> p->num_packets++;
> p->total_normal_pages += p->normal_num;
> + p->total_zero_pages += p->zero_num;
I can see it getting declared, incremented and used. But where is it initialized
in zero? I mean, should it not have 'p->total_normal_pages = 0;' somewhere in
setup?
(I understand multifd_save_setup() allocates a multifd_send_state->params with
g_new0(),but other variables are zeroed there, like p->pending_job and
p->write_flags, so why not?)
> p->pages->num = 0;
> p->pages->block = NULL;
> p->sent_bytes += p->packet_len;;
> @@ -753,7 +768,8 @@ out:
> qemu_mutex_unlock(&p->mutex);
>
> rcu_unregister_thread();
> - trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
> + trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages,
> + p->total_zero_pages);
>
> return NULL;
> }
> @@ -938,6 +954,7 @@ int multifd_save_setup(Error **errp)
> p->normal = g_new0(ram_addr_t, page_count);
> p->page_size = qemu_target_page_size();
> p->page_count = page_count;
> + p->zero = g_new0(ram_addr_t, page_count);
>
> if (migrate_use_zero_copy_send()) {
> p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
> @@ -1046,6 +1063,8 @@ int multifd_load_cleanup(Error **errp)
> p->iov = NULL;
> g_free(p->normal);
> p->normal = NULL;
> + g_free(p->zero);
> + p->zero = NULL;
> multifd_recv_state->ops->recv_cleanup(p);
> }
> qemu_sem_destroy(&multifd_recv_state->sem_sync);
> @@ -1116,13 +1135,14 @@ static void *multifd_recv_thread(void *opaque)
> break;
> }
>
> - trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->flags,
> - p->next_packet_size);
> + trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
> + p->flags, p->next_packet_size);
> sync_needed = p->flags & MULTIFD_FLAG_SYNC;
> /* recv methods don't know how to handle the SYNC flag */
> p->flags &= ~MULTIFD_FLAG_SYNC;
> p->num_packets++;
> p->total_normal_pages += p->normal_num;
> + p->total_normal_pages += p->zero_num;
> qemu_mutex_unlock(&p->mutex);
>
> if (p->normal_num) {
> @@ -1147,7 +1167,8 @@ static void *multifd_recv_thread(void *opaque)
> qemu_mutex_unlock(&p->mutex);
>
> rcu_unregister_thread();
> - trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages);
> + trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages,
> + p->total_zero_pages);
>
> return NULL;
> }
> @@ -1187,6 +1208,7 @@ int multifd_load_setup(Error **errp)
> p->normal = g_new0(ram_addr_t, page_count);
> p->page_count = page_count;
> p->page_size = qemu_target_page_size();
> + p->zero = g_new0(ram_addr_t, page_count);
> }
>
> for (i = 0; i < thread_count; i++) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 291ba5c0ed..2af70f517a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1412,8 +1412,6 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
> if (multifd_queue_page(rs->f, block, offset) < 0) {
> return -1;
> }
> - ram_counters.normal++;
> -
> return 1;
> }
>
> diff --git a/migration/trace-events b/migration/trace-events
> index a34afe7b85..d34aec177c 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -120,21 +120,21 @@ postcopy_preempt_reset_channel(void) ""
>
> # multifd.c
> multifd_new_send_channel_async(uint8_t id) "channel %u"
> -multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " pages %u flags 0x%x next packet size %u"
> +multifd_recv(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t zero, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
> multifd_recv_new_channel(uint8_t id) "channel %u"
> multifd_recv_sync_main(long packet_num) "packet num %ld"
> multifd_recv_sync_main_signal(uint8_t id) "channel %u"
> multifd_recv_sync_main_wait(uint8_t id) "channel %u"
> multifd_recv_terminate_threads(bool error) "error %d"
> -multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %u packets %" PRIu64 " pages %" PRIu64
> +multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64 " zero pages %" PRIu64
> multifd_recv_thread_start(uint8_t id) "%u"
> -multifd_send(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u flags 0x%x next packet size %u"
> +multifd_send(uint8_t id, uint64_t packet_num, uint32_t normalpages, uint32_t zero_pages, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
> multifd_send_error(uint8_t id) "channel %u"
> multifd_send_sync_main(long packet_num) "packet num %ld"
> multifd_send_sync_main_signal(uint8_t id) "channel %u"
> multifd_send_sync_main_wait(uint8_t id) "channel %u"
> multifd_send_terminate_threads(bool error) "error %d"
> -multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64
> +multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t normal_pages, uint64_t zero_pages) "channel %u packets %" PRIu64 " normal pages %" PRIu64 " zero pages %" PRIu64
> multifd_send_thread_start(uint8_t id) "%u"
> multifd_tls_outgoing_handshake_start(void *ioc, void *tioc, const char *hostname) "ioc=%p tioc=%p hostname=%s"
> multifd_tls_outgoing_handshake_error(void *ioc, const char *err) "ioc=%p err=%s"
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 10/12] multifd: Support for zero pages transmission
2022-09-02 13:27 ` Leonardo Brás
@ 2022-11-14 12:09 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-11-14 12:09 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
...
>> @@ -712,6 +726,7 @@ static void *multifd_send_thread(void *opaque)
>> qemu_mutex_lock(&p->mutex);
>> p->num_packets++;
>> p->total_normal_pages += p->normal_num;
>> + p->total_zero_pages += p->zero_num;
>
> I can see it getting declared, incremented and used. But where is it initialized
> in zero? I mean, should it not have 'p->total_normal_pages = 0;' somewhere in
> setup?
int multifd_save_setup(Error **errp)
{
....
thread_count = migrate_multifd_channels();
multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
You can see here, that we setup everything to zero. We only need to
initialize explicitely whatever is not zero.
> (I understand multifd_save_setup() allocates a multifd_send_state->params with
> g_new0(),but other variables are zeroed there, like p->pending_job and
> p->write_flags, so why not?)
Humm, I think that it is better to do it the other way around. Remove
the initilazations that are not zero. That way we only put whatever is
not zero.
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v7 10/12] multifd: Support for zero pages transmission
2022-08-02 6:39 ` [PATCH v7 10/12] multifd: Support for zero pages transmission Juan Quintela
2022-09-02 13:27 ` Leonardo Brás
@ 2022-10-25 9:10 ` chuang xu
2022-11-14 12:10 ` Juan Quintela
1 sibling, 1 reply; 43+ messages in thread
From: chuang xu @ 2022-10-25 9:10 UTC (permalink / raw)
To: Juan Quintela
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert,
Leonardo Bras, Peter Xu, Eric Blake, Philippe Mathieu-Daudé,
Yanan Wang, Markus Armbruster, Eduardo Habkost
On 2022/8/2 下午2:39, Juan Quintela wrote:
> This patch adds counters and similar. Logic will be added on the
> following patch.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> Added counters for duplicated/non duplicated pages.
> Removed reviewed by from David.
> Add total_zero_pages
> ---
> migration/multifd.h | 17 ++++++++++++++++-
> migration/multifd.c | 36 +++++++++++++++++++++++++++++-------
> migration/ram.c | 2 --
> migration/trace-events | 8 ++++----
> 4 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index cd389d18d2..a1b852200d 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -47,7 +47,10 @@ typedef struct {
> /* size of the next packet that contains pages */
> uint32_t next_packet_size;
> uint64_t packet_num;
> - uint64_t unused[4]; /* Reserved for future use */
> + /* zero pages */
> + uint32_t zero_pages;
> + uint32_t unused32[1]; /* Reserved for future use */
> + uint64_t unused64[3]; /* Reserved for future use */
> char ramblock[256];
> uint64_t offset[];
> } __attribute__((packed)) MultiFDPacket_t;
> @@ -127,6 +130,8 @@ typedef struct {
> uint64_t num_packets;
> /* non zero pages sent through this channel */
> uint64_t total_normal_pages;
> + /* zero pages sent through this channel */
> + uint64_t total_zero_pages;
> /* buffers to send */
> struct iovec *iov;
> /* number of iovs used */
> @@ -135,6 +140,10 @@ typedef struct {
> ram_addr_t *normal;
> /* num of non zero pages */
> uint32_t normal_num;
> + /* Pages that are zero */
> + ram_addr_t *zero;
> + /* num of zero pages */
> + uint32_t zero_num;
> /* used for compression methods */
> void *data;
> } MultiFDSendParams;
> @@ -184,12 +193,18 @@ typedef struct {
> uint8_t *host;
> /* non zero pages recv through this channel */
> uint64_t total_normal_pages;
> + /* zero pages recv through this channel */
> + uint64_t total_zero_pages;
> /* buffers to recv */
> struct iovec *iov;
> /* Pages that are not zero */
> ram_addr_t *normal;
> /* num of non zero pages */
> uint32_t normal_num;
> + /* Pages that are zero */
> + ram_addr_t *zero;
> + /* num of zero pages */
> + uint32_t zero_num;
> /* used for de-compression methods */
> void *data;
> } MultiFDRecvParams;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 68fc9f8e88..4473d9f834 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -263,6 +263,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
> packet->normal_pages = cpu_to_be32(p->normal_num);
> packet->next_packet_size = cpu_to_be32(p->next_packet_size);
> packet->packet_num = cpu_to_be64(p->packet_num);
> + packet->zero_pages = cpu_to_be32(p->zero_num);
>
> if (p->pages->block) {
> strncpy(packet->ramblock, p->pages->block->idstr, 256);
> @@ -323,7 +324,15 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> p->next_packet_size = be32_to_cpu(packet->next_packet_size);
> p->packet_num = be64_to_cpu(packet->packet_num);
>
> - if (p->normal_num == 0) {
> + p->zero_num = be32_to_cpu(packet->zero_pages);
> + if (p->zero_num > packet->pages_alloc - p->normal_num) {
> + error_setg(errp, "multifd: received packet "
> + "with %u zero pages and expected maximum pages are %u",
> + p->zero_num, packet->pages_alloc - p->normal_num) ;
> + return -1;
> + }
> +
> + if (p->normal_num == 0 && p->zero_num == 0) {
> return 0;
> }
>
> @@ -432,6 +441,8 @@ static int multifd_send_pages(QEMUFile *f)
> ram_counters.multifd_bytes += p->sent_bytes;
> qemu_file_acct_rate_limit(f, p->sent_bytes);
> p->sent_bytes = 0;
> + ram_counters.normal += p->normal_num;
> + ram_counters.duplicate += p->zero_num;
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_post(&p->sem);
>
> @@ -545,6 +556,8 @@ void multifd_save_cleanup(void)
> p->iov = NULL;
> g_free(p->normal);
> p->normal = NULL;
> + g_free(p->zero);
> + p->zero = NULL;
> multifd_send_state->ops->send_cleanup(p, &local_err);
> if (local_err) {
> migrate_set_error(migrate_get_current(), local_err);
> @@ -666,6 +679,7 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_unlock(&p->mutex);
>
> p->normal_num = 0;
> + p->zero_num = 0;
>
> if (use_zero_copy_send) {
> p->iovs_num = 0;
> @@ -687,8 +701,8 @@ static void *multifd_send_thread(void *opaque)
> }
> multifd_send_fill_packet(p);
>
> - trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> - p->next_packet_size);
> + trace_multifd_send(p->id, packet_num, p->normal_num, p->zero_num,
> + p->flags, p->next_packet_size);
>
> if (use_zero_copy_send) {
> /* Send header first, without zerocopy */
> @@ -712,6 +726,7 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_lock(&p->mutex);
> p->num_packets++;
> p->total_normal_pages += p->normal_num;
> + p->total_zero_pages += p->zero_num;
> p->pages->num = 0;
> p->pages->block = NULL;
> p->sent_bytes += p->packet_len;;
> @@ -753,7 +768,8 @@ out:
> qemu_mutex_unlock(&p->mutex);
>
> rcu_unregister_thread();
> - trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
> + trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages,
> + p->total_zero_pages);
>
> return NULL;
> }
> @@ -938,6 +954,7 @@ int multifd_save_setup(Error **errp)
> p->normal = g_new0(ram_addr_t, page_count);
> p->page_size = qemu_target_page_size();
> p->page_count = page_count;
> + p->zero = g_new0(ram_addr_t, page_count);
>
> if (migrate_use_zero_copy_send()) {
> p->write_flags = QIO_CHANNEL_WRITE_FLAG_ZERO_COPY;
> @@ -1046,6 +1063,8 @@ int multifd_load_cleanup(Error **errp)
> p->iov = NULL;
> g_free(p->normal);
> p->normal = NULL;
> + g_free(p->zero);
> + p->zero = NULL;
> multifd_recv_state->ops->recv_cleanup(p);
> }
> qemu_sem_destroy(&multifd_recv_state->sem_sync);
> @@ -1116,13 +1135,14 @@ static void *multifd_recv_thread(void *opaque)
> break;
> }
>
> - trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->flags,
> - p->next_packet_size);
> + trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
> + p->flags, p->next_packet_size);
> sync_needed = p->flags & MULTIFD_FLAG_SYNC;
> /* recv methods don't know how to handle the SYNC flag */
> p->flags &= ~MULTIFD_FLAG_SYNC;
> p->num_packets++;
> p->total_normal_pages += p->normal_num;
> + p->total_normal_pages += p->zero_num;
Hi, Juan:
If I understand correctly, it should be "p->total_zero_pages +=
p->zero_num; ".
By the way, This patch seems to greatly improve the performance of zero
page checking, but it seems that there has been no new update in the
past two months. I want to know when it will be merged into master?
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 10/12] multifd: Support for zero pages transmission
2022-10-25 9:10 ` chuang xu
@ 2022-11-14 12:10 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-11-14 12:10 UTC (permalink / raw)
To: chuang xu
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert,
Leonardo Bras, Peter Xu, Eric Blake, Philippe Mathieu-Daudé,
Yanan Wang, Markus Armbruster, Eduardo Habkost
chuang xu <xuchuangxclwt@bytedance.com> wrote:
> On 2022/8/2 下午2:39, Juan Quintela wrote:
>> This patch adds counters and similar. Logic will be added on the
>> following patch.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> sync_needed = p->flags & MULTIFD_FLAG_SYNC;
>> /* recv methods don't know how to handle the SYNC flag */
>> p->flags &= ~MULTIFD_FLAG_SYNC;
>> p->num_packets++;
>> p->total_normal_pages += p->normal_num;
>> + p->total_normal_pages += p->zero_num;
>
> Hi, Juan:
>
> If I understand correctly, it should be "p->total_zero_pages +=
> p->zero_num; ".
Very good catch. Thanks.
That is what rebases make to you.
> By the way, This patch seems to greatly improve the performance of
> zero page checking, but it seems that there has been no new update in
> the past two months. I want to know when it will be merged into
> master?
I am resending right now.
Later, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 11/12] multifd: Zero pages transmission
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (9 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 10/12] multifd: Support for zero pages transmission Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-09-02 13:27 ` Leonardo Brás
2022-08-02 6:39 ` [PATCH v7 12/12] So we use multifd to transmit zero pages Juan Quintela
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
This implements the zero page dection and handling.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
Add comment for offset (dave)
Use local variables for offset/block to have shorter lines
---
migration/multifd.h | 5 +++++
migration/multifd.c | 41 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/migration/multifd.h b/migration/multifd.h
index a1b852200d..5931de6f86 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -52,6 +52,11 @@ typedef struct {
uint32_t unused32[1]; /* Reserved for future use */
uint64_t unused64[3]; /* Reserved for future use */
char ramblock[256];
+ /*
+ * This array contains the pointers to:
+ * - normal pages (initial normal_pages entries)
+ * - zero pages (following zero_pages entries)
+ */
uint64_t offset[];
} __attribute__((packed)) MultiFDPacket_t;
diff --git a/migration/multifd.c b/migration/multifd.c
index 4473d9f834..89811619d8 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -11,6 +11,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include "qemu/rcu.h"
#include "exec/target_page.h"
#include "sysemu/sysemu.h"
@@ -275,6 +276,12 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
packet->offset[i] = cpu_to_be64(temp);
}
+ for (i = 0; i < p->zero_num; i++) {
+ /* there are architectures where ram_addr_t is 32 bit */
+ uint64_t temp = p->zero[i];
+
+ packet->offset[p->normal_num + i] = cpu_to_be64(temp);
+ }
}
static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
@@ -358,6 +365,18 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
p->normal[i] = offset;
}
+ for (i = 0; i < p->zero_num; i++) {
+ uint64_t offset = be64_to_cpu(packet->offset[p->normal_num + i]);
+
+ if (offset > (block->used_length - p->page_size)) {
+ error_setg(errp, "multifd: offset too long %" PRIu64
+ " (max " RAM_ADDR_FMT ")",
+ offset, block->used_length);
+ return -1;
+ }
+ p->zero[i] = offset;
+ }
+
return 0;
}
@@ -648,6 +667,8 @@ static void *multifd_send_thread(void *opaque)
{
MultiFDSendParams *p = opaque;
Error *local_err = NULL;
+ /* qemu older than 7.0 don't understand zero page on multifd channel */
+ bool use_zero_page = migrate_use_multifd_zero_page();
int ret = 0;
bool use_zero_copy_send = migrate_use_zero_copy_send();
@@ -670,6 +691,7 @@ static void *multifd_send_thread(void *opaque)
qemu_mutex_lock(&p->mutex);
if (p->pending_job) {
+ RAMBlock *rb = p->pages->block;
uint64_t packet_num = p->packet_num;
p->flags = 0;
if (p->sync_needed) {
@@ -688,8 +710,16 @@ static void *multifd_send_thread(void *opaque)
}
for (int i = 0; i < p->pages->num; i++) {
- p->normal[p->normal_num] = p->pages->offset[i];
- p->normal_num++;
+ uint64_t offset = p->pages->offset[i];
+ if (use_zero_page &&
+ buffer_is_zero(rb->host + offset, p->page_size)) {
+ p->zero[p->zero_num] = offset;
+ p->zero_num++;
+ ram_release_page(rb->idstr, offset);
+ } else {
+ p->normal[p->normal_num] = offset;
+ p->normal_num++;
+ }
}
if (p->normal_num) {
@@ -1152,6 +1182,13 @@ static void *multifd_recv_thread(void *opaque)
}
}
+ for (int i = 0; i < p->zero_num; i++) {
+ void *page = p->host + p->zero[i];
+ if (!buffer_is_zero(page, p->page_size)) {
+ memset(page, 0, p->page_size);
+ }
+ }
+
if (sync_needed) {
qemu_sem_post(&multifd_recv_state->sem_sync);
qemu_sem_wait(&p->sem_sync);
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 11/12] multifd: Zero pages transmission
2022-08-02 6:39 ` [PATCH v7 11/12] multifd: Zero " Juan Quintela
@ 2022-09-02 13:27 ` Leonardo Brás
2022-11-14 12:20 ` Juan Quintela
2022-11-14 12:27 ` Juan Quintela
0 siblings, 2 replies; 43+ messages in thread
From: Leonardo Brás @ 2022-09-02 13:27 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> This implements the zero page dection and handling.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> Add comment for offset (dave)
> Use local variables for offset/block to have shorter lines
> ---
> migration/multifd.h | 5 +++++
> migration/multifd.c | 41 +++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index a1b852200d..5931de6f86 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -52,6 +52,11 @@ typedef struct {
> uint32_t unused32[1]; /* Reserved for future use */
> uint64_t unused64[3]; /* Reserved for future use */
> char ramblock[256];
> + /*
> + * This array contains the pointers to:
> + * - normal pages (initial normal_pages entries)
> + * - zero pages (following zero_pages entries)
> + */
> uint64_t offset[];
> } __attribute__((packed)) MultiFDPacket_t;
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4473d9f834..89811619d8 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qemu/rcu.h"
> #include "exec/target_page.h"
> #include "sysemu/sysemu.h"
> @@ -275,6 +276,12 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
>
> packet->offset[i] = cpu_to_be64(temp);
> }
> + for (i = 0; i < p->zero_num; i++) {
> + /* there are architectures where ram_addr_t is 32 bit */
> + uint64_t temp = p->zero[i];
> +
> + packet->offset[p->normal_num + i] = cpu_to_be64(temp);
> + }
> }
>
> static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> @@ -358,6 +365,18 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> p->normal[i] = offset;
> }
>
> + for (i = 0; i < p->zero_num; i++) {
> + uint64_t offset = be64_to_cpu(packet->offset[p->normal_num + i]);
> +
> + if (offset > (block->used_length - p->page_size)) {
> + error_setg(errp, "multifd: offset too long %" PRIu64
> + " (max " RAM_ADDR_FMT ")",
> + offset, block->used_length);
> + return -1;
> + }
> + p->zero[i] = offset;
> + }
> +
> return 0;
> }
IIUC ram_addr_t is supposed to be the address size for the architecture, mainly
being 32 or 64 bits. So packet->offset[i] is always u64, and p->zero[i] possibly
being u32 or u64.
Since both local variables and packet->offset[i] are 64-bit, there is no issue.
But on 'p->zero[i] = offset' we can have 'u32 = u64', and this should raise a
warning (or am I missing something?).
>
> @@ -648,6 +667,8 @@ static void *multifd_send_thread(void *opaque)
> {
> MultiFDSendParams *p = opaque;
> Error *local_err = NULL;
> + /* qemu older than 7.0 don't understand zero page on multifd channel */
> + bool use_zero_page = migrate_use_multifd_zero_page();
> int ret = 0;
> bool use_zero_copy_send = migrate_use_zero_copy_send();
>
> @@ -670,6 +691,7 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_lock(&p->mutex);
>
> if (p->pending_job) {
> + RAMBlock *rb = p->pages->block;
> uint64_t packet_num = p->packet_num;
> p->flags = 0;
> if (p->sync_needed) {
> @@ -688,8 +710,16 @@ static void *multifd_send_thread(void *opaque)
> }
>
> for (int i = 0; i < p->pages->num; i++) {
> - p->normal[p->normal_num] = p->pages->offset[i];
> - p->normal_num++;
> + uint64_t offset = p->pages->offset[i];
> + if (use_zero_page &&
> + buffer_is_zero(rb->host + offset, p->page_size)) {
> + p->zero[p->zero_num] = offset;
Same here.
> + p->zero_num++;
> + ram_release_page(rb->idstr, offset);
> + } else {
> + p->normal[p->normal_num] = offset;
Same here? (p->normal[i] can also be u32)
> + p->normal_num++;
> + }
> }
>
> if (p->normal_num) {
> @@ -1152,6 +1182,13 @@ static void *multifd_recv_thread(void *opaque)
> }
> }
>
> + for (int i = 0; i < p->zero_num; i++) {
> + void *page = p->host + p->zero[i];
> + if (!buffer_is_zero(page, p->page_size)) {
> + memset(page, 0, p->page_size);
> + }
> + }
> +
> if (sync_needed) {
> qemu_sem_post(&multifd_recv_state->sem_sync);
> qemu_sem_wait(&p->sem_sync);
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 11/12] multifd: Zero pages transmission
2022-09-02 13:27 ` Leonardo Brás
@ 2022-11-14 12:20 ` Juan Quintela
2022-11-14 12:27 ` Juan Quintela
1 sibling, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-11-14 12:20 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> This implements the zero page dection and handling.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> @@ -358,6 +365,18 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> p->normal[i] = offset;
>> }
>>
>> + for (i = 0; i < p->zero_num; i++) {
>> + uint64_t offset = be64_to_cpu(packet->offset[p->normal_num + i]);
>> +
>> + if (offset > (block->used_length - p->page_size)) {
>> + error_setg(errp, "multifd: offset too long %" PRIu64
>> + " (max " RAM_ADDR_FMT ")",
>> + offset, block->used_length);
>> + return -1;
>> + }
>> + p->zero[i] = offset;
>> + }
>> +
>> return 0;
>> }
>
> IIUC ram_addr_t is supposed to be the address size for the architecture, mainly
> being 32 or 64 bits. So packet->offset[i] is always u64, and p->zero[i] possibly
> being u32 or u64.
>
> Since both local variables and packet->offset[i] are 64-bit, there is no issue.
>
> But on 'p->zero[i] = offset' we can have 'u32 = u64', and this should raise a
> warning (or am I missing something?).
I don't really know what to do here.
The problem is only theoretical (in the long, long past, we have
supported migrating between different architectures, but we aren't
testing anymore).
And because it was a pain in the ass, we define it as:
/* address in the RAM (different from a physical address) */
#if defined(CONFIG_XEN_BACKEND)
typedef uint64_t ram_addr_t;
# define RAM_ADDR_MAX UINT64_MAX
# define RAM_ADDR_FMT "%" PRIx64
#else
typedef uintptr_t ram_addr_t;
# define RAM_ADDR_MAX UINTPTR_MAX
# define RAM_ADDR_FMT "%" PRIxPTR
#endif
So I am pretty sure that almost nothing uses 32bits for it now (I
haven't checked lately, but I guess that nobody is really using/testing
xen on 32 bits).
I don't really know. But it could only happens when you are migrating
from Xen 64 bits to Xen 32 bits, I don't really know if that even work.
I will give it a try to change normal/zero to u64.
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 11/12] multifd: Zero pages transmission
2022-09-02 13:27 ` Leonardo Brás
2022-11-14 12:20 ` Juan Quintela
@ 2022-11-14 12:27 ` Juan Quintela
1 sibling, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-11-14 12:27 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> This implements the zero page dection and handling.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
Hi
on further investigation, I see why it can't be a problem.
>> static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> @@ -358,6 +365,18 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> p->normal[i] = offset;
>> }
>>
>> + for (i = 0; i < p->zero_num; i++) {
>> + uint64_t offset = be64_to_cpu(packet->offset[p->normal_num + i]);
>> +
>> + if (offset > (block->used_length - p->page_size)) {
We are checked that we are inside the RAM block. You can't have a
bigger that 32bit offset when you have 32bits of RAM.
>> @@ -688,8 +710,16 @@ static void *multifd_send_thread(void *opaque)
>> }
>>
>> for (int i = 0; i < p->pages->num; i++) {
>> - p->normal[p->normal_num] = p->pages->offset[i];
>> - p->normal_num++;
>> + uint64_t offset = p->pages->offset[i];
We are reading the offset here.
p->pages->offset is ram_addr_t, so no prolbem here.
>> + if (use_zero_page &&
>> + buffer_is_zero(rb->host + offset, p->page_size)) {
>> + p->zero[p->zero_num] = offset;
>
> Same here.
This and next case are exactly the same, we are doing:
ram_addr_t offset1;
u64 foo = offset1;
ram_addr_t offest2 = foo;
So, it should be right. Everything is unsigned here.
>> + p->zero_num++;
>> + ram_release_page(rb->idstr, offset);
>> + } else {
>> + p->normal[p->normal_num] = offset;
>
> Same here? (p->normal[i] can also be u32)
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v7 12/12] So we use multifd to transmit zero pages.
2022-08-02 6:38 [PATCH v7 00/12] Migration: Transmit and detect zero pages in the multifd threads Juan Quintela
` (10 preceding siblings ...)
2022-08-02 6:39 ` [PATCH v7 11/12] multifd: Zero " Juan Quintela
@ 2022-08-02 6:39 ` Juan Quintela
2022-09-02 13:27 ` Leonardo Brás
11 siblings, 1 reply; 43+ messages in thread
From: Juan Quintela @ 2022-08-02 6:39 UTC (permalink / raw)
To: qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Leonardo Bras, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Juan Quintela, Markus Armbruster, Eduardo Habkost
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
- Check zero_page property before using new code (Dave)
---
migration/migration.c | 4 +---
migration/multifd.c | 6 +++---
migration/ram.c | 33 ++++++++++++++++++++++++++++++++-
3 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index ce3e5cc0cd..13842f6803 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2599,9 +2599,7 @@ bool migrate_use_main_zero_page(void)
s = migrate_get_current();
- // We will enable this when we add the right code.
- // return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
- return true;
+ return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
}
bool migrate_pause_before_switchover(void)
diff --git a/migration/multifd.c b/migration/multifd.c
index 89811619d8..54acdc004c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -667,8 +667,8 @@ static void *multifd_send_thread(void *opaque)
{
MultiFDSendParams *p = opaque;
Error *local_err = NULL;
- /* qemu older than 7.0 don't understand zero page on multifd channel */
- bool use_zero_page = migrate_use_multifd_zero_page();
+ /* older qemu don't understand zero page on multifd channel */
+ bool use_multifd_zero_page = !migrate_use_main_zero_page();
int ret = 0;
bool use_zero_copy_send = migrate_use_zero_copy_send();
@@ -711,7 +711,7 @@ static void *multifd_send_thread(void *opaque)
for (int i = 0; i < p->pages->num; i++) {
uint64_t offset = p->pages->offset[i];
- if (use_zero_page &&
+ if (use_multifd_zero_page &&
buffer_is_zero(rb->host + offset, p->page_size)) {
p->zero[p->zero_num] = offset;
p->zero_num++;
diff --git a/migration/ram.c b/migration/ram.c
index 2af70f517a..26e60b9cc1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2428,6 +2428,32 @@ static void postcopy_preempt_reset_channel(RAMState *rs)
}
}
+/**
+ * ram_save_target_page_multifd: save one target page
+ *
+ * Returns the number of pages written
+ *
+ * @rs: current RAM state
+ * @pss: data about the page we want to send
+ */
+static int ram_save_target_page_multifd(RAMState *rs, PageSearchStatus *pss)
+{
+ RAMBlock *block = pss->block;
+ ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
+ int res;
+
+ if (!migration_in_postcopy()) {
+ return ram_save_multifd_page(rs, block, offset);
+ }
+
+ res = save_zero_page(rs, block, offset);
+ if (res > 0) {
+ return res;
+ }
+
+ return ram_save_page(rs, pss);
+}
+
/**
* ram_save_host_page: save a whole host page
*
@@ -3225,7 +3251,12 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
ram_control_before_iterate(f, RAM_CONTROL_SETUP);
ram_control_after_iterate(f, RAM_CONTROL_SETUP);
- (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
+ if (migrate_use_multifd() && !migrate_use_main_zero_page()) {
+ (*rsp)->ram_save_target_page = ram_save_target_page_multifd;
+ } else {
+ (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
+ }
+
ret = multifd_send_sync_main(f);
if (ret < 0) {
return ret;
--
2.37.1
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v7 12/12] So we use multifd to transmit zero pages.
2022-08-02 6:39 ` [PATCH v7 12/12] So we use multifd to transmit zero pages Juan Quintela
@ 2022-09-02 13:27 ` Leonardo Brás
2022-11-14 12:30 ` Juan Quintela
0 siblings, 1 reply; 43+ messages in thread
From: Leonardo Brás @ 2022-09-02 13:27 UTC (permalink / raw)
To: Juan Quintela, qemu-devel
Cc: Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu, Eric Blake,
Philippe Mathieu-Daudé, Yanan Wang, Markus Armbruster,
Eduardo Habkost
On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> ---
>
> - Check zero_page property before using new code (Dave)
> ---
> migration/migration.c | 4 +---
> migration/multifd.c | 6 +++---
> migration/ram.c | 33 ++++++++++++++++++++++++++++++++-
> 3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index ce3e5cc0cd..13842f6803 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2599,9 +2599,7 @@ bool migrate_use_main_zero_page(void)
>
> s = migrate_get_current();
>
> - // We will enable this when we add the right code.
> - // return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
> - return true;
> + return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
> }
>
> bool migrate_pause_before_switchover(void)
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 89811619d8..54acdc004c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -667,8 +667,8 @@ static void *multifd_send_thread(void *opaque)
> {
> MultiFDSendParams *p = opaque;
> Error *local_err = NULL;
> - /* qemu older than 7.0 don't understand zero page on multifd channel */
> - bool use_zero_page = migrate_use_multifd_zero_page();
> + /* older qemu don't understand zero page on multifd channel */
> + bool use_multifd_zero_page = !migrate_use_main_zero_page();
I understand that "use_main_zero_page", which is introduced as a new capability,
is in fact the old behavior, and the new feature is introduced when this
capability is disabled.
But it sure looks weird reading:
use_multifd_zero_page = !migrate_use_main_zero_page();
This series is fresh in my mind, but it took a few seconds to see that this is
actually not a typo.
> int ret = 0;
> bool use_zero_copy_send = migrate_use_zero_copy_send();
>
> @@ -711,7 +711,7 @@ static void *multifd_send_thread(void *opaque)
>
> for (int i = 0; i < p->pages->num; i++) {
> uint64_t offset = p->pages->offset[i];
> - if (use_zero_page &&
> + if (use_multifd_zero_page &&
> buffer_is_zero(rb->host + offset, p->page_size)) {
> p->zero[p->zero_num] = offset;
> p->zero_num++;
> diff --git a/migration/ram.c b/migration/ram.c
> index 2af70f517a..26e60b9cc1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2428,6 +2428,32 @@ static void postcopy_preempt_reset_channel(RAMState *rs)
> }
> }
>
> +/**
> + * ram_save_target_page_multifd: save one target page
> + *
> + * Returns the number of pages written
> + *
> + * @rs: current RAM state
> + * @pss: data about the page we want to send
> + */
> +static int ram_save_target_page_multifd(RAMState *rs, PageSearchStatus *pss)
> +{
> + RAMBlock *block = pss->block;
> + ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
> + int res;
> +
> + if (!migration_in_postcopy()) {
> + return ram_save_multifd_page(rs, block, offset);
> + }
> +
> + res = save_zero_page(rs, block, offset);
> + if (res > 0) {
> + return res;
> + }
> +
> + return ram_save_page(rs, pss);
> +}
> +
> /**
> * ram_save_host_page: save a whole host page
> *
> @@ -3225,7 +3251,12 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> ram_control_before_iterate(f, RAM_CONTROL_SETUP);
> ram_control_after_iterate(f, RAM_CONTROL_SETUP);
>
> - (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
> + if (migrate_use_multifd() && !migrate_use_main_zero_page()) {
> + (*rsp)->ram_save_target_page = ram_save_target_page_multifd;
> + } else {
> + (*rsp)->ram_save_target_page = ram_save_target_page_legacy;
> + }
> +
> ret = multifd_send_sync_main(f);
> if (ret < 0) {
> return ret;
The rest LGTM.
FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v7 12/12] So we use multifd to transmit zero pages.
2022-09-02 13:27 ` Leonardo Brás
@ 2022-11-14 12:30 ` Juan Quintela
0 siblings, 0 replies; 43+ messages in thread
From: Juan Quintela @ 2022-11-14 12:30 UTC (permalink / raw)
To: Leonardo Brás
Cc: qemu-devel, Marcel Apfelbaum, Dr. David Alan Gilbert, Peter Xu,
Eric Blake, Philippe Mathieu-Daudé, Yanan Wang,
Markus Armbruster, Eduardo Habkost
Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 89811619d8..54acdc004c 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -667,8 +667,8 @@ static void *multifd_send_thread(void *opaque)
>> {
>> MultiFDSendParams *p = opaque;
>> Error *local_err = NULL;
>> - /* qemu older than 7.0 don't understand zero page on multifd channel */
>> - bool use_zero_page = migrate_use_multifd_zero_page();
>> + /* older qemu don't understand zero page on multifd channel */
>> + bool use_multifd_zero_page = !migrate_use_main_zero_page();
>
> I understand that "use_main_zero_page", which is introduced as a new capability,
> is in fact the old behavior, and the new feature is introduced when this
> capability is disabled.
>
> But it sure looks weird reading:
> use_multifd_zero_page = !migrate_use_main_zero_page();
>
> This series is fresh in my mind, but it took a few seconds to see that this is
> actually not a typo.
We can't have it both ways.
All other capabilities are false by default. And libvirt assumes they
are false. So, or we are willing to change the expectations, or we need
to do it this way.
In previous versions, I had the capability named the other way around,
and I changed it due to this.
Thanks, Juan.
^ permalink raw reply [flat|nested] 43+ messages in thread