[PATCH 0/7] migration: Postcopy cleanup on ram disgard

[PATCH 0/7] migration: Postcopy cleanup on ram disgard

[Peter Xu]

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

[PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap()

[Peter Xu]

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

[PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages()

[Peter Xu]

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

[PATCH 3/7] migration: Drop postcopy_chunk_hostpages()

[Peter Xu]

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

[PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard()

[Peter Xu]

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

[PATCH 5/7] migration: Drop return code for disgard ram process

[Peter Xu]

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

[PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp

[Peter Xu]

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

[PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN

[Peter Xu]

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

Re: [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap()

[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>

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

Re: [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages()

[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>

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

Re: [PATCH 3/7] migration: Drop postcopy_chunk_hostpages()

[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>

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

Re: [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard()

[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: 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

Re: [PATCH 5/7] migration: Drop return code for disgard ram process

[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: 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

Re: [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 6/7] migration: Dump sub-cmd name in loadvm_process_command tp

[Peter Xu]

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

Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN

[Peter Xu]

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

Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 1/7] migration: Drop dead code of ram_debug_dump_bitmap()

[Juan Quintela]

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

Re: [PATCH 2/7] migration: Don't return for postcopy_chunk_hostpages()

[Juan Quintela]

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

Re: [PATCH 3/7] migration: Drop postcopy_chunk_hostpages()

[Juan Quintela]

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

Re: [PATCH 4/7] migration: Do chunk page in postcopy_each_ram_send_discard()

[Juan Quintela]

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

Re: [PATCH 5/7] migration: Drop return code for disgard ram process

[Juan Quintela]

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