* [PATCH 0/5] migration: misc cleanups
@ 2021-12-16 9:13 Juan Quintela
2021-12-16 9:13 ` [PATCH 1/5] migration: All this fields are unsigned Juan Quintela
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Juan Quintela @ 2021-12-16 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu, Juan Quintela
Hi
This series do several cleanups:
- Dave found that I was using "%d" for unsigned, fix all uses.
- We pass last_stage left and right, but we only use it in two places
Just move it to RAMState.
- do_compress_page() used a goto when not needed.
- ram_release_pages() was always used with one page
- And put it when we detect zero pages, instead of everywhere we have find a zero page.
Please, review.
Juan Quintela (5):
migration: All this fields are unsigned
migration: We only need last_stage in two places
migration: ram_release_pages() always receive 1 page as argument
migration: simplify do_compress_ram_page
migration: Move ram_release_pages() call to save_zero_page_to_file()
migration/multifd-zlib.c | 20 +++++------
migration/multifd-zstd.c | 24 +++++++-------
migration/multifd.c | 16 ++++-----
migration/ram.c | 71 +++++++++++++++++-----------------------
migration/trace-events | 26 +++++++--------
5 files changed, 73 insertions(+), 84 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] migration: All this fields are unsigned
2021-12-16 9:13 [PATCH 0/5] migration: misc cleanups Juan Quintela
@ 2021-12-16 9:13 ` Juan Quintela
2021-12-16 9:26 ` Philippe Mathieu-Daudé
2021-12-16 9:13 ` [PATCH 2/5] migration: We only need last_stage in two places Juan Quintela
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Juan Quintela @ 2021-12-16 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu, Juan Quintela
So printing it as %d is wrong. Notice that for the channel id, that
is an uint8_t, but I changed it anyways for consistency.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/multifd-zlib.c | 20 ++++++++++----------
migration/multifd-zstd.c | 24 ++++++++++++------------
migration/multifd.c | 16 ++++++++--------
migration/trace-events | 26 +++++++++++++-------------
4 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c
index da6201704c..9f6ebf1076 100644
--- a/migration/multifd-zlib.c
+++ b/migration/multifd-zlib.c
@@ -51,7 +51,7 @@ static int zlib_send_setup(MultiFDSendParams *p, Error **errp)
zs->opaque = Z_NULL;
if (deflateInit(zs, migrate_multifd_zlib_level()) != Z_OK) {
g_free(z);
- error_setg(errp, "multifd %d: deflate init failed", p->id);
+ error_setg(errp, "multifd %u: deflate init failed", p->id);
return -1;
}
/* To be safe, we reserve twice the size of the packet */
@@ -60,7 +60,7 @@ static int zlib_send_setup(MultiFDSendParams *p, Error **errp)
if (!z->zbuff) {
deflateEnd(&z->zs);
g_free(z);
- error_setg(errp, "multifd %d: out of memory for zbuff", p->id);
+ error_setg(errp, "multifd %u: out of memory for zbuff", p->id);
return -1;
}
p->data = z;
@@ -132,12 +132,12 @@ static int zlib_send_prepare(MultiFDSendParams *p, Error **errp)
ret = deflate(zs, flush);
} while (ret == Z_OK && zs->avail_in && zs->avail_out);
if (ret == Z_OK && zs->avail_in) {
- error_setg(errp, "multifd %d: deflate failed to compress all input",
+ error_setg(errp, "multifd %u: deflate failed to compress all input",
p->id);
return -1;
}
if (ret != Z_OK) {
- error_setg(errp, "multifd %d: deflate returned %d instead of Z_OK",
+ error_setg(errp, "multifd %u: deflate returned %d instead of Z_OK",
p->id, ret);
return -1;
}
@@ -190,7 +190,7 @@ static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp)
zs->avail_in = 0;
zs->next_in = Z_NULL;
if (inflateInit(zs) != Z_OK) {
- error_setg(errp, "multifd %d: inflate init failed", p->id);
+ error_setg(errp, "multifd %u: inflate init failed", p->id);
return -1;
}
/* To be safe, we reserve twice the size of the packet */
@@ -198,7 +198,7 @@ static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp)
z->zbuff = g_try_malloc(z->zbuff_len);
if (!z->zbuff) {
inflateEnd(zs);
- error_setg(errp, "multifd %d: out of memory for zbuff", p->id);
+ error_setg(errp, "multifd %u: out of memory for zbuff", p->id);
return -1;
}
return 0;
@@ -247,7 +247,7 @@ static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
int i;
if (flags != MULTIFD_FLAG_ZLIB) {
- error_setg(errp, "multifd %d: flags received %x flags expected %x",
+ error_setg(errp, "multifd %u: flags received %x flags expected %x",
p->id, flags, MULTIFD_FLAG_ZLIB);
return -1;
}
@@ -284,19 +284,19 @@ static int zlib_recv_pages(MultiFDRecvParams *p, Error **errp)
} while (ret == Z_OK && zs->avail_in
&& (zs->total_out - start) < page_size);
if (ret == Z_OK && (zs->total_out - start) < page_size) {
- error_setg(errp, "multifd %d: inflate generated too few output",
+ error_setg(errp, "multifd %u: inflate generated too few output",
p->id);
return -1;
}
if (ret != Z_OK) {
- error_setg(errp, "multifd %d: inflate returned %d instead of Z_OK",
+ error_setg(errp, "multifd %u: inflate returned %d instead of Z_OK",
p->id, ret);
return -1;
}
}
out_size = zs->total_out - out_size;
if (out_size != expected_size) {
- error_setg(errp, "multifd %d: packet size received %d size expected %d",
+ error_setg(errp, "multifd %u: packet size received %u size expected %u",
p->id, out_size, expected_size);
return -1;
}
diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c
index 2d5b61106c..cc4e991724 100644
--- a/migration/multifd-zstd.c
+++ b/migration/multifd-zstd.c
@@ -55,7 +55,7 @@ static int zstd_send_setup(MultiFDSendParams *p, Error **errp)
z->zcs = ZSTD_createCStream();
if (!z->zcs) {
g_free(z);
- error_setg(errp, "multifd %d: zstd createCStream failed", p->id);
+ error_setg(errp, "multifd %u: zstd createCStream failed", p->id);
return -1;
}
@@ -63,7 +63,7 @@ static int zstd_send_setup(MultiFDSendParams *p, Error **errp)
if (ZSTD_isError(res)) {
ZSTD_freeCStream(z->zcs);
g_free(z);
- error_setg(errp, "multifd %d: initCStream failed with error %s",
+ error_setg(errp, "multifd %u: initCStream failed with error %s",
p->id, ZSTD_getErrorName(res));
return -1;
}
@@ -73,7 +73,7 @@ static int zstd_send_setup(MultiFDSendParams *p, Error **errp)
if (!z->zbuff) {
ZSTD_freeCStream(z->zcs);
g_free(z);
- error_setg(errp, "multifd %d: out of memory for zbuff", p->id);
+ error_setg(errp, "multifd %u: out of memory for zbuff", p->id);
return -1;
}
return 0;
@@ -144,12 +144,12 @@ static int zstd_send_prepare(MultiFDSendParams *p, Error **errp)
} while (ret > 0 && (z->in.size - z->in.pos > 0)
&& (z->out.size - z->out.pos > 0));
if (ret > 0 && (z->in.size - z->in.pos > 0)) {
- error_setg(errp, "multifd %d: compressStream buffer too small",
+ error_setg(errp, "multifd %u: compressStream buffer too small",
p->id);
return -1;
}
if (ZSTD_isError(ret)) {
- error_setg(errp, "multifd %d: compressStream error %s",
+ error_setg(errp, "multifd %u: compressStream error %s",
p->id, ZSTD_getErrorName(ret));
return -1;
}
@@ -198,7 +198,7 @@ static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp)
z->zds = ZSTD_createDStream();
if (!z->zds) {
g_free(z);
- error_setg(errp, "multifd %d: zstd createDStream failed", p->id);
+ error_setg(errp, "multifd %u: zstd createDStream failed", p->id);
return -1;
}
@@ -206,7 +206,7 @@ static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp)
if (ZSTD_isError(ret)) {
ZSTD_freeDStream(z->zds);
g_free(z);
- error_setg(errp, "multifd %d: initDStream failed with error %s",
+ error_setg(errp, "multifd %u: initDStream failed with error %s",
p->id, ZSTD_getErrorName(ret));
return -1;
}
@@ -217,7 +217,7 @@ static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp)
if (!z->zbuff) {
ZSTD_freeDStream(z->zds);
g_free(z);
- error_setg(errp, "multifd %d: out of memory for zbuff", p->id);
+ error_setg(errp, "multifd %u: out of memory for zbuff", p->id);
return -1;
}
return 0;
@@ -265,7 +265,7 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
int i;
if (flags != MULTIFD_FLAG_ZSTD) {
- error_setg(errp, "multifd %d: flags received %x flags expected %x",
+ error_setg(errp, "multifd %u: flags received %x flags expected %x",
p->id, flags, MULTIFD_FLAG_ZSTD);
return -1;
}
@@ -297,19 +297,19 @@ static int zstd_recv_pages(MultiFDRecvParams *p, Error **errp)
} while (ret > 0 && (z->in.size - z->in.pos > 0)
&& (z->out.pos < page_size));
if (ret > 0 && (z->out.pos < page_size)) {
- error_setg(errp, "multifd %d: decompressStream buffer too small",
+ error_setg(errp, "multifd %u: decompressStream buffer too small",
p->id);
return -1;
}
if (ZSTD_isError(ret)) {
- error_setg(errp, "multifd %d: decompressStream returned %s",
+ error_setg(errp, "multifd %u: decompressStream returned %s",
p->id, ZSTD_getErrorName(ret));
return ret;
}
out_size += z->out.pos;
}
if (out_size != expected_size) {
- error_setg(errp, "multifd %d: packet size received %d size expected %d",
+ error_setg(errp, "multifd %u: packet size received %u size expected %u",
p->id, out_size, expected_size);
return -1;
}
diff --git a/migration/multifd.c b/migration/multifd.c
index 3242f688e5..4d62850258 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -148,7 +148,7 @@ static int nocomp_recv_pages(MultiFDRecvParams *p, Error **errp)
uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK;
if (flags != MULTIFD_FLAG_NOCOMP) {
- error_setg(errp, "multifd %d: flags received %x flags expected %x",
+ error_setg(errp, "multifd %u: flags received %x flags expected %x",
p->id, flags, MULTIFD_FLAG_NOCOMP);
return -1;
}
@@ -212,8 +212,8 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
}
if (msg.version != MULTIFD_VERSION) {
- error_setg(errp, "multifd: received packet version %d "
- "expected %d", msg.version, MULTIFD_VERSION);
+ error_setg(errp, "multifd: received packet version %u "
+ "expected %u", msg.version, MULTIFD_VERSION);
return -1;
}
@@ -229,8 +229,8 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
}
if (msg.id > migrate_multifd_channels()) {
- error_setg(errp, "multifd: received channel version %d "
- "expected %d", msg.version, MULTIFD_VERSION);
+ error_setg(errp, "multifd: received channel version %u "
+ "expected %u", msg.version, MULTIFD_VERSION);
return -1;
}
@@ -303,7 +303,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
packet->version = be32_to_cpu(packet->version);
if (packet->version != MULTIFD_VERSION) {
error_setg(errp, "multifd: received packet "
- "version %d and expected version %d",
+ "version %u and expected version %u",
packet->version, MULTIFD_VERSION);
return -1;
}
@@ -317,7 +317,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
*/
if (packet->pages_alloc > pages_max * 100) {
error_setg(errp, "multifd: received packet "
- "with size %d and expected a maximum size of %d",
+ "with size %u and expected a maximum size of %u",
packet->pages_alloc, pages_max * 100) ;
return -1;
}
@@ -333,7 +333,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
p->pages->num = be32_to_cpu(packet->pages_used);
if (p->pages->num > packet->pages_alloc) {
error_setg(errp, "multifd: received packet "
- "with %d pages and expected maximum pages are %d",
+ "with %u pages and expected maximum pages are %u",
p->pages->num, packet->pages_alloc) ;
return -1;
}
diff --git a/migration/trace-events b/migration/trace-events
index b48d873b8a..5172cb3b3d 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -115,23 +115,23 @@ ram_write_tracking_ramblock_start(const char *block_id, size_t page_size, void *
ram_write_tracking_ramblock_stop(const char *block_id, size_t page_size, void *addr, size_t length) "%s: page_size: %zu addr: %p length: %zu"
# multifd.c
-multifd_new_send_channel_async(uint8_t id) "channel %d"
-multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %d packet_num %" PRIu64 " pages %d flags 0x%x next packet size %d"
-multifd_recv_new_channel(uint8_t id) "channel %d"
+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_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 %d"
-multifd_recv_sync_main_wait(uint8_t id) "channel %d"
+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 %d packets %" PRIu64 " pages %" PRIu64
-multifd_recv_thread_start(uint8_t id) "%d"
-multifd_send(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %d packet_num %" PRIu64 " pages %d flags 0x%x next packet size %d"
-multifd_send_error(uint8_t id) "channel %d"
+multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %u packets %" PRIu64 " pages %" PRIu64
+multifd_recv_thread_start(uint8_t id) "%u"
+multifd_send(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_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 %d"
-multifd_send_sync_main_wait(uint8_t id) "channel %d"
+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 pages) "channel %d packets %" PRIu64 " pages %" PRIu64
-multifd_send_thread_start(uint8_t id) "%d"
+multifd_send_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %u packets %" PRIu64 " 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"
multifd_tls_outgoing_handshake_complete(void *ioc) "ioc=%p"
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/5] migration: We only need last_stage in two places
2021-12-16 9:13 [PATCH 0/5] migration: misc cleanups Juan Quintela
2021-12-16 9:13 ` [PATCH 1/5] migration: All this fields are unsigned Juan Quintela
@ 2021-12-16 9:13 ` Juan Quintela
2021-12-16 9:30 ` Philippe Mathieu-Daudé
2021-12-16 9:13 ` [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument Juan Quintela
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Juan Quintela @ 2021-12-16 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu, Juan Quintela
And we are passing it all around. Just add a field to RAMState that
passes it.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 41 ++++++++++++++++++-----------------------
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 57efa67f20..7223b0d8ca 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -325,7 +325,8 @@ struct RAMState {
uint64_t xbzrle_bytes_prev;
/* Start using XBZRLE (e.g., after the first round). */
bool xbzrle_enabled;
-
+ /* Are we on the last stage of migration */
+ bool last_stage;
/* compression statistics since the beginning of the period */
/* amount of count that no free thread to compress data */
uint64_t compress_thread_busy_prev;
@@ -683,11 +684,10 @@ static void xbzrle_cache_zero_page(RAMState *rs, ram_addr_t current_addr)
* @current_addr: addr of the page
* @block: block that contains the page we want to send
* @offset: offset inside the block for the page
- * @last_stage: if we are at the completion stage
*/
static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
ram_addr_t current_addr, RAMBlock *block,
- ram_addr_t offset, bool last_stage)
+ ram_addr_t offset)
{
int encoded_len = 0, bytes_xbzrle;
uint8_t *prev_cached_page;
@@ -695,7 +695,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
if (!cache_is_cached(XBZRLE.cache, current_addr,
ram_counters.dirty_sync_count)) {
xbzrle_counters.cache_miss++;
- if (!last_stage) {
+ if (!rs->last_stage) {
if (cache_insert(XBZRLE.cache, current_addr, *current_data,
ram_counters.dirty_sync_count) == -1) {
return -1;
@@ -734,7 +734,7 @@ static int save_xbzrle_page(RAMState *rs, uint8_t **current_data,
* Update the cache contents, so that it corresponds to the data
* sent, in all cases except where we skip the page.
*/
- if (!last_stage && encoded_len != 0) {
+ if (!rs->last_stage && encoded_len != 0) {
memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
/*
* In the case where we couldn't compress, ensure that the caller
@@ -1290,9 +1290,8 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
* @rs: current RAM state
* @block: block that contains the page we want to send
* @offset: offset inside the block for the page
- * @last_stage: if we are at the completion stage
*/
-static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
+static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
{
int pages = -1;
uint8_t *p;
@@ -1307,8 +1306,8 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
XBZRLE_cache_lock();
if (rs->xbzrle_enabled && !migration_in_postcopy()) {
pages = save_xbzrle_page(rs, &p, current_addr, block,
- offset, last_stage);
- if (!last_stage) {
+ offset);
+ if (!rs->last_stage) {
/* Can't send this cached data async, since the cache page
* might get updated before it gets to the wire
*/
@@ -2129,10 +2128,8 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
*
* @rs: current RAM state
* @pss: data about the page we want to send
- * @last_stage: if we are at the completion stage
*/
-static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
- bool last_stage)
+static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
{
RAMBlock *block = pss->block;
ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
@@ -2171,7 +2168,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
return ram_save_multifd_page(rs, block, offset);
}
- return ram_save_page(rs, pss, last_stage);
+ return ram_save_page(rs, pss);
}
/**
@@ -2190,10 +2187,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
* @rs: current RAM state
* @ms: current migration state
* @pss: data about the page we want to send
- * @last_stage: if we are at the completion stage
*/
-static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
- bool last_stage)
+static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss)
{
int tmppages, pages = 0;
size_t pagesize_bits =
@@ -2211,7 +2206,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
do {
/* 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, last_stage);
+ tmppages = ram_save_target_page(rs, pss);
if (tmppages < 0) {
return tmppages;
}
@@ -2245,13 +2240,11 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
* or negative on error
*
* @rs: current RAM state
- * @last_stage: if we are at the completion stage
*
* On systems where host-page-size > target-page-size it will send all the
* pages in a host page that are dirty.
*/
-
-static int ram_find_and_save_block(RAMState *rs, bool last_stage)
+static int ram_find_and_save_block(RAMState *rs)
{
PageSearchStatus pss;
int pages = 0;
@@ -2280,7 +2273,7 @@ static int ram_find_and_save_block(RAMState *rs, bool last_stage)
}
if (found) {
- pages = ram_save_host_page(rs, &pss, last_stage);
+ pages = ram_save_host_page(rs, &pss);
}
} while (!pages && again);
@@ -3080,7 +3073,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
break;
}
- pages = ram_find_and_save_block(rs, false);
+ pages = ram_find_and_save_block(rs);
/* no more pages to sent */
if (pages == 0) {
done = 1;
@@ -3160,6 +3153,8 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
RAMState *rs = *temp;
int ret = 0;
+ rs->last_stage = !migration_in_colo_state();
+
WITH_RCU_READ_LOCK_GUARD() {
if (!migration_in_postcopy()) {
migration_bitmap_sync_precopy(rs);
@@ -3173,7 +3168,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
while (true) {
int pages;
- pages = ram_find_and_save_block(rs, !migration_in_colo_state());
+ pages = ram_find_and_save_block(rs);
/* no more blocks to sent */
if (pages == 0) {
break;
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument
2021-12-16 9:13 [PATCH 0/5] migration: misc cleanups Juan Quintela
2021-12-16 9:13 ` [PATCH 1/5] migration: All this fields are unsigned Juan Quintela
2021-12-16 9:13 ` [PATCH 2/5] migration: We only need last_stage in two places Juan Quintela
@ 2021-12-16 9:13 ` Juan Quintela
2021-12-16 9:27 ` Philippe Mathieu-Daudé
2021-12-16 9:13 ` [PATCH 4/5] migration: simplify do_compress_ram_page Juan Quintela
2021-12-16 9:13 ` [PATCH 5/5] migration: Move ram_release_pages() call to save_zero_page_to_file() Juan Quintela
4 siblings, 1 reply; 13+ messages in thread
From: Juan Quintela @ 2021-12-16 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu, Juan Quintela
Remove the pages argument. And s/pages/page/
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 7223b0d8ca..8d29d64b35 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1204,13 +1204,13 @@ static int save_zero_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
return -1;
}
-static void ram_release_pages(const char *rbname, uint64_t offset, int pages)
+static void ram_release_page(const char *rbname, uint64_t offset)
{
if (!migrate_release_ram() || !migration_in_postcopy()) {
return;
}
- ram_discard_range(rbname, offset, ((ram_addr_t)pages) << TARGET_PAGE_BITS);
+ ram_discard_range(rbname, offset, ((ram_addr_t)1) << TARGET_PAGE_BITS);
}
/*
@@ -1365,7 +1365,7 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
}
exit:
- ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+ ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
return zero_page;
}
@@ -2153,7 +2153,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
xbzrle_cache_zero_page(rs, block->offset + offset);
XBZRLE_cache_unlock();
}
- ram_release_pages(block->idstr, offset, res);
+ ram_release_page(block->idstr, offset);
return res;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/5] migration: simplify do_compress_ram_page
2021-12-16 9:13 [PATCH 0/5] migration: misc cleanups Juan Quintela
` (2 preceding siblings ...)
2021-12-16 9:13 ` [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument Juan Quintela
@ 2021-12-16 9:13 ` Juan Quintela
2021-12-16 9:13 ` [PATCH 5/5] migration: Move ram_release_pages() call to save_zero_page_to_file() Juan Quintela
4 siblings, 0 replies; 13+ messages in thread
From: Juan Quintela @ 2021-12-16 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu, Juan Quintela
The goto is not needed at all.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 8d29d64b35..0911811ed9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1341,12 +1341,11 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
{
RAMState *rs = ram_state;
uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
- bool zero_page = false;
int ret;
if (save_zero_page_to_file(rs, f, block, offset)) {
- zero_page = true;
- goto exit;
+ ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
+ return true;
}
save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
@@ -1361,12 +1360,8 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
if (ret < 0) {
qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
error_report("compressed data failed!");
- return false;
}
-
-exit:
- ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
- return zero_page;
+ return false;
}
static void
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] migration: Move ram_release_pages() call to save_zero_page_to_file()
2021-12-16 9:13 [PATCH 0/5] migration: misc cleanups Juan Quintela
` (3 preceding siblings ...)
2021-12-16 9:13 ` [PATCH 4/5] migration: simplify do_compress_ram_page Juan Quintela
@ 2021-12-16 9:13 ` Juan Quintela
4 siblings, 0 replies; 13+ messages in thread
From: Juan Quintela @ 2021-12-16 9:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu, Juan Quintela
We always need to call it when we find a zero page, so put it in a
single place.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
migration/ram.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 0911811ed9..eeeeb0b888 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1158,6 +1158,15 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
}
}
+static void ram_release_page(const char *rbname, uint64_t offset)
+{
+ if (!migrate_release_ram() || !migration_in_postcopy()) {
+ return;
+ }
+
+ ram_discard_range(rbname, offset, ((ram_addr_t)1) << TARGET_PAGE_BITS);
+}
+
/**
* save_zero_page_to_file: send the zero page to the file
*
@@ -1179,6 +1188,7 @@ static int save_zero_page_to_file(RAMState *rs, QEMUFile *file,
len += save_page_header(rs, file, block, offset | RAM_SAVE_FLAG_ZERO);
qemu_put_byte(file, 0);
len += 1;
+ ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
}
return len;
}
@@ -1204,15 +1214,6 @@ static int save_zero_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
return -1;
}
-static void ram_release_page(const char *rbname, uint64_t offset)
-{
- if (!migrate_release_ram() || !migration_in_postcopy()) {
- return;
- }
-
- ram_discard_range(rbname, offset, ((ram_addr_t)1) << TARGET_PAGE_BITS);
-}
-
/*
* @pages: the number of pages written by the control path,
* < 0 - error
@@ -1344,7 +1345,6 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
int ret;
if (save_zero_page_to_file(rs, f, block, offset)) {
- ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
return true;
}
@@ -2148,7 +2148,6 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
xbzrle_cache_zero_page(rs, block->offset + offset);
XBZRLE_cache_unlock();
}
- ram_release_page(block->idstr, offset);
return res;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] migration: All this fields are unsigned
2021-12-16 9:13 ` [PATCH 1/5] migration: All this fields are unsigned Juan Quintela
@ 2021-12-16 9:26 ` Philippe Mathieu-Daudé
2021-12-17 20:25 ` Richard Henderson
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-16 9:26 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu
On 12/16/21 10:13, Juan Quintela wrote:
> So printing it as %d is wrong. Notice that for the channel id, that
> is an uint8_t, but I changed it anyways for consistency.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/multifd-zlib.c | 20 ++++++++++----------
> migration/multifd-zstd.c | 24 ++++++++++++------------
> migration/multifd.c | 16 ++++++++--------
> migration/trace-events | 26 +++++++++++++-------------
> 4 files changed, 43 insertions(+), 43 deletions(-)
> # multifd.c
> -multifd_new_send_channel_async(uint8_t id) "channel %d"
> -multifd_recv(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %d packet_num %" PRIu64 " pages %d flags 0x%x next packet size %d"
> -multifd_recv_new_channel(uint8_t id) "channel %d"
> +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_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 %d"
> -multifd_recv_sync_main_wait(uint8_t id) "channel %d"
> +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 %d packets %" PRIu64 " pages %" PRIu64
> -multifd_recv_thread_start(uint8_t id) "%d"
> -multifd_send(uint8_t id, uint64_t packet_num, uint32_t used, uint32_t flags, uint32_t next_packet_size) "channel %d packet_num %" PRIu64 " pages %d flags 0x%x next packet size %d"
> -multifd_send_error(uint8_t id) "channel %d"
> +multifd_recv_thread_end(uint8_t id, uint64_t packets, uint64_t pages) "channel %u packets %" PRIu64 " pages %" PRIu64
> +multifd_recv_thread_start(uint8_t id) "%u"
> +multifd_send(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_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 %d"
> -multifd_send_sync_main_wait(uint8_t id) "channel %d"
> +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"
Nitpicking: bool is unsigned :)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument
2021-12-16 9:13 ` [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument Juan Quintela
@ 2021-12-16 9:27 ` Philippe Mathieu-Daudé
2021-12-21 9:35 ` Juan Quintela
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-16 9:27 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu
On 12/16/21 10:13, Juan Quintela wrote:
> Remove the pages argument. And s/pages/page/
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/ram.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> -static void ram_release_pages(const char *rbname, uint64_t offset, int pages)
> +static void ram_release_page(const char *rbname, uint64_t offset)
> {
> if (!migrate_release_ram() || !migration_in_postcopy()) {
> return;
> }
>
> - ram_discard_range(rbname, offset, ((ram_addr_t)pages) << TARGET_PAGE_BITS);
> + ram_discard_range(rbname, offset, ((ram_addr_t)1) << TARGET_PAGE_BITS);
1ULL?
Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/5] migration: We only need last_stage in two places
2021-12-16 9:13 ` [PATCH 2/5] migration: We only need last_stage in two places Juan Quintela
@ 2021-12-16 9:30 ` Philippe Mathieu-Daudé
2021-12-20 19:02 ` Juan Quintela
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-16 9:30 UTC (permalink / raw)
To: Juan Quintela, qemu-devel; +Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu
On 12/16/21 10:13, Juan Quintela wrote:
> And we are passing it all around. Just add a field to RAMState that
> passes it.
Splitting half of the description in the subject makes it hard to read.
Better to repeat the subject as first description line. Matter of taste.
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> migration/ram.c | 41 ++++++++++++++++++-----------------------
> 1 file changed, 18 insertions(+), 23 deletions(-)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] migration: All this fields are unsigned
2021-12-16 9:26 ` Philippe Mathieu-Daudé
@ 2021-12-17 20:25 ` Richard Henderson
2021-12-17 21:01 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 13+ messages in thread
From: Richard Henderson @ 2021-12-17 20:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Juan Quintela, qemu-devel
Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu
On 12/16/21 1:26 AM, Philippe Mathieu-Daudé wrote:
> On 12/16/21 10:13, Juan Quintela wrote:
>> multifd_send_terminate_threads(bool error) "error %d"
>
> Nitpicking: bool is unsigned :)
Eh, while the value is not negative, bool will promote to int in stdarg.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] migration: All this fields are unsigned
2021-12-17 20:25 ` Richard Henderson
@ 2021-12-17 21:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-17 21:01 UTC (permalink / raw)
To: Richard Henderson, Juan Quintela, qemu-devel
Cc: Leonardo Bras, Dr. David Alan Gilbert, Peter Xu
On 12/17/21 21:25, Richard Henderson wrote:
> On 12/16/21 1:26 AM, Philippe Mathieu-Daudé wrote:
>> On 12/16/21 10:13, Juan Quintela wrote:
>>> multifd_send_terminate_threads(bool error) "error %d"
>>
>> Nitpicking: bool is unsigned :)
>
> Eh, while the value is not negative, bool will promote to int in stdarg.
Oh, TIL, thanks :)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/5] migration: We only need last_stage in two places
2021-12-16 9:30 ` Philippe Mathieu-Daudé
@ 2021-12-20 19:02 ` Juan Quintela
0 siblings, 0 replies; 13+ messages in thread
From: Juan Quintela @ 2021-12-20 19:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Leonardo Bras, qemu-devel, Peter Xu, Dr. David Alan Gilbert
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 12/16/21 10:13, Juan Quintela wrote:
>> And we are passing it all around. Just add a field to RAMState that
>> passes it.
>
> Splitting half of the description in the subject makes it hard to read.
> Better to repeat the subject as first description line. Matter of taste.
I don't care enough to fight it.
Changed O:-)
Thanks, Juan.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument
2021-12-16 9:27 ` Philippe Mathieu-Daudé
@ 2021-12-21 9:35 ` Juan Quintela
0 siblings, 0 replies; 13+ messages in thread
From: Juan Quintela @ 2021-12-21 9:35 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Leonardo Bras, qemu-devel, Peter Xu, Dr. David Alan Gilbert
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 12/16/21 10:13, Juan Quintela wrote:
>> Remove the pages argument. And s/pages/page/
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> migration/ram.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>> -static void ram_release_pages(const char *rbname, uint64_t offset, int pages)
>> +static void ram_release_page(const char *rbname, uint64_t offset)
>> {
>> if (!migrate_release_ram() || !migration_in_postcopy()) {
>> return;
>> }
>>
>> - ram_discard_range(rbname, offset, ((ram_addr_t)pages) << TARGET_PAGE_BITS);
>> + ram_discard_range(rbname, offset, ((ram_addr_t)1) << TARGET_PAGE_BITS);
>
> 1ULL?
I am changing it, but the argument to ram_discard_range is a size_t, and
that is different in 32 bits arch. Once told that, it is not worse that
what we have here, as ram_addr_t type depends on the phase of the moon.
/* 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
Later, Juan.
PD. No, I don't know either why it is not casted to size_t.
PD2. And yes, I still think that pure int operations should be ok.
The value TARGET_PAGE_BITS more typical is 10, and here pages is
only used with value 1. C promotion rules should make everything
ok (famous last words).
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-12-21 9:38 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-16 9:13 [PATCH 0/5] migration: misc cleanups Juan Quintela
2021-12-16 9:13 ` [PATCH 1/5] migration: All this fields are unsigned Juan Quintela
2021-12-16 9:26 ` Philippe Mathieu-Daudé
2021-12-17 20:25 ` Richard Henderson
2021-12-17 21:01 ` Philippe Mathieu-Daudé
2021-12-16 9:13 ` [PATCH 2/5] migration: We only need last_stage in two places Juan Quintela
2021-12-16 9:30 ` Philippe Mathieu-Daudé
2021-12-20 19:02 ` Juan Quintela
2021-12-16 9:13 ` [PATCH 3/5] migration: ram_release_pages() always receive 1 page as argument Juan Quintela
2021-12-16 9:27 ` Philippe Mathieu-Daudé
2021-12-21 9:35 ` Juan Quintela
2021-12-16 9:13 ` [PATCH 4/5] migration: simplify do_compress_ram_page Juan Quintela
2021-12-16 9:13 ` [PATCH 5/5] migration: Move ram_release_pages() call to save_zero_page_to_file() Juan Quintela
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.