* [PATCH 0/7] migration: Postcopy cleanup on ram disgard
@ 2021-12-07 11:50 Peter Xu
2021-12-07 11:50 ` [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap() Peter Xu
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
Some queued patches for ram disgard cleanup, and some debug probes.
QEMU's ram disgard logic is probably a bit hard to predict because we send a
bunch of packets to notify the disgarded ranges rather than sending the bitmap.
The packets to send depending on the bitmap layout.
Initially I thought it could be a problem but in reality it's fine so far per
my initial measurement. So I'm flushing the cleanup/trace patches out because
I think they're still helpful.
Please have a look, thanks.
Peter Xu (7):
migration: Drop dead code of ram_debug_dump_bitmap()
migration: Don't return for postcopy_chunk_hostpages()
migration: Drop postcopy_chunk_hostpages()
migration: Do chunk page in postcopy_each_ram_send_discard()
migration: Drop return code for disgard ram process
migration: Dump sub-cmd name in loadvm_process_command tp
migration: Finer grained tracepoints for POSTCOPY_LISTEN
migration/migration.c | 5 +-
migration/ram.c | 103 ++++++-----------------------------------
migration/ram.h | 4 +-
migration/savevm.c | 7 ++-
migration/trace-events | 4 +-
5 files changed, 23 insertions(+), 100 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap()
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 16:05 ` Dr. David Alan Gilbert
2022-01-26 21:02 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages() Peter Xu
` (5 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
I planned to add "#ifdef DEBUG_POSTCOPY" around the function too because
otherwise it'll be compiled into qemu binary even if it'll never be used. Then
I found that maybe it's easier to just drop it for good..
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/ram.c | 39 ---------------------------------------
migration/ram.h | 2 --
2 files changed, 41 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 863035d235..756ac800a7 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2413,40 +2413,6 @@ static void ram_state_reset(RAMState *rs)
#define MAX_WAIT 50 /* ms, half buffered_file limit */
-/*
- * 'expected' is the value you expect the bitmap mostly to be full
- * of; it won't bother printing lines that are all this value.
- * If 'todump' is null the migration bitmap is dumped.
- */
-void ram_debug_dump_bitmap(unsigned long *todump, bool expected,
- unsigned long pages)
-{
- int64_t cur;
- int64_t linelen = 128;
- char linebuf[129];
-
- for (cur = 0; cur < pages; cur += linelen) {
- int64_t curb;
- bool found = false;
- /*
- * Last line; catch the case where the line length
- * is longer than remaining ram
- */
- if (cur + linelen > pages) {
- linelen = pages - cur;
- }
- for (curb = 0; curb < linelen; curb++) {
- bool thisbit = test_bit(cur + curb, todump);
- linebuf[curb] = thisbit ? '1' : '.';
- found = found || (thisbit != expected);
- }
- if (found) {
- linebuf[curb] = '\0';
- fprintf(stderr, "0x%08" PRIx64 " : %s\n", cur, linebuf);
- }
- }
-}
-
/* **** functions for postcopy ***** */
void ram_postcopy_migrated_memory_release(MigrationState *ms)
@@ -2674,11 +2640,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
if (ret) {
return ret;
}
-
-#ifdef DEBUG_POSTCOPY
- ram_debug_dump_bitmap(block->bmap, true,
- block->used_length >> TARGET_PAGE_BITS);
-#endif
}
trace_ram_postcopy_send_discard_bitmap();
diff --git a/migration/ram.h b/migration/ram.h
index c515396a9a..f543e25765 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -55,8 +55,6 @@ void mig_throttle_counter_reset(void);
uint64_t ram_pagesize_summary(void);
int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
void acct_update_position(QEMUFile *f, size_t size, bool zero);
-void ram_debug_dump_bitmap(unsigned long *todump, bool expected,
- unsigned long pages);
void ram_postcopy_migrated_memory_release(MigrationState *ms);
/* For outgoing discard bitmap */
int ram_postcopy_send_discard_bitmap(MigrationState *ms);
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages()
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
2021-12-07 11:50 ` [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap() Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 16:28 ` Dr. David Alan Gilbert
2022-01-26 21:03 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 3/7] migration: Drop postcopy_chunk_hostpages() Peter Xu
` (4 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
It always return zero, because it just can't go wrong so far. Simplify the
code with no functional change.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/ram.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 756ac800a7..fb8c1a887e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2585,12 +2585,10 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
* dirty host-page size chunks as all dirty. In this case the host-page
* is the host-page for the particular RAMBlock, i.e. it might be a huge page
*
- * Returns zero on success
- *
* @ms: current migration state
* @block: block we want to work with
*/
-static int postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
+static void postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
{
postcopy_discard_send_init(ms, block->idstr);
@@ -2600,7 +2598,6 @@ static int postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
postcopy_chunk_hostpages_pass(ms, block);
postcopy_discard_send_finish(ms);
- return 0;
}
/**
@@ -2622,7 +2619,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
{
RAMState *rs = ram_state;
RAMBlock *block;
- int ret;
RCU_READ_LOCK_GUARD();
@@ -2636,10 +2632,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
/* Deal with TPS != HPS and huge pages */
- ret = postcopy_chunk_hostpages(ms, block);
- if (ret) {
- return ret;
- }
+ postcopy_chunk_hostpages(ms, block);
}
trace_ram_postcopy_send_discard_bitmap();
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/7] migration: Drop postcopy_chunk_hostpages()
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
2021-12-07 11:50 ` [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap() Peter Xu
2021-12-07 11:50 ` [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages() Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 16:55 ` Dr. David Alan Gilbert
2022-01-26 21:14 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard() Peter Xu
` (3 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
This function calls three functions:
- postcopy_discard_send_init(ms, block->idstr);
- postcopy_chunk_hostpages_pass(ms, block);
- postcopy_discard_send_finish(ms);
However only the 2nd function call is meaningful. It's major role is to make
sure dirty bits are applied in host-page-size granule, so there will be no
partial dirty bits set for a whole host page if huge pages are used.
The 1st/3rd call are for latter when we want to send the disgard ranges.
They're mostly no-op here besides some tracepoints (which are misleading!).
Drop them, then we can directly drop postcopy_chunk_hostpages() as a whole
because we can call postcopy_chunk_hostpages_pass() directly.
There're still some nice comments above postcopy_chunk_hostpages() that explain
what it does. Copy it over to the caller's site.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/ram.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index fb8c1a887e..e3876181ab 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2576,30 +2576,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
}
}
-/**
- * postcopy_chunk_hostpages: discard any partially sent host page
- *
- * Utility for the outgoing postcopy code.
- *
- * Discard any partially sent host-page size chunks, mark any partially
- * dirty host-page size chunks as all dirty. In this case the host-page
- * is the host-page for the particular RAMBlock, i.e. it might be a huge page
- *
- * @ms: current migration state
- * @block: block we want to work with
- */
-static void postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
-{
- postcopy_discard_send_init(ms, block->idstr);
-
- /*
- * Ensure that all partially dirty host pages are made fully dirty.
- */
- postcopy_chunk_hostpages_pass(ms, block);
-
- postcopy_discard_send_finish(ms);
-}
-
/**
* ram_postcopy_send_discard_bitmap: transmit the discard bitmap
*
@@ -2631,8 +2607,13 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
rs->last_page = 0;
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
- /* Deal with TPS != HPS and huge pages */
- postcopy_chunk_hostpages(ms, block);
+ /*
+ * Deal with TPS != HPS and huge pages. It discard any partially sent
+ * host-page size chunks, mark any partially dirty host-page size
+ * chunks as all dirty. In this case the host-page is the host-page
+ * for the particular RAMBlock, i.e. it might be a huge page.
+ */
+ postcopy_chunk_hostpages_pass(ms, block);
}
trace_ram_postcopy_send_discard_bitmap();
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard()
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
` (2 preceding siblings ...)
2021-12-07 11:50 ` [PATCH 3/7] migration: Drop postcopy_chunk_hostpages() Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 17:39 ` Dr. David Alan Gilbert
2022-01-26 21:15 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 5/7] migration: Drop return code for disgard ram process Peter Xu
` (2 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
Right now we loop ramblocks for twice, the 1st time chunk the dirty bits with
huge page information; the 2nd time we send the discard ranges. That's not
necessary - we can do them in a single loop.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/ram.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index e3876181ab..ecc744d54d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2473,6 +2473,8 @@ static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
return 0;
}
+static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
+
/**
* postcopy_each_ram_send_discard: discard all RAMBlocks
*
@@ -2494,6 +2496,14 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
postcopy_discard_send_init(ms, block->idstr);
+ /*
+ * Deal with TPS != HPS and huge pages. It discard any partially sent
+ * host-page size chunks, mark any partially dirty host-page size
+ * chunks as all dirty. In this case the host-page is the host-page
+ * for the particular RAMBlock, i.e. it might be a huge page.
+ */
+ postcopy_chunk_hostpages_pass(ms, block);
+
/*
* Postcopy sends chunks of bitmap over the wire, but it
* just needs indexes at this point, avoids it having
@@ -2594,7 +2604,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
int ram_postcopy_send_discard_bitmap(MigrationState *ms)
{
RAMState *rs = ram_state;
- RAMBlock *block;
RCU_READ_LOCK_GUARD();
@@ -2606,15 +2615,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
rs->last_sent_block = NULL;
rs->last_page = 0;
- RAMBLOCK_FOREACH_NOT_IGNORED(block) {
- /*
- * Deal with TPS != HPS and huge pages. It discard any partially sent
- * host-page size chunks, mark any partially dirty host-page size
- * chunks as all dirty. In this case the host-page is the host-page
- * for the particular RAMBlock, i.e. it might be a huge page.
- */
- postcopy_chunk_hostpages_pass(ms, block);
- }
trace_ram_postcopy_send_discard_bitmap();
return postcopy_each_ram_send_discard(ms);
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/7] migration: Drop return code for disgard ram process
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
` (3 preceding siblings ...)
2021-12-07 11:50 ` [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard() Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 18:12 ` Dr. David Alan Gilbert
2022-01-26 21:17 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp Peter Xu
2021-12-07 11:50 ` [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN Peter Xu
6 siblings, 2 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
It will just never fail. Drop those return values where they're constantly
zeros.
A tiny touch-up on the tracepoint so trace_ram_postcopy_send_discard_bitmap()
is called after the logic itself (which sounds more reasonable).
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 5 +----
migration/ram.c | 20 +++++---------------
migration/ram.h | 2 +-
3 files changed, 7 insertions(+), 20 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index abaf6f9e3d..c2e5539721 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2983,10 +2983,7 @@ static int postcopy_start(MigrationState *ms)
* that are dirty
*/
if (migrate_postcopy_ram()) {
- if (ram_postcopy_send_discard_bitmap(ms)) {
- error_report("postcopy send discard bitmap failed");
- goto fail;
- }
+ ram_postcopy_send_discard_bitmap(ms);
}
/*
diff --git a/migration/ram.c b/migration/ram.c
index ecc744d54d..28f1ace0f7 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2478,8 +2478,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
/**
* postcopy_each_ram_send_discard: discard all RAMBlocks
*
- * Returns 0 for success or negative for error
- *
* Utility for the outgoing postcopy code.
* Calls postcopy_send_discard_bm_ram for each RAMBlock
* passing it bitmap indexes and name.
@@ -2488,10 +2486,9 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
*
* @ms: current migration state
*/
-static int postcopy_each_ram_send_discard(MigrationState *ms)
+static void postcopy_each_ram_send_discard(MigrationState *ms)
{
struct RAMBlock *block;
- int ret;
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
postcopy_discard_send_init(ms, block->idstr);
@@ -2509,14 +2506,9 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
* just needs indexes at this point, avoids it having
* target page specific code.
*/
- ret = postcopy_send_discard_bm_ram(ms, block);
+ postcopy_send_discard_bm_ram(ms, block);
postcopy_discard_send_finish(ms);
- if (ret) {
- return ret;
- }
}
-
- return 0;
}
/**
@@ -2589,8 +2581,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
/**
* ram_postcopy_send_discard_bitmap: transmit the discard bitmap
*
- * Returns zero on success
- *
* Transmit the set of pages to be discarded after precopy to the target
* these are pages that:
* a) Have been previously transmitted but are now dirty again
@@ -2601,7 +2591,7 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
*
* @ms: current migration state
*/
-int ram_postcopy_send_discard_bitmap(MigrationState *ms)
+void ram_postcopy_send_discard_bitmap(MigrationState *ms)
{
RAMState *rs = ram_state;
@@ -2615,9 +2605,9 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
rs->last_sent_block = NULL;
rs->last_page = 0;
- trace_ram_postcopy_send_discard_bitmap();
+ postcopy_each_ram_send_discard(ms);
- return postcopy_each_ram_send_discard(ms);
+ trace_ram_postcopy_send_discard_bitmap();
}
/**
diff --git a/migration/ram.h b/migration/ram.h
index f543e25765..2c6dc3675d 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -57,7 +57,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
void acct_update_position(QEMUFile *f, size_t size, bool zero);
void ram_postcopy_migrated_memory_release(MigrationState *ms);
/* For outgoing discard bitmap */
-int ram_postcopy_send_discard_bitmap(MigrationState *ms);
+void ram_postcopy_send_discard_bitmap(MigrationState *ms);
/* For incoming postcopy discard */
int ram_discard_range(const char *block_name, uint64_t start, size_t length);
int ram_postcopy_incoming_init(MigrationIncomingState *mis);
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
` (4 preceding siblings ...)
2021-12-07 11:50 ` [PATCH 5/7] migration: Drop return code for disgard ram process Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 18:41 ` Dr. David Alan Gilbert
2021-12-07 11:50 ` [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN Peter Xu
6 siblings, 1 reply; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
It'll be easier to read the name rather than index of sub-cmd when debugging.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/savevm.c | 2 +-
migration/trace-events | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index d59e976d50..17b8e25e00 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2271,7 +2271,7 @@ static int loadvm_process_command(QEMUFile *f)
return qemu_file_get_error(f);
}
- trace_loadvm_process_command(cmd, len);
+ trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
return -EINVAL;
diff --git a/migration/trace-events b/migration/trace-events
index b48d873b8a..d63a5915f5 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -22,7 +22,7 @@ loadvm_postcopy_handle_resume(void) ""
loadvm_postcopy_ram_handle_discard(void) ""
loadvm_postcopy_ram_handle_discard_end(void) ""
loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %ud"
-loadvm_process_command(uint16_t com, uint16_t len) "com=0x%x len=%d"
+loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d"
loadvm_process_command_ping(uint32_t val) "0x%x"
postcopy_ram_listen_thread_exit(void) ""
postcopy_ram_listen_thread_start(void) ""
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
` (5 preceding siblings ...)
2021-12-07 11:50 ` [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp Peter Xu
@ 2021-12-07 11:50 ` Peter Xu
2021-12-08 19:46 ` Dr. David Alan Gilbert
6 siblings, 1 reply; 23+ messages in thread
From: Peter Xu @ 2021-12-07 11:50 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Dr . David Alan Gilbert, peterx,
Leonardo Bras Soares Passos
The enablement of postcopy listening has a few steps, add a few tracepoints to
be there ready for some basic measurements for them.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/savevm.c | 5 ++++-
migration/trace-events | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 17b8e25e00..5b3f31eab2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1946,7 +1946,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
{
PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_LISTENING);
- trace_loadvm_postcopy_handle_listen();
+ trace_loadvm_postcopy_handle_listen(1);
Error *local_err = NULL;
if (ps != POSTCOPY_INCOMING_ADVISE && ps != POSTCOPY_INCOMING_DISCARD) {
@@ -1962,6 +1962,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
postcopy_ram_prepare_discard(mis);
}
}
+ trace_loadvm_postcopy_handle_listen(2);
/*
* Sensitise RAM - can now generate requests for blocks that don't exist
@@ -1974,6 +1975,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
return -1;
}
}
+ trace_loadvm_postcopy_handle_listen(3);
if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN, &local_err)) {
error_report_err(local_err);
@@ -1988,6 +1990,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
QEMU_THREAD_DETACHED);
qemu_sem_wait(&mis->listen_thread_sem);
qemu_sem_destroy(&mis->listen_thread_sem);
+ trace_loadvm_postcopy_handle_listen(4);
return 0;
}
diff --git a/migration/trace-events b/migration/trace-events
index d63a5915f5..1aa6937dc1 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -14,7 +14,7 @@ loadvm_handle_cmd_packaged_main(int ret) "%d"
loadvm_handle_cmd_packaged_received(int ret) "%d"
loadvm_handle_recv_bitmap(char *s) "%s"
loadvm_postcopy_handle_advise(void) ""
-loadvm_postcopy_handle_listen(void) ""
+loadvm_postcopy_handle_listen(int i) "%d"
loadvm_postcopy_handle_run(void) ""
loadvm_postcopy_handle_run_cpu_sync(void) ""
loadvm_postcopy_handle_run_vmstart(void) ""
--
2.32.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap()
2021-12-07 11:50 ` [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap() Peter Xu
@ 2021-12-08 16:05 ` Dr. David Alan Gilbert
2022-01-26 21:02 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 16:05 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> I planned to add "#ifdef DEBUG_POSTCOPY" around the function too because
> otherwise it'll be compiled into qemu binary even if it'll never be used. Then
> I found that maybe it's easier to just drop it for good..
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Yeh, it was useful debugging the bitmap in the early days.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Dave
> ---
> migration/ram.c | 39 ---------------------------------------
> migration/ram.h | 2 --
> 2 files changed, 41 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 863035d235..756ac800a7 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2413,40 +2413,6 @@ static void ram_state_reset(RAMState *rs)
>
> #define MAX_WAIT 50 /* ms, half buffered_file limit */
>
> -/*
> - * 'expected' is the value you expect the bitmap mostly to be full
> - * of; it won't bother printing lines that are all this value.
> - * If 'todump' is null the migration bitmap is dumped.
> - */
> -void ram_debug_dump_bitmap(unsigned long *todump, bool expected,
> - unsigned long pages)
> -{
> - int64_t cur;
> - int64_t linelen = 128;
> - char linebuf[129];
> -
> - for (cur = 0; cur < pages; cur += linelen) {
> - int64_t curb;
> - bool found = false;
> - /*
> - * Last line; catch the case where the line length
> - * is longer than remaining ram
> - */
> - if (cur + linelen > pages) {
> - linelen = pages - cur;
> - }
> - for (curb = 0; curb < linelen; curb++) {
> - bool thisbit = test_bit(cur + curb, todump);
> - linebuf[curb] = thisbit ? '1' : '.';
> - found = found || (thisbit != expected);
> - }
> - if (found) {
> - linebuf[curb] = '\0';
> - fprintf(stderr, "0x%08" PRIx64 " : %s\n", cur, linebuf);
> - }
> - }
> -}
> -
> /* **** functions for postcopy ***** */
>
> void ram_postcopy_migrated_memory_release(MigrationState *ms)
> @@ -2674,11 +2640,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> if (ret) {
> return ret;
> }
> -
> -#ifdef DEBUG_POSTCOPY
> - ram_debug_dump_bitmap(block->bmap, true,
> - block->used_length >> TARGET_PAGE_BITS);
> -#endif
> }
> trace_ram_postcopy_send_discard_bitmap();
>
> diff --git a/migration/ram.h b/migration/ram.h
> index c515396a9a..f543e25765 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -55,8 +55,6 @@ void mig_throttle_counter_reset(void);
> uint64_t ram_pagesize_summary(void);
> int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
> void acct_update_position(QEMUFile *f, size_t size, bool zero);
> -void ram_debug_dump_bitmap(unsigned long *todump, bool expected,
> - unsigned long pages);
> void ram_postcopy_migrated_memory_release(MigrationState *ms);
> /* For outgoing discard bitmap */
> int ram_postcopy_send_discard_bitmap(MigrationState *ms);
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages()
2021-12-07 11:50 ` [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages() Peter Xu
@ 2021-12-08 16:28 ` Dr. David Alan Gilbert
2022-01-26 21:03 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 16:28 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> It always return zero, because it just can't go wrong so far. Simplify the
> code with no functional change.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
OK, I was wondering if the discard_send_finish could fail, but I chased
it another 3 or 4 levels and nothing returns an error flag either.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/ram.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 756ac800a7..fb8c1a887e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2585,12 +2585,10 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
> * dirty host-page size chunks as all dirty. In this case the host-page
> * is the host-page for the particular RAMBlock, i.e. it might be a huge page
> *
> - * Returns zero on success
> - *
> * @ms: current migration state
> * @block: block we want to work with
> */
> -static int postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
> +static void postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
> {
> postcopy_discard_send_init(ms, block->idstr);
>
> @@ -2600,7 +2598,6 @@ static int postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
> postcopy_chunk_hostpages_pass(ms, block);
>
> postcopy_discard_send_finish(ms);
> - return 0;
> }
>
> /**
> @@ -2622,7 +2619,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> {
> RAMState *rs = ram_state;
> RAMBlock *block;
> - int ret;
>
> RCU_READ_LOCK_GUARD();
>
> @@ -2636,10 +2632,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
>
> RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> /* Deal with TPS != HPS and huge pages */
> - ret = postcopy_chunk_hostpages(ms, block);
> - if (ret) {
> - return ret;
> - }
> + postcopy_chunk_hostpages(ms, block);
> }
> trace_ram_postcopy_send_discard_bitmap();
>
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/7] migration: Drop postcopy_chunk_hostpages()
2021-12-07 11:50 ` [PATCH 3/7] migration: Drop postcopy_chunk_hostpages() Peter Xu
@ 2021-12-08 16:55 ` Dr. David Alan Gilbert
2022-01-26 21:14 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 16:55 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> This function calls three functions:
>
> - postcopy_discard_send_init(ms, block->idstr);
> - postcopy_chunk_hostpages_pass(ms, block);
> - postcopy_discard_send_finish(ms);
>
> However only the 2nd function call is meaningful. It's major role is to make
> sure dirty bits are applied in host-page-size granule, so there will be no
> partial dirty bits set for a whole host page if huge pages are used.
>
> The 1st/3rd call are for latter when we want to send the disgard ranges.
> They're mostly no-op here besides some tracepoints (which are misleading!).
>
> Drop them, then we can directly drop postcopy_chunk_hostpages() as a whole
> because we can call postcopy_chunk_hostpages_pass() directly.
>
> There're still some nice comments above postcopy_chunk_hostpages() that explain
> what it does. Copy it over to the caller's site.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Yeh, I think originally the idea was to send some of the messages during
the chunking
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/ram.c | 33 +++++++--------------------------
> 1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index fb8c1a887e..e3876181ab 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2576,30 +2576,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
> }
> }
>
> -/**
> - * postcopy_chunk_hostpages: discard any partially sent host page
> - *
> - * Utility for the outgoing postcopy code.
> - *
> - * Discard any partially sent host-page size chunks, mark any partially
> - * dirty host-page size chunks as all dirty. In this case the host-page
> - * is the host-page for the particular RAMBlock, i.e. it might be a huge page
> - *
> - * @ms: current migration state
> - * @block: block we want to work with
> - */
> -static void postcopy_chunk_hostpages(MigrationState *ms, RAMBlock *block)
> -{
> - postcopy_discard_send_init(ms, block->idstr);
> -
> - /*
> - * Ensure that all partially dirty host pages are made fully dirty.
> - */
> - postcopy_chunk_hostpages_pass(ms, block);
> -
> - postcopy_discard_send_finish(ms);
> -}
> -
> /**
> * ram_postcopy_send_discard_bitmap: transmit the discard bitmap
> *
> @@ -2631,8 +2607,13 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> rs->last_page = 0;
>
> RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> - /* Deal with TPS != HPS and huge pages */
> - postcopy_chunk_hostpages(ms, block);
> + /*
> + * Deal with TPS != HPS and huge pages. It discard any partially sent
> + * host-page size chunks, mark any partially dirty host-page size
> + * chunks as all dirty. In this case the host-page is the host-page
> + * for the particular RAMBlock, i.e. it might be a huge page.
> + */
> + postcopy_chunk_hostpages_pass(ms, block);
> }
> trace_ram_postcopy_send_discard_bitmap();
>
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard()
2021-12-07 11:50 ` [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard() Peter Xu
@ 2021-12-08 17:39 ` Dr. David Alan Gilbert
2022-01-26 21:15 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 17:39 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> Right now we loop ramblocks for twice, the 1st time chunk the dirty bits with
> huge page information; the 2nd time we send the discard ranges. That's not
> necessary - we can do them in a single loop.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/ram.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index e3876181ab..ecc744d54d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2473,6 +2473,8 @@ static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
> return 0;
> }
>
> +static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
> +
> /**
> * postcopy_each_ram_send_discard: discard all RAMBlocks
> *
> @@ -2494,6 +2496,14 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
> RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> postcopy_discard_send_init(ms, block->idstr);
>
> + /*
> + * Deal with TPS != HPS and huge pages. It discard any partially sent
> + * host-page size chunks, mark any partially dirty host-page size
> + * chunks as all dirty. In this case the host-page is the host-page
> + * for the particular RAMBlock, i.e. it might be a huge page.
> + */
> + postcopy_chunk_hostpages_pass(ms, block);
> +
> /*
> * Postcopy sends chunks of bitmap over the wire, but it
> * just needs indexes at this point, avoids it having
> @@ -2594,7 +2604,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
> int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> {
> RAMState *rs = ram_state;
> - RAMBlock *block;
>
> RCU_READ_LOCK_GUARD();
>
> @@ -2606,15 +2615,6 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> rs->last_sent_block = NULL;
> rs->last_page = 0;
>
> - RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> - /*
> - * Deal with TPS != HPS and huge pages. It discard any partially sent
> - * host-page size chunks, mark any partially dirty host-page size
> - * chunks as all dirty. In this case the host-page is the host-page
> - * for the particular RAMBlock, i.e. it might be a huge page.
> - */
> - postcopy_chunk_hostpages_pass(ms, block);
> - }
> trace_ram_postcopy_send_discard_bitmap();
>
> return postcopy_each_ram_send_discard(ms);
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/7] migration: Drop return code for disgard ram process
2021-12-07 11:50 ` [PATCH 5/7] migration: Drop return code for disgard ram process Peter Xu
@ 2021-12-08 18:12 ` Dr. David Alan Gilbert
2022-01-26 21:17 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 18:12 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> It will just never fail. Drop those return values where they're constantly
> zeros.
>
> A tiny touch-up on the tracepoint so trace_ram_postcopy_send_discard_bitmap()
> is called after the logic itself (which sounds more reasonable).
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/migration.c | 5 +----
> migration/ram.c | 20 +++++---------------
> migration/ram.h | 2 +-
> 3 files changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index abaf6f9e3d..c2e5539721 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2983,10 +2983,7 @@ static int postcopy_start(MigrationState *ms)
> * that are dirty
> */
> if (migrate_postcopy_ram()) {
> - if (ram_postcopy_send_discard_bitmap(ms)) {
> - error_report("postcopy send discard bitmap failed");
> - goto fail;
> - }
> + ram_postcopy_send_discard_bitmap(ms);
> }
>
> /*
> diff --git a/migration/ram.c b/migration/ram.c
> index ecc744d54d..28f1ace0f7 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2478,8 +2478,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
> /**
> * postcopy_each_ram_send_discard: discard all RAMBlocks
> *
> - * Returns 0 for success or negative for error
> - *
> * Utility for the outgoing postcopy code.
> * Calls postcopy_send_discard_bm_ram for each RAMBlock
> * passing it bitmap indexes and name.
> @@ -2488,10 +2486,9 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
> *
> * @ms: current migration state
> */
> -static int postcopy_each_ram_send_discard(MigrationState *ms)
> +static void postcopy_each_ram_send_discard(MigrationState *ms)
> {
> struct RAMBlock *block;
> - int ret;
>
> RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> postcopy_discard_send_init(ms, block->idstr);
> @@ -2509,14 +2506,9 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
> * just needs indexes at this point, avoids it having
> * target page specific code.
> */
> - ret = postcopy_send_discard_bm_ram(ms, block);
> + postcopy_send_discard_bm_ram(ms, block);
> postcopy_discard_send_finish(ms);
> - if (ret) {
> - return ret;
> - }
> }
> -
> - return 0;
> }
>
> /**
> @@ -2589,8 +2581,6 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
> /**
> * ram_postcopy_send_discard_bitmap: transmit the discard bitmap
> *
> - * Returns zero on success
> - *
> * Transmit the set of pages to be discarded after precopy to the target
> * these are pages that:
> * a) Have been previously transmitted but are now dirty again
> @@ -2601,7 +2591,7 @@ static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block)
> *
> * @ms: current migration state
> */
> -int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> +void ram_postcopy_send_discard_bitmap(MigrationState *ms)
> {
> RAMState *rs = ram_state;
>
> @@ -2615,9 +2605,9 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
> rs->last_sent_block = NULL;
> rs->last_page = 0;
>
> - trace_ram_postcopy_send_discard_bitmap();
> + postcopy_each_ram_send_discard(ms);
>
> - return postcopy_each_ram_send_discard(ms);
> + trace_ram_postcopy_send_discard_bitmap();
> }
>
> /**
> diff --git a/migration/ram.h b/migration/ram.h
> index f543e25765..2c6dc3675d 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -57,7 +57,7 @@ int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
> void acct_update_position(QEMUFile *f, size_t size, bool zero);
> void ram_postcopy_migrated_memory_release(MigrationState *ms);
> /* For outgoing discard bitmap */
> -int ram_postcopy_send_discard_bitmap(MigrationState *ms);
> +void ram_postcopy_send_discard_bitmap(MigrationState *ms);
> /* For incoming postcopy discard */
> int ram_discard_range(const char *block_name, uint64_t start, size_t length);
> int ram_postcopy_incoming_init(MigrationIncomingState *mis);
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp
2021-12-07 11:50 ` [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp Peter Xu
@ 2021-12-08 18:41 ` Dr. David Alan Gilbert
2021-12-09 1:44 ` Peter Xu
0 siblings, 1 reply; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 18:41 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> It'll be easier to read the name rather than index of sub-cmd when debugging.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/savevm.c | 2 +-
> migration/trace-events | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d59e976d50..17b8e25e00 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2271,7 +2271,7 @@ static int loadvm_process_command(QEMUFile *f)
> return qemu_file_get_error(f);
> }
>
> - trace_loadvm_process_command(cmd, len);
> + trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
> if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
No! you can't do that name lookup before the limit check.
Dave
> error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> return -EINVAL;
> diff --git a/migration/trace-events b/migration/trace-events
> index b48d873b8a..d63a5915f5 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -22,7 +22,7 @@ loadvm_postcopy_handle_resume(void) ""
> loadvm_postcopy_ram_handle_discard(void) ""
> loadvm_postcopy_ram_handle_discard_end(void) ""
> loadvm_postcopy_ram_handle_discard_header(const char *ramid, uint16_t len) "%s: %ud"
> -loadvm_process_command(uint16_t com, uint16_t len) "com=0x%x len=%d"
> +loadvm_process_command(const char *s, uint16_t len) "com=%s len=%d"
> loadvm_process_command_ping(uint32_t val) "0x%x"
> postcopy_ram_listen_thread_exit(void) ""
> postcopy_ram_listen_thread_start(void) ""
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN
2021-12-07 11:50 ` [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN Peter Xu
@ 2021-12-08 19:46 ` Dr. David Alan Gilbert
2021-12-09 1:54 ` Peter Xu
0 siblings, 1 reply; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-08 19:46 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> The enablement of postcopy listening has a few steps, add a few tracepoints to
> be there ready for some basic measurements for them.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/savevm.c | 5 ++++-
> migration/trace-events | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 17b8e25e00..5b3f31eab2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1946,7 +1946,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> {
> PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_LISTENING);
> - trace_loadvm_postcopy_handle_listen();
> + trace_loadvm_postcopy_handle_listen(1);
I think we tend just to split this into separate traces in many places;
or if we're using the same one then we should use a string
I'd make this:
trace_loadvm_postcopy_handle_listen_entry();
for example.
> Error *local_err = NULL;
>
> if (ps != POSTCOPY_INCOMING_ADVISE && ps != POSTCOPY_INCOMING_DISCARD) {
> @@ -1962,6 +1962,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> postcopy_ram_prepare_discard(mis);
> }
> }
> + trace_loadvm_postcopy_handle_listen(2);
>
> /*
> * Sensitise RAM - can now generate requests for blocks that don't exist
> @@ -1974,6 +1975,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> return -1;
> }
> }
> + trace_loadvm_postcopy_handle_listen(3);
>
> if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN, &local_err)) {
> error_report_err(local_err);
> @@ -1988,6 +1990,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> QEMU_THREAD_DETACHED);
> qemu_sem_wait(&mis->listen_thread_sem);
> qemu_sem_destroy(&mis->listen_thread_sem);
> + trace_loadvm_postcopy_handle_listen(4);
trace_loadvm_postcopy_handle_listen_entry_end();
>
> return 0;
> }
> diff --git a/migration/trace-events b/migration/trace-events
> index d63a5915f5..1aa6937dc1 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -14,7 +14,7 @@ loadvm_handle_cmd_packaged_main(int ret) "%d"
> loadvm_handle_cmd_packaged_received(int ret) "%d"
> loadvm_handle_recv_bitmap(char *s) "%s"
> loadvm_postcopy_handle_advise(void) ""
> -loadvm_postcopy_handle_listen(void) ""
> +loadvm_postcopy_handle_listen(int i) "%d"
> loadvm_postcopy_handle_run(void) ""
> loadvm_postcopy_handle_run_cpu_sync(void) ""
> loadvm_postcopy_handle_run_vmstart(void) ""
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp
2021-12-08 18:41 ` Dr. David Alan Gilbert
@ 2021-12-09 1:44 ` Peter Xu
0 siblings, 0 replies; 23+ messages in thread
From: Peter Xu @ 2021-12-09 1:44 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
On Wed, Dec 08, 2021 at 06:41:22PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > It'll be easier to read the name rather than index of sub-cmd when debugging.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > migration/savevm.c | 2 +-
> > migration/trace-events | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index d59e976d50..17b8e25e00 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2271,7 +2271,7 @@ static int loadvm_process_command(QEMUFile *f)
> > return qemu_file_get_error(f);
> > }
> >
> > - trace_loadvm_process_command(cmd, len);
> > + trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
> > if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
>
> No! you can't do that name lookup before the limit check.
Heh, right. :)
I guess it shouldn't matter in reality as we don't worry too much on untrusted
or uncompatible src qemu, but it's very reasonable to fix it.
Thanks!
--
Peter Xu
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN
2021-12-08 19:46 ` Dr. David Alan Gilbert
@ 2021-12-09 1:54 ` Peter Xu
2021-12-09 9:04 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 23+ messages in thread
From: Peter Xu @ 2021-12-09 1:54 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
On Wed, Dec 08, 2021 at 07:46:20PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > The enablement of postcopy listening has a few steps, add a few tracepoints to
> > be there ready for some basic measurements for them.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > migration/savevm.c | 5 ++++-
> > migration/trace-events | 2 +-
> > 2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 17b8e25e00..5b3f31eab2 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1946,7 +1946,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > {
> > PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_LISTENING);
> > - trace_loadvm_postcopy_handle_listen();
> > + trace_loadvm_postcopy_handle_listen(1);
>
> I think we tend just to split this into separate traces in many places;
> or if we're using the same one then we should use a string
>
> I'd make this:
> trace_loadvm_postcopy_handle_listen_entry();
>
> for example.
>
> > Error *local_err = NULL;
> >
> > if (ps != POSTCOPY_INCOMING_ADVISE && ps != POSTCOPY_INCOMING_DISCARD) {
> > @@ -1962,6 +1962,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > postcopy_ram_prepare_discard(mis);
> > }
> > }
> > + trace_loadvm_postcopy_handle_listen(2);
> >
> > /*
> > * Sensitise RAM - can now generate requests for blocks that don't exist
> > @@ -1974,6 +1975,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > return -1;
> > }
> > }
> > + trace_loadvm_postcopy_handle_listen(3);
> >
> > if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN, &local_err)) {
> > error_report_err(local_err);
> > @@ -1988,6 +1990,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > QEMU_THREAD_DETACHED);
> > qemu_sem_wait(&mis->listen_thread_sem);
> > qemu_sem_destroy(&mis->listen_thread_sem);
> > + trace_loadvm_postcopy_handle_listen(4);
>
> trace_loadvm_postcopy_handle_listen_entry_end();
I see, I can use it. It's just that I want to trap more than entry/exit.
For the "4 steps" here I split it into four procedures, the 2 steps inside are
majorly to trap either uffd registration time or external uffd handling of
other processes.
One example:
We may want to know how slow is postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN)
when there're some external process attached. I wanted to be prepared for that
so when there's need to evaluate slowness of this procedure with vhost-user
enabled we have some tracepoints without replacing the binaries.
It's easy to use strings too if that's better looking than numbers. How about:
trace_loadvm_postcopy_handle_listen("entry")
trace_loadvm_postcopy_handle_listen("uffd-reg")
trace_loadvm_postcopy_handle_listen("external")
trace_loadvm_postcopy_handle_listen("exit")
?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN
2021-12-09 1:54 ` Peter Xu
@ 2021-12-09 9:04 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2021-12-09 9:04 UTC (permalink / raw)
To: Peter Xu; +Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos
* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Dec 08, 2021 at 07:46:20PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > The enablement of postcopy listening has a few steps, add a few tracepoints to
> > > be there ready for some basic measurements for them.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > migration/savevm.c | 5 ++++-
> > > migration/trace-events | 2 +-
> > > 2 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 17b8e25e00..5b3f31eab2 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -1946,7 +1946,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > > static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > > {
> > > PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_LISTENING);
> > > - trace_loadvm_postcopy_handle_listen();
> > > + trace_loadvm_postcopy_handle_listen(1);
> >
> > I think we tend just to split this into separate traces in many places;
> > or if we're using the same one then we should use a string
> >
> > I'd make this:
> > trace_loadvm_postcopy_handle_listen_entry();
> >
> > for example.
> >
> > > Error *local_err = NULL;
> > >
> > > if (ps != POSTCOPY_INCOMING_ADVISE && ps != POSTCOPY_INCOMING_DISCARD) {
> > > @@ -1962,6 +1962,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > > postcopy_ram_prepare_discard(mis);
> > > }
> > > }
> > > + trace_loadvm_postcopy_handle_listen(2);
> > >
> > > /*
> > > * Sensitise RAM - can now generate requests for blocks that don't exist
> > > @@ -1974,6 +1975,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > > return -1;
> > > }
> > > }
> > > + trace_loadvm_postcopy_handle_listen(3);
> > >
> > > if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN, &local_err)) {
> > > error_report_err(local_err);
> > > @@ -1988,6 +1990,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > > QEMU_THREAD_DETACHED);
> > > qemu_sem_wait(&mis->listen_thread_sem);
> > > qemu_sem_destroy(&mis->listen_thread_sem);
> > > + trace_loadvm_postcopy_handle_listen(4);
> >
> > trace_loadvm_postcopy_handle_listen_entry_end();
>
> I see, I can use it. It's just that I want to trap more than entry/exit.
>
> For the "4 steps" here I split it into four procedures, the 2 steps inside are
> majorly to trap either uffd registration time or external uffd handling of
> other processes.
>
> One example:
>
> We may want to know how slow is postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN)
> when there're some external process attached. I wanted to be prepared for that
> so when there's need to evaluate slowness of this procedure with vhost-user
> enabled we have some tracepoints without replacing the binaries.
>
> It's easy to use strings too if that's better looking than numbers. How about:
>
> trace_loadvm_postcopy_handle_listen("entry")
> trace_loadvm_postcopy_handle_listen("uffd-reg")
> trace_loadvm_postcopy_handle_listen("external")
> trace_loadvm_postcopy_handle_listen("exit")
>
Yes, that's fine; but it would also be fine to create 4 separate traces.
Dave
> ?
>
> Thanks,
>
> --
> Peter Xu
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap()
2021-12-07 11:50 ` [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap() Peter Xu
2021-12-08 16:05 ` Dr. David Alan Gilbert
@ 2022-01-26 21:02 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2022-01-26 21:02 UTC (permalink / raw)
To: Peter Xu; +Cc: Leonardo Bras Soares Passos, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> wrote:
> I planned to add "#ifdef DEBUG_POSTCOPY" around the function too because
> otherwise it'll be compiled into qemu binary even if it'll never be used. Then
> I found that maybe it's easier to just drop it for good..
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
queued
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages()
2021-12-07 11:50 ` [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages() Peter Xu
2021-12-08 16:28 ` Dr. David Alan Gilbert
@ 2022-01-26 21:03 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2022-01-26 21:03 UTC (permalink / raw)
To: Peter Xu; +Cc: Leonardo Bras Soares Passos, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> wrote:
> It always return zero, because it just can't go wrong so far. Simplify the
> code with no functional change.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
queued
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/7] migration: Drop postcopy_chunk_hostpages()
2021-12-07 11:50 ` [PATCH 3/7] migration: Drop postcopy_chunk_hostpages() Peter Xu
2021-12-08 16:55 ` Dr. David Alan Gilbert
@ 2022-01-26 21:14 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2022-01-26 21:14 UTC (permalink / raw)
To: Peter Xu; +Cc: Leonardo Bras Soares Passos, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> wrote:
> This function calls three functions:
>
> - postcopy_discard_send_init(ms, block->idstr);
> - postcopy_chunk_hostpages_pass(ms, block);
> - postcopy_discard_send_finish(ms);
>
> However only the 2nd function call is meaningful. It's major role is to make
> sure dirty bits are applied in host-page-size granule, so there will be no
> partial dirty bits set for a whole host page if huge pages are used.
>
> The 1st/3rd call are for latter when we want to send the disgard ranges.
> They're mostly no-op here besides some tracepoints (which are misleading!).
>
> Drop them, then we can directly drop postcopy_chunk_hostpages() as a whole
> because we can call postcopy_chunk_hostpages_pass() directly.
>
> There're still some nice comments above postcopy_chunk_hostpages() that explain
> what it does. Copy it over to the caller's site.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
queued
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard()
2021-12-07 11:50 ` [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard() Peter Xu
2021-12-08 17:39 ` Dr. David Alan Gilbert
@ 2022-01-26 21:15 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2022-01-26 21:15 UTC (permalink / raw)
To: Peter Xu; +Cc: Leonardo Bras Soares Passos, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> wrote:
> Right now we loop ramblocks for twice, the 1st time chunk the dirty bits with
> huge page information; the 2nd time we send the discard ranges. That's not
> necessary - we can do them in a single loop.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
queued
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/7] migration: Drop return code for disgard ram process
2021-12-07 11:50 ` [PATCH 5/7] migration: Drop return code for disgard ram process Peter Xu
2021-12-08 18:12 ` Dr. David Alan Gilbert
@ 2022-01-26 21:17 ` Juan Quintela
1 sibling, 0 replies; 23+ messages in thread
From: Juan Quintela @ 2022-01-26 21:17 UTC (permalink / raw)
To: Peter Xu; +Cc: Leonardo Bras Soares Passos, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> wrote:
> It will just never fail. Drop those return values where they're constantly
> zeros.
>
> A tiny touch-up on the tracepoint so trace_ram_postcopy_send_discard_bitmap()
> is called after the logic itself (which sounds more reasonable).
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
queued
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-01-26 21:20 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-07 11:50 [PATCH 0/7] migration: Postcopy cleanup on ram disgard Peter Xu
2021-12-07 11:50 ` [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap() Peter Xu
2021-12-08 16:05 ` Dr. David Alan Gilbert
2022-01-26 21:02 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages() Peter Xu
2021-12-08 16:28 ` Dr. David Alan Gilbert
2022-01-26 21:03 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 3/7] migration: Drop postcopy_chunk_hostpages() Peter Xu
2021-12-08 16:55 ` Dr. David Alan Gilbert
2022-01-26 21:14 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard() Peter Xu
2021-12-08 17:39 ` Dr. David Alan Gilbert
2022-01-26 21:15 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 5/7] migration: Drop return code for disgard ram process Peter Xu
2021-12-08 18:12 ` Dr. David Alan Gilbert
2022-01-26 21:17 ` Juan Quintela
2021-12-07 11:50 ` [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp Peter Xu
2021-12-08 18:41 ` Dr. David Alan Gilbert
2021-12-09 1:44 ` Peter Xu
2021-12-07 11:50 ` [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN Peter Xu
2021-12-08 19:46 ` Dr. David Alan Gilbert
2021-12-09 1:54 ` Peter Xu
2021-12-09 9:04 ` Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).