[PATCH RFC 00/10] migration: auto-converge refinements for huge VM

[PATCH RFC 00/10] migration: auto-converge refinements for huge VM

[Hyman Huang]

Currently, a huge VM with high memory overload may take a long time
to increase its maximum throttle percentage. The root cause is that
the current auto-converge throttle logic doesn't look like it will
scale because migration_trigger_throttle() is only called for each
iteration, so it won't be invoked for a long time if one iteration
can take a long time.

This patchset provides two refinements aiming at the above case.

1: The periodic CPU throttle. As Peter points out, "throttle only
   for each sync, sync for each iteration" may make sense in the
   old days, but perhaps not anymore. So we introduce perioidic
   CPU throttle implementation for migration, which is a trade-off
   between synchronization overhead and CPU throttle impact.

2: The responsive CPU throttle. We present new criteria called
   "dirty ratio" to help improve the detection accuracy and hence
   accelerate the throttle's invocation.

The RFC version of the refinement may be a rudimentary implementation,
I would appreciate hearing more feedback.

Yong, thanks.

Hyman Huang (10):
  migration: Introduce structs for periodic CPU throttle
  migration: Refine util functions to support periodic CPU throttle
  qapi/migration: Introduce periodic CPU throttling parameters
  qapi/migration: Introduce the iteration-count
  migration: Introduce util functions for periodic CPU throttle
  migration: Support periodic CPU throttle
  tests/migration-tests: Add test case for periodic throttle
  migration: Introduce cpu-responsive-throttle parameter
  migration: Support responsive CPU throttle
  tests/migration-tests: Add test case for responsive CPU throttle

 include/exec/ram_addr.h        | 117 ++++++++++++++++--
 include/exec/ramblock.h        |  45 +++++++
 migration/migration-hmp-cmds.c |  25 ++++
 migration/migration-stats.h    |   4 +
 migration/migration.c          |  12 ++
 migration/options.c            |  74 +++++++++++
 migration/options.h            |   3 +
 migration/ram.c                | 218 ++++++++++++++++++++++++++++++---
 migration/ram.h                |   4 +
 migration/trace-events         |   4 +
 qapi/migration.json            |  45 ++++++-
 tests/qtest/migration-test.c   |  77 +++++++++++-
 12 files changed, 600 insertions(+), 28 deletions(-)

-- 
2.39.1

[PATCH RFC 01/10] migration: Introduce structs for periodic CPU throttle

[Hyman Huang]

shadow_bmap, iter_bmap, iter_dirty_pages and
periodic_sync_shown_up are introduced to satisfy the need
for periodic CPU throttle.

Meanwhile, introduce enumeration of dirty bitmap sync method.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 include/exec/ramblock.h | 45 +++++++++++++++++++++++++++++++++++++++++
 migration/ram.c         |  6 ++++++
 2 files changed, 51 insertions(+)

diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
index 0babd105c0..619c52885a 100644
--- a/include/exec/ramblock.h
+++ b/include/exec/ramblock.h
@@ -24,6 +24,30 @@
 #include "qemu/rcu.h"
 #include "exec/ramlist.h"
 
+/* Possible bits for migration_bitmap_sync */
+
+/*
+ * The old-fashioned sync method, which is, in turn, used for CPU
+ * throttle and memory transfer.
+ */
+#define RAMBLOCK_SYN_LEGACY_ITER    (1U << 0)
+
+/*
+ * The modern sync method, which is, in turn, used for CPU throttle
+ * and memory transfer.
+ */
+#define RAMBLOCK_SYN_MODERN_ITER    (1U << 1)
+
+/* The modern sync method, which is used for CPU throttle only */
+#define RAMBLOCK_SYN_MODERN_PERIOD  (1U << 2)
+
+#define RAMBLOCK_SYN_MASK   (0x7)
+
+typedef enum RAMBlockSynMode {
+    RAMBLOCK_SYN_LEGACY,    /* Old-fashined mode */
+    RAMBLOCK_SYN_MODERN,
+} RAMBlockSynMode;
+
 struct RAMBlock {
     struct rcu_head rcu;
     struct MemoryRegion *mr;
@@ -89,6 +113,27 @@ struct RAMBlock {
      * could not have been valid on the source.
      */
     ram_addr_t postcopy_length;
+
+    /*
+     * Used to backup the bmap during periodic sync to see whether any dirty
+     * pages were sent during that time.
+     */
+    unsigned long *shadow_bmap;
+
+    /*
+     * The bitmap "bmap," which was initially used for both sync and memory
+     * transfer, will be replaced by two bitmaps: the previously used "bmap"
+     * and the recently added "iter_bmap." Only the memory transfer is
+     * conducted with the previously used "bmap"; the recently added
+     * "iter_bmap" is utilized for sync.
+     */
+    unsigned long *iter_bmap;
+
+    /* Number of new dirty pages during iteration */
+    uint64_t iter_dirty_pages;
+
+    /* If periodic sync has shown up during iteration */
+    bool periodic_sync_shown_up;
 };
 #endif
 #endif
diff --git a/migration/ram.c b/migration/ram.c
index 67ca3d5d51..f29faa82d6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2362,6 +2362,10 @@ static void ram_bitmaps_destroy(void)
         block->bmap = NULL;
         g_free(block->file_bmap);
         block->file_bmap = NULL;
+        g_free(block->shadow_bmap);
+        block->shadow_bmap = NULL;
+        g_free(block->iter_bmap);
+        block->iter_bmap = NULL;
     }
 }
 
@@ -2753,6 +2757,8 @@ static void ram_list_init_bitmaps(void)
             }
             block->clear_bmap_shift = shift;
             block->clear_bmap = bitmap_new(clear_bmap_size(pages, shift));
+            block->shadow_bmap = bitmap_new(pages);
+            block->iter_bmap = bitmap_new(pages);
         }
     }
 }
-- 
2.39.1

[PATCH RFC 02/10] migration: Refine util functions to support periodic CPU throttle

[Hyman Huang]

Supply the migration_bitmap_sync function along with a periodic
argument. Introduce the sync_mode global variable to track the
sync mode and support periodic throttling while keeping backward
compatibility.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 include/exec/ram_addr.h | 117 ++++++++++++++++++++++++++++++++++++----
 migration/ram.c         |  49 +++++++++++++----
 2 files changed, 147 insertions(+), 19 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 891c44cf2d..43fa4d7b18 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -472,17 +472,68 @@ static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start,
     cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE);
 }
 
+static void ramblock_clear_iter_bmap(RAMBlock *rb,
+                                     ram_addr_t start,
+                                     ram_addr_t length)
+{
+    ram_addr_t addr;
+    unsigned long *bmap = rb->bmap;
+    unsigned long *shadow_bmap = rb->shadow_bmap;
+    unsigned long *iter_bmap = rb->iter_bmap;
+
+    for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
+        long k = (start + addr) >> TARGET_PAGE_BITS;
+        if (test_bit(k, shadow_bmap) && !test_bit(k, bmap)) {
+            /* Page has been sent, clear the iter bmap */
+            clear_bit(k, iter_bmap);
+        }
+    }
+}
+
+static void ramblock_update_iter_bmap(RAMBlock *rb,
+                                      ram_addr_t start,
+                                      ram_addr_t length)
+{
+    ram_addr_t addr;
+    unsigned long *bmap = rb->bmap;
+    unsigned long *iter_bmap = rb->iter_bmap;
+
+    for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
+        long k = (start + addr) >> TARGET_PAGE_BITS;
+        if (test_bit(k, iter_bmap)) {
+            if (!test_bit(k, bmap)) {
+                set_bit(k, bmap);
+                rb->iter_dirty_pages++;
+            }
+        }
+    }
+}
 
 /* Called with RCU critical section */
 static inline
 uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
                                                ram_addr_t start,
-                                               ram_addr_t length)
+                                               ram_addr_t length,
+                                               unsigned int flag)
 {
     ram_addr_t addr;
     unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
     uint64_t num_dirty = 0;
     unsigned long *dest = rb->bmap;
+    unsigned long *shadow_bmap = rb->shadow_bmap;
+    unsigned long *iter_bmap = rb->iter_bmap;
+
+    assert(flag && !(flag & (~RAMBLOCK_SYN_MASK)));
+
+    /*
+     * We must remove the sent dirty page from the iter_bmap in order to
+     * minimize redundant page transfers if periodic sync has appeared
+     * during this iteration.
+     */
+    if (rb->periodic_sync_shown_up &&
+        (flag & (RAMBLOCK_SYN_MODERN_ITER | RAMBLOCK_SYN_MODERN_PERIOD))) {
+        ramblock_clear_iter_bmap(rb, start, length);
+    }
 
     /* start address and length is aligned at the start of a word? */
     if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
@@ -503,8 +554,20 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
             if (src[idx][offset]) {
                 unsigned long bits = qatomic_xchg(&src[idx][offset], 0);
                 unsigned long new_dirty;
+                if (flag & (RAMBLOCK_SYN_MODERN_ITER |
+                            RAMBLOCK_SYN_MODERN_PERIOD)) {
+                    /* Back-up bmap for the next iteration */
+                    iter_bmap[k] |= bits;
+                    if (flag == RAMBLOCK_SYN_MODERN_PERIOD) {
+                        /* Back-up bmap to detect pages has been sent */
+                        shadow_bmap[k] = dest[k];
+                    }
+                }
                 new_dirty = ~dest[k];
-                dest[k] |= bits;
+                if (flag == RAMBLOCK_SYN_LEGACY_ITER) {
+                    dest[k] |= bits;
+                }
+
                 new_dirty &= bits;
                 num_dirty += ctpopl(new_dirty);
             }
@@ -534,18 +597,54 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
         ram_addr_t offset = rb->offset;
 
         for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) {
-            if (cpu_physical_memory_test_and_clear_dirty(
-                        start + addr + offset,
-                        TARGET_PAGE_SIZE,
-                        DIRTY_MEMORY_MIGRATION)) {
-                long k = (start + addr) >> TARGET_PAGE_BITS;
-                if (!test_and_set_bit(k, dest)) {
-                    num_dirty++;
+            long k = (start + addr) >> TARGET_PAGE_BITS;
+            if (flag == RAMBLOCK_SYN_MODERN_PERIOD) {
+                if (test_bit(k, dest)) {
+                    /* Back-up bmap to detect pages has been sent */
+                    set_bit(k, shadow_bmap);
+                }
+            }
+
+            if (flag == RAMBLOCK_SYN_LEGACY_ITER) {
+                if (cpu_physical_memory_test_and_clear_dirty(
+                            start + addr + offset,
+                            TARGET_PAGE_SIZE,
+                            DIRTY_MEMORY_MIGRATION)) {
+                    if (!test_and_set_bit(k, dest)) {
+                        num_dirty++;
+                    }
+                }
+            } else {
+                if (cpu_physical_memory_test_and_clear_dirty(
+                            start + addr + offset,
+                            TARGET_PAGE_SIZE,
+                            DIRTY_MEMORY_MIGRATION)) {
+                    if (!test_bit(k, dest)) {
+                        num_dirty++;
+                    }
+                    /* Back-up bmap for the next iteration */
+                    set_bit(k, iter_bmap);
                 }
             }
         }
     }
 
+    /*
+     * If periodic sync has emerged, we have to resync every dirty
+     * page from the iter_bmap one by one. It's possible that not
+     * all of the dirty pages that this iteration is meant to send
+     * are included in the bitmap that the current sync retrieved
+     * from the KVM.
+     */
+    if (rb->periodic_sync_shown_up &&
+        (flag == RAMBLOCK_SYN_MODERN_ITER)) {
+        ramblock_update_iter_bmap(rb, start, length);
+    }
+
+    if (flag == RAMBLOCK_SYN_MODERN_PERIOD) {
+        rb->periodic_sync_shown_up = true;
+    }
+
     return num_dirty;
 }
 #endif
diff --git a/migration/ram.c b/migration/ram.c
index f29faa82d6..a56634eb46 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -112,6 +112,8 @@
 
 XBZRLECacheStats xbzrle_counters;
 
+static RAMBlockSynMode sync_mode = RAMBLOCK_SYN_LEGACY;
+
 /* used by the search for pages to send */
 struct PageSearchStatus {
     /* The migration channel used for a specific host page */
@@ -912,13 +914,38 @@ bool ramblock_page_is_discarded(RAMBlock *rb, ram_addr_t start)
     return false;
 }
 
+static void ramblock_reset_iter_stats(RAMBlock *rb)
+{
+    bitmap_clear(rb->shadow_bmap, 0, rb->used_length >> TARGET_PAGE_BITS);
+    bitmap_clear(rb->iter_bmap, 0, rb->used_length >> TARGET_PAGE_BITS);
+    rb->iter_dirty_pages = 0;
+    rb->periodic_sync_shown_up = false;
+}
+
 /* Called with RCU critical section */
-static void ramblock_sync_dirty_bitmap(RAMState *rs, RAMBlock *rb)
+static void ramblock_sync_dirty_bitmap(RAMState *rs,
+                                       RAMBlock *rb,
+                                       bool periodic)
 {
-    uint64_t new_dirty_pages =
-        cpu_physical_memory_sync_dirty_bitmap(rb, 0, rb->used_length);
+    uint64_t new_dirty_pages;
+    unsigned int flag = RAMBLOCK_SYN_LEGACY_ITER;
+
+    if (sync_mode == RAMBLOCK_SYN_MODERN) {
+        flag = periodic ? RAMBLOCK_SYN_MODERN_PERIOD : RAMBLOCK_SYN_MODERN_ITER;
+    }
+
+    new_dirty_pages =
+        cpu_physical_memory_sync_dirty_bitmap(rb, 0, rb->used_length, flag);
+
+    if (flag & (RAMBLOCK_SYN_LEGACY_ITER | RAMBLOCK_SYN_MODERN_ITER)) {
+        if (flag == RAMBLOCK_SYN_LEGACY_ITER) {
+            rs->migration_dirty_pages += new_dirty_pages;
+        } else {
+            rs->migration_dirty_pages += rb->iter_dirty_pages;
+            ramblock_reset_iter_stats(rb);
+        }
+    }
 
-    rs->migration_dirty_pages += new_dirty_pages;
     rs->num_dirty_pages_period += new_dirty_pages;
 }
 
@@ -1041,7 +1068,9 @@ static void migration_trigger_throttle(RAMState *rs)
     }
 }
 
-static void migration_bitmap_sync(RAMState *rs, bool last_stage)
+static void migration_bitmap_sync(RAMState *rs,
+                                  bool last_stage,
+                                  bool periodic)
 {
     RAMBlock *block;
     int64_t end_time;
@@ -1058,7 +1087,7 @@ static void migration_bitmap_sync(RAMState *rs, bool last_stage)
     WITH_QEMU_LOCK_GUARD(&rs->bitmap_mutex) {
         WITH_RCU_READ_LOCK_GUARD() {
             RAMBLOCK_FOREACH_NOT_IGNORED(block) {
-                ramblock_sync_dirty_bitmap(rs, block);
+                ramblock_sync_dirty_bitmap(rs, block, periodic);
             }
             stat64_set(&mig_stats.dirty_bytes_last_sync, ram_bytes_remaining());
         }
@@ -1101,7 +1130,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs, bool last_stage)
         local_err = NULL;
     }
 
-    migration_bitmap_sync(rs, last_stage);
+    migration_bitmap_sync(rs, last_stage, false);
 
     if (precopy_notify(PRECOPY_NOTIFY_AFTER_BITMAP_SYNC, &local_err)) {
         error_report_err(local_err);
@@ -2594,7 +2623,7 @@ void ram_postcopy_send_discard_bitmap(MigrationState *ms)
     RCU_READ_LOCK_GUARD();
 
     /* This should be our last sync, the src is now paused */
-    migration_bitmap_sync(rs, false);
+    migration_bitmap_sync(rs, false, false);
 
     /* Easiest way to make sure we don't resume in the middle of a host-page */
     rs->pss[RAM_CHANNEL_PRECOPY].last_sent_block = NULL;
@@ -3581,7 +3610,7 @@ void colo_incoming_start_dirty_log(void)
     memory_global_dirty_log_sync(false);
     WITH_RCU_READ_LOCK_GUARD() {
         RAMBLOCK_FOREACH_NOT_IGNORED(block) {
-            ramblock_sync_dirty_bitmap(ram_state, block);
+            ramblock_sync_dirty_bitmap(ram_state, block, false);
             /* Discard this dirty bitmap record */
             bitmap_zero(block->bmap, block->max_length >> TARGET_PAGE_BITS);
         }
@@ -3862,7 +3891,7 @@ void colo_flush_ram_cache(void)
     qemu_mutex_lock(&ram_state->bitmap_mutex);
     WITH_RCU_READ_LOCK_GUARD() {
         RAMBLOCK_FOREACH_NOT_IGNORED(block) {
-            ramblock_sync_dirty_bitmap(ram_state, block);
+            ramblock_sync_dirty_bitmap(ram_state, block, false);
         }
     }
 
-- 
2.39.1

[PATCH RFC 03/10] qapi/migration: Introduce periodic CPU throttling parameters

[Hyman Huang]

To activate the periodic CPU throttleing feature, introduce
the cpu-periodic-throttle.

To control the frequency of throttling, introduce the
cpu-periodic-throttle-interval.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/migration-hmp-cmds.c | 17 +++++++++++
 migration/options.c            | 54 ++++++++++++++++++++++++++++++++++
 migration/options.h            |  2 ++
 qapi/migration.json            | 25 +++++++++++++++-
 4 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 7d608d26e1..f7b8e06bb4 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -264,6 +264,15 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %s\n",
             MigrationParameter_str(MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW),
             params->cpu_throttle_tailslow ? "on" : "off");
+        assert(params->has_cpu_periodic_throttle);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_CPU_PERIODIC_THROTTLE),
+            params->cpu_periodic_throttle ? "on" : "off");
+        assert(params->has_cpu_periodic_throttle_interval);
+        monitor_printf(mon, "%s: %u\n",
+            MigrationParameter_str(
+                MIGRATION_PARAMETER_CPU_PERIODIC_THROTTLE_INTERVAL),
+            params->cpu_periodic_throttle_interval);
         assert(params->has_max_cpu_throttle);
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
@@ -512,6 +521,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_cpu_throttle_tailslow = true;
         visit_type_bool(v, param, &p->cpu_throttle_tailslow, &err);
         break;
+    case MIGRATION_PARAMETER_CPU_PERIODIC_THROTTLE:
+        p->has_cpu_periodic_throttle = true;
+        visit_type_bool(v, param, &p->cpu_periodic_throttle, &err);
+        break;
+    case MIGRATION_PARAMETER_CPU_PERIODIC_THROTTLE_INTERVAL:
+        p->has_cpu_periodic_throttle_interval = true;
+        visit_type_uint8(v, param, &p->cpu_periodic_throttle_interval, &err);
+        break;
     case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
         p->has_max_cpu_throttle = true;
         visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
diff --git a/migration/options.c b/migration/options.c
index 645f55003d..2dbe275ba0 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -44,6 +44,7 @@
 #define DEFAULT_MIGRATE_THROTTLE_TRIGGER_THRESHOLD 50
 #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20
 #define DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT 10
+#define DEFAULT_MIGRATE_CPU_PERIODIC_THROTTLE_INTERVAL 5
 #define DEFAULT_MIGRATE_MAX_CPU_THROTTLE 99
 
 /* Migration XBZRLE default cache size */
@@ -104,6 +105,11 @@ Property migration_properties[] = {
                       DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
     DEFINE_PROP_BOOL("x-cpu-throttle-tailslow", MigrationState,
                       parameters.cpu_throttle_tailslow, false),
+    DEFINE_PROP_BOOL("x-cpu-periodic-throttle", MigrationState,
+                      parameters.cpu_periodic_throttle, false),
+    DEFINE_PROP_UINT8("x-cpu-periodic-throttle-interval", MigrationState,
+                      parameters.cpu_periodic_throttle_interval,
+                      DEFAULT_MIGRATE_CPU_PERIODIC_THROTTLE_INTERVAL),
     DEFINE_PROP_SIZE("x-max-bandwidth", MigrationState,
                       parameters.max_bandwidth, MAX_THROTTLE),
     DEFINE_PROP_SIZE("avail-switchover-bandwidth", MigrationState,
@@ -695,6 +701,20 @@ uint8_t migrate_cpu_throttle_initial(void)
     return s->parameters.cpu_throttle_initial;
 }
 
+uint8_t migrate_periodic_throttle_interval(void)
+{
+    MigrationState *s = migrate_get_current();
+
+    return s->parameters.cpu_periodic_throttle_interval;
+}
+
+bool migrate_periodic_throttle(void)
+{
+    MigrationState *s = migrate_get_current();
+
+    return s->parameters.cpu_periodic_throttle;
+}
+
 bool migrate_cpu_throttle_tailslow(void)
 {
     MigrationState *s = migrate_get_current();
@@ -874,6 +894,11 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
     params->has_cpu_throttle_tailslow = true;
     params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
+    params->has_cpu_periodic_throttle = true;
+    params->cpu_periodic_throttle = s->parameters.cpu_periodic_throttle;
+    params->has_cpu_periodic_throttle_interval = true;
+    params->cpu_periodic_throttle_interval =
+        s->parameters.cpu_periodic_throttle_interval;
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     params->tls_hostname = g_strdup(s->parameters.tls_hostname);
     params->tls_authz = g_strdup(s->parameters.tls_authz ?
@@ -940,6 +965,8 @@ void migrate_params_init(MigrationParameters *params)
     params->has_cpu_throttle_initial = true;
     params->has_cpu_throttle_increment = true;
     params->has_cpu_throttle_tailslow = true;
+    params->has_cpu_periodic_throttle = true;
+    params->has_cpu_periodic_throttle_interval = true;
     params->has_max_bandwidth = true;
     params->has_downtime_limit = true;
     params->has_x_checkpoint_delay = true;
@@ -996,6 +1023,15 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
+    if (params->has_cpu_periodic_throttle_interval &&
+        (params->cpu_periodic_throttle_interval < 2 ||
+         params->cpu_periodic_throttle_interval > 10)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "cpu_periodic_throttle_interval",
+                   "an integer in the range of 2 to 10");
+        return false;
+    }
+
     if (params->has_max_bandwidth && (params->max_bandwidth > SIZE_MAX)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "max_bandwidth",
@@ -1163,6 +1199,15 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow;
     }
 
+    if (params->has_cpu_periodic_throttle) {
+        dest->cpu_periodic_throttle = params->cpu_periodic_throttle;
+    }
+
+    if (params->has_cpu_periodic_throttle_interval) {
+        dest->cpu_periodic_throttle_interval =
+            params->cpu_periodic_throttle_interval;
+    }
+
     if (params->tls_creds) {
         assert(params->tls_creds->type == QTYPE_QSTRING);
         dest->tls_creds = params->tls_creds->u.s;
@@ -1271,6 +1316,15 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.cpu_throttle_tailslow = params->cpu_throttle_tailslow;
     }
 
+    if (params->has_cpu_periodic_throttle) {
+        s->parameters.cpu_periodic_throttle = params->cpu_periodic_throttle;
+    }
+
+    if (params->has_cpu_periodic_throttle_interval) {
+        s->parameters.cpu_periodic_throttle_interval =
+            params->cpu_periodic_throttle_interval;
+    }
+
     if (params->tls_creds) {
         g_free(s->parameters.tls_creds);
         assert(params->tls_creds->type == QTYPE_QSTRING);
diff --git a/migration/options.h b/migration/options.h
index a2397026db..efeac01470 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -68,6 +68,8 @@ bool migrate_has_block_bitmap_mapping(void);
 uint32_t migrate_checkpoint_delay(void);
 uint8_t migrate_cpu_throttle_increment(void);
 uint8_t migrate_cpu_throttle_initial(void);
+uint8_t migrate_periodic_throttle_interval(void);
+bool migrate_periodic_throttle(void);
 bool migrate_cpu_throttle_tailslow(void);
 bool migrate_direct_io(void);
 uint64_t migrate_downtime_limit(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index 7324571e92..8281d4a83b 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -724,6 +724,12 @@
 #     be excessive at tail stage.  The default value is false.  (Since
 #     5.1)
 #
+# @cpu-periodic-throttle: Make CPU throttling periodically.
+#     (Since 9.1)
+#
+# @cpu-periodic-throttle-interval: Interval of the periodic CPU throttling.
+#     (Since 9.1)
+#
 # @tls-creds: ID of the 'tls-creds' object that provides credentials
 #     for establishing a TLS connection over the migration data
 #     channel.  On the outgoing side of the migration, the credentials
@@ -844,7 +850,8 @@
            'announce-rounds', 'announce-step',
            'throttle-trigger-threshold',
            'cpu-throttle-initial', 'cpu-throttle-increment',
-           'cpu-throttle-tailslow',
+           'cpu-throttle-tailslow', 'cpu-periodic-throttle',
+           'cpu-periodic-throttle-interval',
            'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
            'avail-switchover-bandwidth', 'downtime-limit',
            { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
@@ -899,6 +906,12 @@
 #     be excessive at tail stage.  The default value is false.  (Since
 #     5.1)
 #
+# @cpu-periodic-throttle: Make CPU throttling periodically.
+#     (Since 9.1)
+#
+# @cpu-periodic-throttle-interval: Interval of the periodic CPU throttling.
+#     (Since 9.1)
+#
 # @tls-creds: ID of the 'tls-creds' object that provides credentials
 #     for establishing a TLS connection over the migration data
 #     channel.  On the outgoing side of the migration, the credentials
@@ -1026,6 +1039,8 @@
             '*cpu-throttle-initial': 'uint8',
             '*cpu-throttle-increment': 'uint8',
             '*cpu-throttle-tailslow': 'bool',
+            '*cpu-periodic-throttle': 'bool',
+            '*cpu-periodic-throttle-interval': 'uint8',
             '*tls-creds': 'StrOrNull',
             '*tls-hostname': 'StrOrNull',
             '*tls-authz': 'StrOrNull',
@@ -1107,6 +1122,12 @@
 #     be excessive at tail stage.  The default value is false.  (Since
 #     5.1)
 #
+# @cpu-periodic-throttle: Make CPU throttling periodically.
+#     (Since 9.1)
+#
+# @cpu-periodic-throttle-interval: Interval of the periodic CPU throttling.
+#     (Since 9.1)
+#
 # @tls-creds: ID of the 'tls-creds' object that provides credentials
 #     for establishing a TLS connection over the migration data
 #     channel.  On the outgoing side of the migration, the credentials
@@ -1227,6 +1248,8 @@
             '*cpu-throttle-initial': 'uint8',
             '*cpu-throttle-increment': 'uint8',
             '*cpu-throttle-tailslow': 'bool',
+            '*cpu-periodic-throttle': 'bool',
+            '*cpu-periodic-throttle-interval': 'uint8',
             '*tls-creds': 'str',
             '*tls-hostname': 'str',
             '*tls-authz': 'str',
-- 
2.39.1

[PATCH RFC 04/10] qapi/migration: Introduce the iteration-count

[Hyman Huang]

The original migration information dirty-sync-count
could no longer reflect iteration count due to the
introduction of periodic synchronization in the next
commit; add the iteration count to compensate.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/migration-stats.h  |  4 ++++
 migration/migration.c        |  1 +
 migration/ram.c              | 12 ++++++++----
 qapi/migration.json          |  6 +++++-
 tests/qtest/migration-test.c |  2 +-
 5 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 05290ade76..43ee0f4f05 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -50,6 +50,10 @@ typedef struct {
      * Number of times we have synchronized guest bitmaps.
      */
     Stat64 dirty_sync_count;
+    /*
+     * Number of migration iteration processed.
+     */
+    Stat64 iteration_count;
     /*
      * Number of times zero copy failed to send any page using zero
      * copy.
diff --git a/migration/migration.c b/migration/migration.c
index 3dea06d577..055d527ff6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1197,6 +1197,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     info->ram->mbps = s->mbps;
     info->ram->dirty_sync_count =
         stat64_get(&mig_stats.dirty_sync_count);
+    info->ram->iteration_count = stat64_get(&mig_stats.iteration_count);
     info->ram->dirty_sync_missed_zero_copy =
         stat64_get(&mig_stats.dirty_sync_missed_zero_copy);
     info->ram->postcopy_requests =
diff --git a/migration/ram.c b/migration/ram.c
index a56634eb46..23471c9e5a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -594,7 +594,7 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
     /* We don't care if this fails to allocate a new cache page
      * as long as it updated an old one */
     cache_insert(XBZRLE.cache, current_addr, XBZRLE.zero_target_page,
-                 stat64_get(&mig_stats.dirty_sync_count));
+                 stat64_get(&mig_stats.iteration_count));
 }
 
 #define ENCODING_FLAG_XBZRLE 0x1
@@ -620,7 +620,7 @@ static int save_xbzrle_page(RAMState *rs, PageSearchStatus *pss,
     int encoded_len = 0, bytes_xbzrle;
     uint8_t *prev_cached_page;
     QEMUFile *file = pss->pss_channel;
-    uint64_t generation = stat64_get(&mig_stats.dirty_sync_count);
+    uint64_t generation = stat64_get(&mig_stats.iteration_count);
 
     if (!cache_is_cached(XBZRLE.cache, current_addr, generation)) {
         xbzrle_counters.cache_miss++;
@@ -1075,6 +1075,10 @@ static void migration_bitmap_sync(RAMState *rs,
     RAMBlock *block;
     int64_t end_time;
 
+    if (!periodic) {
+        stat64_add(&mig_stats.iteration_count, 1);
+    }
+
     stat64_add(&mig_stats.dirty_sync_count, 1);
 
     if (!rs->time_last_bitmap_sync) {
@@ -1111,8 +1115,8 @@ static void migration_bitmap_sync(RAMState *rs,
         rs->num_dirty_pages_period = 0;
         rs->bytes_xfer_prev = migration_transferred_bytes();
     }
-    if (migrate_events()) {
-        uint64_t generation = stat64_get(&mig_stats.dirty_sync_count);
+    if (!periodic && migrate_events()) {
+        uint64_t generation = stat64_get(&mig_stats.iteration_count);
         qapi_event_send_migration_pass(generation);
     }
 }
diff --git a/qapi/migration.json b/qapi/migration.json
index 8281d4a83b..6d8358c202 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -60,6 +60,9 @@
 #     between 0 and @dirty-sync-count * @multifd-channels.  (since
 #     7.1)
 #
+# @iteration-count: The number of iterations since migration started.
+#     (since 9.2)
+#
 # Since: 0.14
 ##
 { 'struct': 'MigrationStats',
@@ -72,7 +75,8 @@
            'multifd-bytes': 'uint64', 'pages-per-second': 'uint64',
            'precopy-bytes': 'uint64', 'downtime-bytes': 'uint64',
            'postcopy-bytes': 'uint64',
-           'dirty-sync-missed-zero-copy': 'uint64' } }
+           'dirty-sync-missed-zero-copy': 'uint64',
+           'iteration-count' : 'int' } }
 
 ##
 # @XBZRLECacheStats:
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9d08101643..2fb10658d4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -278,7 +278,7 @@ static int64_t read_migrate_property_int(QTestState *who, const char *property)
 
 static uint64_t get_migration_pass(QTestState *who)
 {
-    return read_ram_property_int(who, "dirty-sync-count");
+    return read_ram_property_int(who, "iteration-count");
 }
 
 static void read_blocktime(QTestState *who)
-- 
2.39.1

[PATCH RFC 05/10] migration: Introduce util functions for periodic CPU throttle

[Hyman Huang]

Provide useful utilities to manage the periodic_throttle_thread's
lifespan. Additionally, to set up sync mode, provide
periodic_throttle_setup.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/ram.c        | 98 +++++++++++++++++++++++++++++++++++++++++-
 migration/ram.h        |  4 ++
 migration/trace-events |  3 ++
 3 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 23471c9e5a..d9d8ed0fda 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -416,6 +416,10 @@ struct RAMState {
      * RAM migration.
      */
     unsigned int postcopy_bmap_sync_requested;
+
+    /* Periodic throttle information */
+    bool throttle_running;
+    QemuThread throttle_thread;
 };
 typedef struct RAMState RAMState;
 
@@ -1075,7 +1079,13 @@ static void migration_bitmap_sync(RAMState *rs,
     RAMBlock *block;
     int64_t end_time;
 
-    if (!periodic) {
+    if (periodic) {
+        /* Be careful that we don't synchronize too often */
+        int64_t curr_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+        if (curr_time < rs->time_last_bitmap_sync + 1000) {
+            return;
+        }
+    } else {
         stat64_add(&mig_stats.iteration_count, 1);
     }
 
@@ -1121,6 +1131,92 @@ static void migration_bitmap_sync(RAMState *rs,
     }
 }
 
+static void *periodic_throttle_thread(void *opaque)
+{
+    RAMState *rs = opaque;
+    bool skip_sleep = false;
+    int sleep_duration = migrate_periodic_throttle_interval();
+
+    rcu_register_thread();
+
+    while (qatomic_read(&rs->throttle_running)) {
+        int64_t curr_time;
+        /*
+         * The first iteration copies all memory anyhow and has no
+         * effect on guest performance, therefore omit it to avoid
+         * paying extra for the sync penalty.
+         */
+        if (stat64_get(&mig_stats.iteration_count) <= 1) {
+            continue;
+        }
+
+        if (!skip_sleep) {
+            sleep(sleep_duration);
+        }
+
+        /* Be careful that we don't synchronize too often */
+        curr_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+        if (curr_time > rs->time_last_bitmap_sync + 1000) {
+            bql_lock();
+            trace_migration_periodic_throttle();
+            WITH_RCU_READ_LOCK_GUARD() {
+                migration_bitmap_sync(rs, false, true);
+            }
+            bql_unlock();
+            skip_sleep = false;
+        } else {
+            skip_sleep = true;
+        }
+    }
+
+    rcu_unregister_thread();
+
+    return NULL;
+}
+
+void periodic_throttle_start(void)
+{
+    RAMState *rs = ram_state;
+
+    if (!rs) {
+        return;
+    }
+
+    if (qatomic_read(&rs->throttle_running)) {
+        return;
+    }
+
+    trace_migration_periodic_throttle_start();
+
+    qatomic_set(&rs->throttle_running, 1);
+    qemu_thread_create(&rs->throttle_thread,
+                       NULL, periodic_throttle_thread,
+                       rs, QEMU_THREAD_JOINABLE);
+}
+
+void periodic_throttle_stop(void)
+{
+    RAMState *rs = ram_state;
+
+    if (!rs) {
+        return;
+    }
+
+    if (!qatomic_read(&rs->throttle_running)) {
+        return;
+    }
+
+    trace_migration_periodic_throttle_stop();
+
+    qatomic_set(&rs->throttle_running, 0);
+    qemu_thread_join(&rs->throttle_thread);
+}
+
+void periodic_throttle_setup(bool enable)
+{
+    sync_mode = enable ? RAMBLOCK_SYN_MODERN : RAMBLOCK_SYN_LEGACY;
+}
+
 static void migration_bitmap_sync_precopy(RAMState *rs, bool last_stage)
 {
     Error *local_err = NULL;
diff --git a/migration/ram.h b/migration/ram.h
index bc0318b834..f7c7b2e7ad 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -93,4 +93,8 @@ void ram_write_tracking_prepare(void);
 int ram_write_tracking_start(void);
 void ram_write_tracking_stop(void);
 
+/* Periodic throttle */
+void periodic_throttle_start(void);
+void periodic_throttle_stop(void);
+void periodic_throttle_setup(bool enable);
 #endif
diff --git a/migration/trace-events b/migration/trace-events
index c65902f042..5b9db57c8f 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -95,6 +95,9 @@ get_queued_page_not_dirty(const char *block_name, uint64_t tmp_offset, unsigned
 migration_bitmap_sync_start(void) ""
 migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64
 migration_bitmap_clear_dirty(char *str, uint64_t start, uint64_t size, unsigned long page) "rb %s start 0x%"PRIx64" size 0x%"PRIx64" page 0x%lx"
+migration_periodic_throttle(void) ""
+migration_periodic_throttle_start(void) ""
+migration_periodic_throttle_stop(void) ""
 migration_throttle(void) ""
 migration_dirty_limit_guest(int64_t dirtyrate) "guest dirty page rate limit %" PRIi64 " MB/s"
 ram_discard_range(const char *rbname, uint64_t start, size_t len) "%s: start: %" PRIx64 " %zx"
-- 
2.39.1

[PATCH RFC 06/10] migration: Support periodic CPU throttle

[Hyman Huang]

When VM is configured with huge memory, the current throttle logic
doesn't look like to scale, because migration_trigger_throttle()
is only called for each iteration, so it won't be invoked for a long
time if one iteration can take a long time.

The periodic sync and throttle aims to fix the above issue by
synchronizing the remote dirty bitmap and triggering the throttle
periodically. This is a trade-off between synchronization overhead
and CPU throttle impact.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/migration.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 055d527ff6..fefd93b683 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1420,6 +1420,9 @@ static void migrate_fd_cleanup(MigrationState *s)
             qemu_thread_join(&s->thread);
             s->migration_thread_running = false;
         }
+        if (migrate_periodic_throttle()) {
+            periodic_throttle_stop();
+        }
         bql_lock();
 
         multifd_send_shutdown();
@@ -3263,6 +3266,9 @@ static MigIterateState migration_iteration_run(MigrationState *s)
 
     if ((!pending_size || pending_size < s->threshold_size) && can_switchover) {
         trace_migration_thread_low_pending(pending_size);
+        if (migrate_periodic_throttle()) {
+            periodic_throttle_stop();
+        }
         migration_completion(s);
         return MIG_ITERATE_BREAK;
     }
@@ -3508,6 +3514,11 @@ static void *migration_thread(void *opaque)
     ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
     bql_unlock();
 
+    if (migrate_periodic_throttle()) {
+        periodic_throttle_setup(true);
+        periodic_throttle_start();
+    }
+
     qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
                                MIGRATION_STATUS_ACTIVE);
 
-- 
2.39.1

[PATCH RFC 07/10] tests/migration-tests: Add test case for periodic throttle

[Hyman Huang]

To make sure periodic throttle feature doesn't regression
any features and functionalities, enable this feature in
the auto-converge migration test.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 tests/qtest/migration-test.c | 56 +++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 2fb10658d4..61d7182f88 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -281,6 +281,11 @@ static uint64_t get_migration_pass(QTestState *who)
     return read_ram_property_int(who, "iteration-count");
 }
 
+static uint64_t get_migration_dirty_sync_count(QTestState *who)
+{
+    return read_ram_property_int(who, "dirty-sync-count");
+}
+
 static void read_blocktime(QTestState *who)
 {
     QDict *rsp_return;
@@ -710,6 +715,11 @@ typedef struct {
     PostcopyRecoveryFailStage postcopy_recovery_fail_stage;
 } MigrateCommon;
 
+typedef struct {
+    /* CPU throttle parameters */
+    bool periodic;
+} AutoConvergeArgs;
+
 static int test_migrate_start(QTestState **from, QTestState **to,
                               const char *uri, MigrateStart *args)
 {
@@ -2778,12 +2788,13 @@ static void test_validate_uri_channels_none_set(void)
  * To make things even worse, we need to run the initial stage at
  * 3MB/s so we enter autoconverge even when host is (over)loaded.
  */
-static void test_migrate_auto_converge(void)
+static void test_migrate_auto_converge_args(AutoConvergeArgs *input_args)
 {
     g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     MigrateStart args = {};
     QTestState *from, *to;
     int64_t percentage;
+    bool periodic = (input_args && input_args->periodic);
 
     /*
      * We want the test to be stable and as fast as possible.
@@ -2791,6 +2802,7 @@ static void test_migrate_auto_converge(void)
      * so we need to decrease a bandwidth.
      */
     const int64_t init_pct = 5, inc_pct = 25, max_pct = 95;
+    const int64_t periodic_throttle_interval = 2;
 
     if (test_migrate_start(&from, &to, uri, &args)) {
         return;
@@ -2801,6 +2813,12 @@ static void test_migrate_auto_converge(void)
     migrate_set_parameter_int(from, "cpu-throttle-increment", inc_pct);
     migrate_set_parameter_int(from, "max-cpu-throttle", max_pct);
 
+    if (periodic) {
+        migrate_set_parameter_bool(from, "cpu-periodic-throttle", true);
+        migrate_set_parameter_int(from, "cpu-periodic-throttle-interval",
+                periodic_throttle_interval);
+    }
+
     /*
      * Set the initial parameters so that the migration could not converge
      * without throttling.
@@ -2827,6 +2845,29 @@ static void test_migrate_auto_converge(void)
     } while (true);
     /* The first percentage of throttling should be at least init_pct */
     g_assert_cmpint(percentage, >=, init_pct);
+
+    if (periodic) {
+        /*
+         * Check if periodic sync take effect, set the timeout with 20s
+         * (max_try_count * 1s), if extra sync doesn't show up, fail test.
+         */
+        uint64_t iteration_count, dirty_sync_count;
+        bool extra_sync = false;
+        int max_try_count = 20;
+
+        /* Check if periodic sync take effect */
+        while (--max_try_count) {
+            usleep(1000 * 1000);
+            iteration_count = get_migration_pass(from);
+            dirty_sync_count = get_migration_dirty_sync_count(from);
+            if (dirty_sync_count > iteration_count) {
+                extra_sync = true;
+                break;
+            }
+        }
+        g_assert(extra_sync);
+    }
+
     /* Now, when we tested that throttling works, let it converge */
     migrate_ensure_converge(from);
 
@@ -2849,6 +2890,17 @@ static void test_migrate_auto_converge(void)
     test_migrate_end(from, to, true);
 }
 
+static void test_migrate_auto_converge(void)
+{
+    test_migrate_auto_converge_args(NULL);
+}
+
+static void test_migrate_auto_converge_periodic_throttle(void)
+{
+    AutoConvergeArgs args = {.periodic = true};
+    test_migrate_auto_converge_args(&args);
+}
+
 static void *
 test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
                                               QTestState *to,
@@ -3900,6 +3952,8 @@ int main(int argc, char **argv)
     if (g_test_slow()) {
         migration_test_add("/migration/auto_converge",
                            test_migrate_auto_converge);
+        migration_test_add("/migration/auto_converge_periodic_throttle",
+                           test_migrate_auto_converge_periodic_throttle);
         if (g_str_equal(arch, "x86_64") &&
             has_kvm && kvm_dirty_ring_supported()) {
             migration_test_add("/migration/dirty_limit",
-- 
2.39.1

[PATCH RFC 08/10] migration: Introduce cpu-responsive-throttle parameter

[Hyman Huang]

To enable the responsive throttle that will be implemented
in the next commit, introduce the cpu-responsive-throttle
parameter.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/migration-hmp-cmds.c |  8 ++++++++
 migration/options.c            | 20 ++++++++++++++++++++
 migration/options.h            |  1 +
 qapi/migration.json            | 16 +++++++++++++++-
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index f7b8e06bb4..a3d4d3f62f 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -273,6 +273,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             MigrationParameter_str(
                 MIGRATION_PARAMETER_CPU_PERIODIC_THROTTLE_INTERVAL),
             params->cpu_periodic_throttle_interval);
+        assert(params->has_cpu_responsive_throttle);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_CPU_RESPONSIVE_THROTTLE),
+            params->cpu_responsive_throttle ? "on" : "off");
         assert(params->has_max_cpu_throttle);
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
@@ -529,6 +533,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_cpu_periodic_throttle_interval = true;
         visit_type_uint8(v, param, &p->cpu_periodic_throttle_interval, &err);
         break;
+    case MIGRATION_PARAMETER_CPU_RESPONSIVE_THROTTLE:
+        p->has_cpu_responsive_throttle = true;
+        visit_type_bool(v, param, &p->cpu_responsive_throttle, &err);
+        break;
     case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
         p->has_max_cpu_throttle = true;
         visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
diff --git a/migration/options.c b/migration/options.c
index 2dbe275ba0..aa233684ee 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -110,6 +110,8 @@ Property migration_properties[] = {
     DEFINE_PROP_UINT8("x-cpu-periodic-throttle-interval", MigrationState,
                       parameters.cpu_periodic_throttle_interval,
                       DEFAULT_MIGRATE_CPU_PERIODIC_THROTTLE_INTERVAL),
+    DEFINE_PROP_BOOL("x-cpu-responsive-throttle", MigrationState,
+                      parameters.cpu_responsive_throttle, false),
     DEFINE_PROP_SIZE("x-max-bandwidth", MigrationState,
                       parameters.max_bandwidth, MAX_THROTTLE),
     DEFINE_PROP_SIZE("avail-switchover-bandwidth", MigrationState,
@@ -715,6 +717,13 @@ bool migrate_periodic_throttle(void)
     return s->parameters.cpu_periodic_throttle;
 }
 
+bool migrate_responsive_throttle(void)
+{
+    MigrationState *s = migrate_get_current();
+
+    return s->parameters.cpu_responsive_throttle;
+}
+
 bool migrate_cpu_throttle_tailslow(void)
 {
     MigrationState *s = migrate_get_current();
@@ -899,6 +908,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->has_cpu_periodic_throttle_interval = true;
     params->cpu_periodic_throttle_interval =
         s->parameters.cpu_periodic_throttle_interval;
+    params->has_cpu_responsive_throttle = true;
+    params->cpu_responsive_throttle = s->parameters.cpu_responsive_throttle;
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     params->tls_hostname = g_strdup(s->parameters.tls_hostname);
     params->tls_authz = g_strdup(s->parameters.tls_authz ?
@@ -967,6 +978,7 @@ void migrate_params_init(MigrationParameters *params)
     params->has_cpu_throttle_tailslow = true;
     params->has_cpu_periodic_throttle = true;
     params->has_cpu_periodic_throttle_interval = true;
+    params->has_cpu_responsive_throttle = true;
     params->has_max_bandwidth = true;
     params->has_downtime_limit = true;
     params->has_x_checkpoint_delay = true;
@@ -1208,6 +1220,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
             params->cpu_periodic_throttle_interval;
     }
 
+    if (params->has_cpu_responsive_throttle) {
+        dest->cpu_responsive_throttle = params->cpu_responsive_throttle;
+    }
+
     if (params->tls_creds) {
         assert(params->tls_creds->type == QTYPE_QSTRING);
         dest->tls_creds = params->tls_creds->u.s;
@@ -1325,6 +1341,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
             params->cpu_periodic_throttle_interval;
     }
 
+    if (params->has_cpu_responsive_throttle) {
+        s->parameters.cpu_responsive_throttle = params->cpu_responsive_throttle;
+    }
+
     if (params->tls_creds) {
         g_free(s->parameters.tls_creds);
         assert(params->tls_creds->type == QTYPE_QSTRING);
diff --git a/migration/options.h b/migration/options.h
index efeac01470..613d675003 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -70,6 +70,7 @@ uint8_t migrate_cpu_throttle_increment(void);
 uint8_t migrate_cpu_throttle_initial(void);
 uint8_t migrate_periodic_throttle_interval(void);
 bool migrate_periodic_throttle(void);
+bool migrate_responsive_throttle(void);
 bool migrate_cpu_throttle_tailslow(void);
 bool migrate_direct_io(void);
 uint64_t migrate_downtime_limit(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index 6d8358c202..9f52ed1899 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -734,6 +734,10 @@
 # @cpu-periodic-throttle-interval: Interval of the periodic CPU throttling.
 #     (Since 9.1)
 #
+# @cpu-responsive-throttle: Make CPU throttling more responsively by
+#                           introduce an extra detection metric of
+#                           migration convergence. (Since 9.1)
+#
 # @tls-creds: ID of the 'tls-creds' object that provides credentials
 #     for establishing a TLS connection over the migration data
 #     channel.  On the outgoing side of the migration, the credentials
@@ -855,7 +859,7 @@
            'throttle-trigger-threshold',
            'cpu-throttle-initial', 'cpu-throttle-increment',
            'cpu-throttle-tailslow', 'cpu-periodic-throttle',
-           'cpu-periodic-throttle-interval',
+           'cpu-periodic-throttle-interval', 'cpu-responsive-throttle',
            'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
            'avail-switchover-bandwidth', 'downtime-limit',
            { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
@@ -916,6 +920,10 @@
 # @cpu-periodic-throttle-interval: Interval of the periodic CPU throttling.
 #     (Since 9.1)
 #
+# @cpu-responsive-throttle: Make CPU throttling more responsively by
+#                           introduce an extra detection metric of
+#                           migration convergence. (Since 9.1)
+#
 # @tls-creds: ID of the 'tls-creds' object that provides credentials
 #     for establishing a TLS connection over the migration data
 #     channel.  On the outgoing side of the migration, the credentials
@@ -1045,6 +1053,7 @@
             '*cpu-throttle-tailslow': 'bool',
             '*cpu-periodic-throttle': 'bool',
             '*cpu-periodic-throttle-interval': 'uint8',
+            '*cpu-responsive-throttle': 'bool',
             '*tls-creds': 'StrOrNull',
             '*tls-hostname': 'StrOrNull',
             '*tls-authz': 'StrOrNull',
@@ -1132,6 +1141,10 @@
 # @cpu-periodic-throttle-interval: Interval of the periodic CPU throttling.
 #     (Since 9.1)
 #
+# @cpu-responsive-throttle: Make CPU throttling more responsively by
+#                           introduce an extra detection metric of
+#                           migration convergence. (Since 9.1)
+#
 # @tls-creds: ID of the 'tls-creds' object that provides credentials
 #     for establishing a TLS connection over the migration data
 #     channel.  On the outgoing side of the migration, the credentials
@@ -1254,6 +1267,7 @@
             '*cpu-throttle-tailslow': 'bool',
             '*cpu-periodic-throttle': 'bool',
             '*cpu-periodic-throttle-interval': 'uint8',
+            '*cpu-responsive-throttle': 'bool',
             '*tls-creds': 'str',
             '*tls-hostname': 'str',
             '*tls-authz': 'str',
-- 
2.39.1

[PATCH RFC 09/10] migration: Support responsive CPU throttle

[Hyman Huang]

Currently, the convergence algorithm determines that the migration
cannot converge according to the following principle:
The dirty pages generated in current iteration exceed a specific
percentage (throttle-trigger-threshold, 50 by default) of the number
of transmissions. Let's refer to this criteria as the "dirty rate".
If this criteria is met more than or equal to twice
(dirty_rate_high_cnt >= 2), the throttle percentage increased.

In most cases, above implementation is appropriate. However, for a
VM with high memory overload, each iteration is time-consuming.
The VM's computing performance may be throttled at a high percentage
and last for a long time due to the repeated confirmation behavior.
Which may be intolerable for some computationally sensitive software
in the VM.

As the comment mentioned in the migration_trigger_throttle function,
in order to avoid erroneous detection, the original algorithm confirms
the criteria repeatedly. Put differently, the criteria does not need
to be validated again once the detection is more reliable.

In the refinement, in order to make the detection more accurate, we
introduce another criteria, called the "dirty ratio" to determine
the migration convergence. The "dirty ratio" is the ratio of
bytes_xfer_period and bytes_dirty_period. When the algorithm
repeatedly detects that the "dirty ratio" of current sync is lower
than the previous, the algorithm determines that the migration cannot
converge. For the "dirty rate" and "dirty ratio", if one of the two
criteria is met, the penalty percentage would be increased. This
makes CPU throttle more responsively and therefor saves the time of
the entire iteration and therefore reduces the time of VM performance
degradation.

In conclusion, this refinement significantly reduces the processing
time required for the throttle percentage step to its maximum while
the VM is under a high memory load.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 migration/ram.c              | 55 ++++++++++++++++++++++++++++++++++--
 migration/trace-events       |  1 +
 tests/qtest/migration-test.c |  1 +
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d9d8ed0fda..5fba572f3e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -420,6 +420,12 @@ struct RAMState {
     /* Periodic throttle information */
     bool throttle_running;
     QemuThread throttle_thread;
+
+    /*
+     * Ratio of bytes_dirty_period and bytes_xfer_period in the previous
+     * sync.
+     */
+    uint64_t dirty_ratio_pct;
 };
 typedef struct RAMState RAMState;
 
@@ -1044,6 +1050,43 @@ static void migration_dirty_limit_guest(void)
     trace_migration_dirty_limit_guest(quota_dirtyrate);
 }
 
+static bool migration_dirty_ratio_high(RAMState *rs)
+{
+    static int dirty_ratio_high_cnt;
+    uint64_t threshold = migrate_throttle_trigger_threshold();
+    uint64_t bytes_xfer_period =
+        migration_transferred_bytes() - rs->bytes_xfer_prev;
+    uint64_t bytes_dirty_period = rs->num_dirty_pages_period * TARGET_PAGE_SIZE;
+    bool dirty_ratio_high = false;
+    uint64_t prev, curr;
+
+    /* Calculate the dirty ratio percentage */
+    curr = 100 * (bytes_dirty_period * 1.0 / bytes_xfer_period);
+
+    prev = rs->dirty_ratio_pct;
+    rs->dirty_ratio_pct = curr;
+
+    if (prev == 0) {
+        return false;
+    }
+
+    /*
+     * If current dirty ratio is greater than previouse, determine
+     * that the migration do not converge.
+     */
+    if (curr > threshold && curr >= prev) {
+        trace_migration_dirty_ratio_high(curr, prev);
+        dirty_ratio_high_cnt++;
+    }
+
+    if (dirty_ratio_high_cnt >= 2) {
+        dirty_ratio_high = true;
+        dirty_ratio_high_cnt = 0;
+    }
+
+    return dirty_ratio_high;
+}
+
 static void migration_trigger_throttle(RAMState *rs)
 {
     uint64_t threshold = migrate_throttle_trigger_threshold();
@@ -1051,6 +1094,11 @@ static void migration_trigger_throttle(RAMState *rs)
         migration_transferred_bytes() - rs->bytes_xfer_prev;
     uint64_t bytes_dirty_period = rs->num_dirty_pages_period * TARGET_PAGE_SIZE;
     uint64_t bytes_dirty_threshold = bytes_xfer_period * threshold / 100;
+    bool dirty_ratio_high = false;
+
+    if (migrate_responsive_throttle() && (bytes_xfer_period != 0)) {
+        dirty_ratio_high = migration_dirty_ratio_high(rs);
+    }
 
     /*
      * The following detection logic can be refined later. For now:
@@ -1060,8 +1108,11 @@ static void migration_trigger_throttle(RAMState *rs)
      * twice, start or increase throttling.
      */
     if ((bytes_dirty_period > bytes_dirty_threshold) &&
-        (++rs->dirty_rate_high_cnt >= 2)) {
-        rs->dirty_rate_high_cnt = 0;
+        ((++rs->dirty_rate_high_cnt >= 2) || dirty_ratio_high)) {
+
+        rs->dirty_rate_high_cnt =
+            rs->dirty_rate_high_cnt >= 2 ? 0 : rs->dirty_rate_high_cnt;
+
         if (migrate_auto_converge()) {
             trace_migration_throttle();
             mig_throttle_guest_down(bytes_dirty_period,
diff --git a/migration/trace-events b/migration/trace-events
index 5b9db57c8f..241bbfcee9 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -95,6 +95,7 @@ get_queued_page_not_dirty(const char *block_name, uint64_t tmp_offset, unsigned
 migration_bitmap_sync_start(void) ""
 migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64
 migration_bitmap_clear_dirty(char *str, uint64_t start, uint64_t size, unsigned long page) "rb %s start 0x%"PRIx64" size 0x%"PRIx64" page 0x%lx"
+migration_dirty_ratio_high(uint64_t cur, uint64_t prev) "current ratio: %" PRIu64 " previous ratio: %" PRIu64
 migration_periodic_throttle(void) ""
 migration_periodic_throttle_start(void) ""
 migration_periodic_throttle_stop(void) ""
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 61d7182f88..4626301435 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2812,6 +2812,7 @@ static void test_migrate_auto_converge_args(AutoConvergeArgs *input_args)
     migrate_set_parameter_int(from, "cpu-throttle-initial", init_pct);
     migrate_set_parameter_int(from, "cpu-throttle-increment", inc_pct);
     migrate_set_parameter_int(from, "max-cpu-throttle", max_pct);
+    migrate_set_parameter_bool(from, "cpu-responsive-throttle", true);
 
     if (periodic) {
         migrate_set_parameter_bool(from, "cpu-periodic-throttle", true);
-- 
2.39.1

[PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Hyman Huang]

Despite the fact that the responsive CPU throttle is enabled,
the dirty sync count may not always increase because this is
an optimization that might not happen in any situation.

This test case just making sure it doesn't interfere with any
current functionality.

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
---
 tests/qtest/migration-test.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4626301435..cf0b1dcb50 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -718,6 +718,7 @@ typedef struct {
 typedef struct {
     /* CPU throttle parameters */
     bool periodic;
+    bool responsive;
 } AutoConvergeArgs;
 
 static int test_migrate_start(QTestState **from, QTestState **to,
@@ -2795,6 +2796,7 @@ static void test_migrate_auto_converge_args(AutoConvergeArgs *input_args)
     QTestState *from, *to;
     int64_t percentage;
     bool periodic = (input_args && input_args->periodic);
+    bool responsive = (input_args && input_args->responsive);
 
     /*
      * We want the test to be stable and as fast as possible.
@@ -2820,6 +2822,16 @@ static void test_migrate_auto_converge_args(AutoConvergeArgs *input_args)
                 periodic_throttle_interval);
     }
 
+    if (responsive) {
+        /*
+         * The dirty-sync-count may not always go down while using responsive
+         * throttle because it is an optimization and may not take effect in
+         * any scenario. Just making sure this feature doesn't break any
+         * existing functionality by turning it on.
+         */
+        migrate_set_parameter_bool(from, "cpu-responsive-throttle", true);
+    }
+
     /*
      * Set the initial parameters so that the migration could not converge
      * without throttling.
@@ -2902,6 +2914,12 @@ static void test_migrate_auto_converge_periodic_throttle(void)
     test_migrate_auto_converge_args(&args);
 }
 
+static void test_migrate_auto_converge_responsive_throttle(void)
+{
+    AutoConvergeArgs args = {.responsive = true};
+    test_migrate_auto_converge_args(&args);
+}
+
 static void *
 test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
                                               QTestState *to,
@@ -3955,6 +3973,8 @@ int main(int argc, char **argv)
                            test_migrate_auto_converge);
         migration_test_add("/migration/auto_converge_periodic_throttle",
                            test_migrate_auto_converge_periodic_throttle);
+        migration_test_add("/migration/auto_converge_responsive_throttle",
+                           test_migrate_auto_converge_responsive_throttle);
         if (g_str_equal(arch, "x86_64") &&
             has_kvm && kvm_dirty_ring_supported()) {
             migration_test_add("/migration/dirty_limit",
-- 
2.39.1

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Maydell]

On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
>
> Despite the fact that the responsive CPU throttle is enabled,
> the dirty sync count may not always increase because this is
> an optimization that might not happen in any situation.
>
> This test case just making sure it doesn't interfere with any
> current functionality.
>
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>

tests/qtest/migration-test already runs 75 different
subtests, takes up a massive chunk of our "make check"
time, and is very commonly a "times out" test on some
of our CI jobs. It runs on five different guest CPU
architectures, each one of which takes between 2 and
5 minutes to complete the full migration-test.

Do we really need to make it even bigger?

thanks
-- PMM

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
> >
> > Despite the fact that the responsive CPU throttle is enabled,
> > the dirty sync count may not always increase because this is
> > an optimization that might not happen in any situation.
> >
> > This test case just making sure it doesn't interfere with any
> > current functionality.
> >
> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> 
> tests/qtest/migration-test already runs 75 different
> subtests, takes up a massive chunk of our "make check"
> time, and is very commonly a "times out" test on some
> of our CI jobs. It runs on five different guest CPU
> architectures, each one of which takes between 2 and
> 5 minutes to complete the full migration-test.
> 
> Do we really need to make it even bigger?

I'll try to find some time in the next few weeks looking into this to see
whether we can further shrink migration test times after previous attemps
from Dan.  At least a low hanging fruit is we should indeed put some more
tests into g_test_slow(), and this new test could also be a candidate (then
we can run "-m slow" for migration PRs only).

Thanks,

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Mon, Sep 9, 2024 at 10:03 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
> >
> > Despite the fact that the responsive CPU throttle is enabled,
> > the dirty sync count may not always increase because this is
> > an optimization that might not happen in any situation.
> >
> > This test case just making sure it doesn't interfere with any
> > current functionality.
> >
> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
>
> tests/qtest/migration-test already runs 75 different
> subtests, takes up a massive chunk of our "make check"
> time, and is very commonly a "times out" test on some
> of our CI jobs. It runs on five different guest CPU
> architectures, each one of which takes between 2 and
> 5 minutes to complete the full migration-test.
>
> Do we really need to make it even bigger?
>

No, I don't insist on that; the cpu-responsive-throttle
parameter may also be enabled on the existing
migrate_auto_converge test case by default.

Thank for the comment.

Yong.


>
> thanks
> -- PMM
>


-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 3022 bytes --]

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
>> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
>> >
>> > Despite the fact that the responsive CPU throttle is enabled,
>> > the dirty sync count may not always increase because this is
>> > an optimization that might not happen in any situation.
>> >
>> > This test case just making sure it doesn't interfere with any
>> > current functionality.
>> >
>> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
>> 
>> tests/qtest/migration-test already runs 75 different
>> subtests, takes up a massive chunk of our "make check"
>> time, and is very commonly a "times out" test on some
>> of our CI jobs. It runs on five different guest CPU
>> architectures, each one of which takes between 2 and
>> 5 minutes to complete the full migration-test.
>> 
>> Do we really need to make it even bigger?
>
> I'll try to find some time in the next few weeks looking into this to see
> whether we can further shrink migration test times after previous attemps
> from Dan.  At least a low hanging fruit is we should indeed put some more
> tests into g_test_slow(), and this new test could also be a candidate (then
> we can run "-m slow" for migration PRs only).

I think we could (using -m slow or any other method) separate tests
that are generic enough that every CI run should benefit from them
vs. tests that are only useful once someone starts touching migration
code. I'd say very few in the former category and most of them in the
latter.

For an idea of where migration bugs lie, I took a look at what was
fixed since 2022:

# bugs | device/subsystem/arch
----------------------------------
    54 | migration
    10 | vfio
     6 | ppc
     3 | virtio-gpu
     2 | pcie_sriov, tpm_emulator,
          vdpa, virtio-rng-pci
     1 | arm, block, gpio, lasi,
          pci, s390, scsi-disk,
          virtio-mem, TCG

From these, ignoring the migration bugs, the migration-tests cover some
of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
once it is, then virtio-gpu would be covered and we could investigate
adding some of the others.

For actual migration code issues:

# bugs | (sub)subsystem | kind
----------------------------------------------
    13 | multifd        | correctness/races
     8 | ram            | correctness
     8 | rdma:          | general programming
     7 | qmp            | new api bugs
     5 | postcopy       | races
     4 | file:          | leaks
     3 | return path    | races
     3 | fd_cleanup     | races
     2 | savevm, aio/coroutines
     1 | xbzrle, colo, dirtyrate, exec:,
          windows, iochannel, qemufile,
          arch (ppc64le)

Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
file, rp, fd_cleanup, iochannel, qemufile, xbzrle.

My suggestion is we run per arch:

"/precopy/tcp/plain"
"/precopy/tcp/tls/psk/match",
"/postcopy/plain"
"/postcopy/preempt/plain"
"/postcopy/preempt/recovery/plain"
"/multifd/tcp/plain/cancel"
"/multifd/tcp/uri/plain/none"

and x86 gets extra:

"/precopy/unix/suspend/live"
"/precopy/unix/suspend/notlive"
"/dirty_ring"

(the other dirty_* tests are too slow)

All the rest go behind a knob that people touching migration code will
enable.

wdyt?

1- allows adding devices to QEMU cmdline for migration-test
https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Mon, Sep 09, 2024 at 06:54:46PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
> >> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
> >> >
> >> > Despite the fact that the responsive CPU throttle is enabled,
> >> > the dirty sync count may not always increase because this is
> >> > an optimization that might not happen in any situation.
> >> >
> >> > This test case just making sure it doesn't interfere with any
> >> > current functionality.
> >> >
> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> >> 
> >> tests/qtest/migration-test already runs 75 different
> >> subtests, takes up a massive chunk of our "make check"
> >> time, and is very commonly a "times out" test on some
> >> of our CI jobs. It runs on five different guest CPU
> >> architectures, each one of which takes between 2 and
> >> 5 minutes to complete the full migration-test.
> >> 
> >> Do we really need to make it even bigger?
> >
> > I'll try to find some time in the next few weeks looking into this to see
> > whether we can further shrink migration test times after previous attemps
> > from Dan.  At least a low hanging fruit is we should indeed put some more
> > tests into g_test_slow(), and this new test could also be a candidate (then
> > we can run "-m slow" for migration PRs only).
> 
> I think we could (using -m slow or any other method) separate tests
> that are generic enough that every CI run should benefit from them
> vs. tests that are only useful once someone starts touching migration
> code. I'd say very few in the former category and most of them in the
> latter.
> 
> For an idea of where migration bugs lie, I took a look at what was
> fixed since 2022:
> 
> # bugs | device/subsystem/arch
> ----------------------------------
>     54 | migration
>     10 | vfio
>      6 | ppc
>      3 | virtio-gpu
>      2 | pcie_sriov, tpm_emulator,
>           vdpa, virtio-rng-pci
>      1 | arm, block, gpio, lasi,
>           pci, s390, scsi-disk,
>           virtio-mem, TCG

Just curious; how did you collect these?

> 
> From these, ignoring the migration bugs, the migration-tests cover some
> of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
> once it is, then virtio-gpu would be covered and we could investigate
> adding some of the others.
> 
> For actual migration code issues:
> 
> # bugs | (sub)subsystem | kind
> ----------------------------------------------
>     13 | multifd        | correctness/races
>      8 | ram            | correctness
>      8 | rdma:          | general programming

8 rdma bugs??? ouch..

>      7 | qmp            | new api bugs
>      5 | postcopy       | races
>      4 | file:          | leaks
>      3 | return path    | races
>      3 | fd_cleanup     | races
>      2 | savevm, aio/coroutines
>      1 | xbzrle, colo, dirtyrate, exec:,
>           windows, iochannel, qemufile,
>           arch (ppc64le)
> 
> Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
> file, rp, fd_cleanup, iochannel, qemufile, xbzrle.
> 
> My suggestion is we run per arch:
> 
> "/precopy/tcp/plain"
> "/precopy/tcp/tls/psk/match",
> "/postcopy/plain"
> "/postcopy/preempt/plain"
> "/postcopy/preempt/recovery/plain"
> "/multifd/tcp/plain/cancel"
> "/multifd/tcp/uri/plain/none"

Don't you want to still keep a few multifd / file tests?

IIUC some file ops can still be relevant to archs.  Multifd still has one
bug that can only reproduce on arm64.. but not x86_64.  I remember it's a
race condition when migration finishes, and the issue could be memory
ordering relevant, but maybe not.

> 
> and x86 gets extra:
> 
> "/precopy/unix/suspend/live"
> "/precopy/unix/suspend/notlive"
> "/dirty_ring"

dirty ring will be disabled anyway when !x86, so probably not a major
concern.

> 
> (the other dirty_* tests are too slow)

These are the 10 slowest tests when I run locally:

/x86_64/migration/multifd/tcp/tls/x509/allow-anon-client 2.41
/x86_64/migration/postcopy/recovery/plain 2.43
/x86_64/migration/multifd/tcp/tls/x509/default-host 2.66
/x86_64/migration/multifd/tcp/tls/x509/override-host 2.86
/x86_64/migration/postcopy/tls/psk 2.91
/x86_64/migration/postcopy/preempt/recovery/tls/psk 3.08
/x86_64/migration/postcopy/preempt/tls/psk 3.30
/x86_64/migration/postcopy/recovery/tls/psk 3.81
/x86_64/migration/vcpu_dirty_limit 13.29
/x86_64/migration/precopy/unix/xbzrle 27.55

Are you aware of people using xbzrle at all?

> 
> All the rest go behind a knob that people touching migration code will
> enable.
> 
> wdyt?

Agree with the general idea, but I worry above exact list can be too small.

IMHO we can definitely, at least, move the last two into slow list
(vcpu_dirty_limit and xbzrle), then it'll already save us 40sec each run..

> 
> 1- allows adding devices to QEMU cmdline for migration-test
> https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de
> 

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Sep 09, 2024 at 06:54:46PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
>> >> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
>> >> >
>> >> > Despite the fact that the responsive CPU throttle is enabled,
>> >> > the dirty sync count may not always increase because this is
>> >> > an optimization that might not happen in any situation.
>> >> >
>> >> > This test case just making sure it doesn't interfere with any
>> >> > current functionality.
>> >> >
>> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
>> >> 
>> >> tests/qtest/migration-test already runs 75 different
>> >> subtests, takes up a massive chunk of our "make check"
>> >> time, and is very commonly a "times out" test on some
>> >> of our CI jobs. It runs on five different guest CPU
>> >> architectures, each one of which takes between 2 and
>> >> 5 minutes to complete the full migration-test.
>> >> 
>> >> Do we really need to make it even bigger?
>> >
>> > I'll try to find some time in the next few weeks looking into this to see
>> > whether we can further shrink migration test times after previous attemps
>> > from Dan.  At least a low hanging fruit is we should indeed put some more
>> > tests into g_test_slow(), and this new test could also be a candidate (then
>> > we can run "-m slow" for migration PRs only).
>> 
>> I think we could (using -m slow or any other method) separate tests
>> that are generic enough that every CI run should benefit from them
>> vs. tests that are only useful once someone starts touching migration
>> code. I'd say very few in the former category and most of them in the
>> latter.
>> 
>> For an idea of where migration bugs lie, I took a look at what was
>> fixed since 2022:
>> 
>> # bugs | device/subsystem/arch
>> ----------------------------------
>>     54 | migration
>>     10 | vfio
>>      6 | ppc
>>      3 | virtio-gpu
>>      2 | pcie_sriov, tpm_emulator,
>>           vdpa, virtio-rng-pci
>>      1 | arm, block, gpio, lasi,
>>           pci, s390, scsi-disk,
>>           virtio-mem, TCG
>
> Just curious; how did you collect these?

git log --since=2022 and then squinted at it. I wrote a warning to take
this with a grain of salt, but it missed the final version.

>
>> 
>> From these, ignoring the migration bugs, the migration-tests cover some
>> of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
>> once it is, then virtio-gpu would be covered and we could investigate
>> adding some of the others.
>> 
>> For actual migration code issues:
>> 
>> # bugs | (sub)subsystem | kind
>> ----------------------------------------------
>>     13 | multifd        | correctness/races
>>      8 | ram            | correctness
>>      8 | rdma:          | general programming
>
> 8 rdma bugs??? ouch..

Mostly caught by a cleanup from Markus. Silly stuff like of mixed signed
integer comparisons and bugs in error handling. I don't even want to
look too much at it.

...hopefully this release we'll manage to resolve that situation.

>
>>      7 | qmp            | new api bugs
>>      5 | postcopy       | races
>>      4 | file:          | leaks
>>      3 | return path    | races
>>      3 | fd_cleanup     | races
>>      2 | savevm, aio/coroutines
>>      1 | xbzrle, colo, dirtyrate, exec:,
>>           windows, iochannel, qemufile,
>>           arch (ppc64le)
>> 
>> Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
>> file, rp, fd_cleanup, iochannel, qemufile, xbzrle.
>> 
>> My suggestion is we run per arch:
>> 
>> "/precopy/tcp/plain"
>> "/precopy/tcp/tls/psk/match",
>> "/postcopy/plain"
>> "/postcopy/preempt/plain"
>> "/postcopy/preempt/recovery/plain"
>> "/multifd/tcp/plain/cancel"
>> "/multifd/tcp/uri/plain/none"
>
> Don't you want to still keep a few multifd / file tests?

Not really, but I won't object if you want to add some more in there. To
be honest, I want to get out of people's way as much as I can because
having to revisit this every couple of months is stressful to me.

My rationale for those is:

"/precopy/tcp/plain":
 Smoke test, the most common migration

"/precopy/tcp/tls/psk/match":
 Something might change in the distro regarding tls. Such as:
 https://gitlab.com/qemu-project/qemu/-/issues/1937

"/postcopy/plain":
 Smoke test for postcopy

"/postcopy/preempt/plain":
 Just to exercise the preempt thread

"/postcopy/preempt/recovery/plain":
 Recovery has had some issues with races in the past

"/multifd/tcp/plain/cancel":
 The MVP of catching concurrency issues

"/multifd/tcp/uri/plain/none":
 Smoke test for multifd

All in all, these will test basic funcionality and run very often. The
more tests we add to this set, the less return we get in relation to the
time they take.

>
> IIUC some file ops can still be relevant to archs.  Multifd still has one
> bug that can only reproduce on arm64.. but not x86_64.  I remember it's a
> race condition when migration finishes, and the issue could be memory
> ordering relevant, but maybe not.

I'm not aware of anything. I believe the last arm64 bug we had was the
threadinfo stuff[1]. If you remember what it's about, let me know.

1- 01ec0f3a92 ("migration/multifd: Protect accesses to migration_threads").

>
>> 
>> and x86 gets extra:
>> 
>> "/precopy/unix/suspend/live"
>> "/precopy/unix/suspend/notlive"
>> "/dirty_ring"
>
> dirty ring will be disabled anyway when !x86, so probably not a major
> concern.
>
>> 
>> (the other dirty_* tests are too slow)
>
> These are the 10 slowest tests when I run locally:
>
> /x86_64/migration/multifd/tcp/tls/x509/allow-anon-client 2.41
> /x86_64/migration/postcopy/recovery/plain 2.43
> /x86_64/migration/multifd/tcp/tls/x509/default-host 2.66
> /x86_64/migration/multifd/tcp/tls/x509/override-host 2.86
> /x86_64/migration/postcopy/tls/psk 2.91
> /x86_64/migration/postcopy/preempt/recovery/tls/psk 3.08
> /x86_64/migration/postcopy/preempt/tls/psk 3.30
> /x86_64/migration/postcopy/recovery/tls/psk 3.81
> /x86_64/migration/vcpu_dirty_limit 13.29
> /x86_64/migration/precopy/unix/xbzrle 27.55
>
> Are you aware of people using xbzrle at all?

Nope.

>
>> 
>> All the rest go behind a knob that people touching migration code will
>> enable.
>> 
>> wdyt?
>
> Agree with the general idea, but I worry above exact list can be too small.

We won't stop running the full set of tests. We can run them in our
forks' CI as much as we want. There are no cases of people reporting a
migration bug because their 'make check' caught something that ours
didn't.

Besides, the main strength of CI is to catch bugs when someone makes a
code change. If people touch migration code, then we'll run it in our CI
anyway. If they touch device code and that device is migrated by default
then any one of the simple tests will catch the issue when it runs via
the migration-compat job. If the device is not enabled by default, then
no tests will catch it.

The worst case scenario is they touch some code completely unrelated and
their 'make check' or CI run breaks because of some race in the
migration code. That's not what CI is for, that's just an annoyance for
everyone. I'd rather it breaks in our hands and then we'll go fix it.

>
> IMHO we can definitely, at least, move the last two into slow list
> (vcpu_dirty_limit and xbzrle), then it'll already save us 40sec each run..

Agreed. I'll send a patch once I get out from under downstream stuff.

>
>> 
>> 1- allows adding devices to QEMU cmdline for migration-test
>> https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de
>> 

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Tue, Sep 10, 2024 at 07:23:43PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, Sep 09, 2024 at 06:54:46PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
> >> >> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
> >> >> >
> >> >> > Despite the fact that the responsive CPU throttle is enabled,
> >> >> > the dirty sync count may not always increase because this is
> >> >> > an optimization that might not happen in any situation.
> >> >> >
> >> >> > This test case just making sure it doesn't interfere with any
> >> >> > current functionality.
> >> >> >
> >> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> >> >> 
> >> >> tests/qtest/migration-test already runs 75 different
> >> >> subtests, takes up a massive chunk of our "make check"
> >> >> time, and is very commonly a "times out" test on some
> >> >> of our CI jobs. It runs on five different guest CPU
> >> >> architectures, each one of which takes between 2 and
> >> >> 5 minutes to complete the full migration-test.
> >> >> 
> >> >> Do we really need to make it even bigger?
> >> >
> >> > I'll try to find some time in the next few weeks looking into this to see
> >> > whether we can further shrink migration test times after previous attemps
> >> > from Dan.  At least a low hanging fruit is we should indeed put some more
> >> > tests into g_test_slow(), and this new test could also be a candidate (then
> >> > we can run "-m slow" for migration PRs only).
> >> 
> >> I think we could (using -m slow or any other method) separate tests
> >> that are generic enough that every CI run should benefit from them
> >> vs. tests that are only useful once someone starts touching migration
> >> code. I'd say very few in the former category and most of them in the
> >> latter.
> >> 
> >> For an idea of where migration bugs lie, I took a look at what was
> >> fixed since 2022:
> >> 
> >> # bugs | device/subsystem/arch
> >> ----------------------------------
> >>     54 | migration
> >>     10 | vfio
> >>      6 | ppc
> >>      3 | virtio-gpu
> >>      2 | pcie_sriov, tpm_emulator,
> >>           vdpa, virtio-rng-pci
> >>      1 | arm, block, gpio, lasi,
> >>           pci, s390, scsi-disk,
> >>           virtio-mem, TCG
> >
> > Just curious; how did you collect these?
> 
> git log --since=2022 and then squinted at it. I wrote a warning to take
> this with a grain of salt, but it missed the final version.
> 
> >
> >> 
> >> From these, ignoring the migration bugs, the migration-tests cover some
> >> of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
> >> once it is, then virtio-gpu would be covered and we could investigate
> >> adding some of the others.
> >> 
> >> For actual migration code issues:
> >> 
> >> # bugs | (sub)subsystem | kind
> >> ----------------------------------------------
> >>     13 | multifd        | correctness/races
> >>      8 | ram            | correctness
> >>      8 | rdma:          | general programming
> >
> > 8 rdma bugs??? ouch..
> 
> Mostly caught by a cleanup from Markus. Silly stuff like of mixed signed
> integer comparisons and bugs in error handling. I don't even want to
> look too much at it.
> 
> ...hopefully this release we'll manage to resolve that situation.
> 
> >
> >>      7 | qmp            | new api bugs
> >>      5 | postcopy       | races
> >>      4 | file:          | leaks
> >>      3 | return path    | races
> >>      3 | fd_cleanup     | races
> >>      2 | savevm, aio/coroutines
> >>      1 | xbzrle, colo, dirtyrate, exec:,
> >>           windows, iochannel, qemufile,
> >>           arch (ppc64le)
> >> 
> >> Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
> >> file, rp, fd_cleanup, iochannel, qemufile, xbzrle.
> >> 
> >> My suggestion is we run per arch:
> >> 
> >> "/precopy/tcp/plain"
> >> "/precopy/tcp/tls/psk/match",
> >> "/postcopy/plain"
> >> "/postcopy/preempt/plain"
> >> "/postcopy/preempt/recovery/plain"
> >> "/multifd/tcp/plain/cancel"
> >> "/multifd/tcp/uri/plain/none"
> >
> > Don't you want to still keep a few multifd / file tests?
> 
> Not really, but I won't object if you want to add some more in there. To
> be honest, I want to get out of people's way as much as I can because
> having to revisit this every couple of months is stressful to me.

I hope migration tests are not too obstructive yet so far :) - this
discussion is still within a context where we might add one more slow test
in CI, and we probably simply should make it a -m slow test.

> 
> My rationale for those is:
> 
> "/precopy/tcp/plain":
>  Smoke test, the most common migration

E.g. unix is missing, and I'm not sure which we use for e.g. kubevirt.

And note that even if unix shares the socket iochannel with tcp, it may not
work the same.  For example, if you remember I mentioned I was looking at
threadify the dest qemu receive side, i have a draft there but currently it
has a bug to hang a unix migration, not tcp..

So IMHO it's not easy to justify which we can simply drop, if we still want
to provide some good gating in CI.  And I won't be 100% surprised if some
other non-migration PR (e.g. io/) changed some slight bit that unix is
broken and tcp keeps working, for example, then we loose some CI benefits.

> 
> "/precopy/tcp/tls/psk/match":
>  Something might change in the distro regarding tls. Such as:
>  https://gitlab.com/qemu-project/qemu/-/issues/1937
> 
> "/postcopy/plain":
>  Smoke test for postcopy
> 
> "/postcopy/preempt/plain":
>  Just to exercise the preempt thread
> 
> "/postcopy/preempt/recovery/plain":
>  Recovery has had some issues with races in the past
> 
> "/multifd/tcp/plain/cancel":
>  The MVP of catching concurrency issues
> 
> "/multifd/tcp/uri/plain/none":
>  Smoke test for multifd
> 
> All in all, these will test basic funcionality and run very often. The
> more tests we add to this set, the less return we get in relation to the
> time they take.

This is true.  We can try to discuss more on which is more important; I
still think something might be good to be added on top of above.

There's also the other way - at some point, I still want to improve
migration-test run speed, and please have a look if you like too at some
point: so there's still chance (average is ~2sec as of now), IMHO, we don't
lose anything in CI but runs simply faster.

> 
> >
> > IIUC some file ops can still be relevant to archs.  Multifd still has one
> > bug that can only reproduce on arm64.. but not x86_64.  I remember it's a
> > race condition when migration finishes, and the issue could be memory
> > ordering relevant, but maybe not.
> 
> I'm not aware of anything. I believe the last arm64 bug we had was the
> threadinfo stuff[1]. If you remember what it's about, let me know.
> 
> 1- 01ec0f3a92 ("migration/multifd: Protect accesses to migration_threads").

https://issues.redhat.com/browse/RHEL-45628

On RH side Bandan is looking at it, but he's during a long holidays
recently.

> 
> >
> >> 
> >> and x86 gets extra:
> >> 
> >> "/precopy/unix/suspend/live"
> >> "/precopy/unix/suspend/notlive"
> >> "/dirty_ring"
> >
> > dirty ring will be disabled anyway when !x86, so probably not a major
> > concern.
> >
> >> 
> >> (the other dirty_* tests are too slow)
> >
> > These are the 10 slowest tests when I run locally:
> >
> > /x86_64/migration/multifd/tcp/tls/x509/allow-anon-client 2.41
> > /x86_64/migration/postcopy/recovery/plain 2.43
> > /x86_64/migration/multifd/tcp/tls/x509/default-host 2.66
> > /x86_64/migration/multifd/tcp/tls/x509/override-host 2.86
> > /x86_64/migration/postcopy/tls/psk 2.91
> > /x86_64/migration/postcopy/preempt/recovery/tls/psk 3.08
> > /x86_64/migration/postcopy/preempt/tls/psk 3.30
> > /x86_64/migration/postcopy/recovery/tls/psk 3.81
> > /x86_64/migration/vcpu_dirty_limit 13.29
> > /x86_64/migration/precopy/unix/xbzrle 27.55
> >
> > Are you aware of people using xbzrle at all?
> 
> Nope.
> 
> >
> >> 
> >> All the rest go behind a knob that people touching migration code will
> >> enable.
> >> 
> >> wdyt?
> >
> > Agree with the general idea, but I worry above exact list can be too small.
> 
> We won't stop running the full set of tests. We can run them in our
> forks' CI as much as we want. There are no cases of people reporting a
> migration bug because their 'make check' caught something that ours
> didn't.

IIUC it's hard to say - when the test is in CI maintainers can catch them
already before sending a formal PR.

If the test is run by default in make check, a developer can trigger a
migration issue (with his/her patch applied) then one can notice it
introduced a bug, fix it, then post the patches.  We won't know whether
that happened.

So one thing we can do (if you think worthwhile to do it now) is we shrink
the default test case a lot as you proposed, then we wait and see what
breaks, and then we gradually add tests back when it can be used to find
breakages.  But that'll also take time if it really can find such tests,
because then we'll need to debug them one by one (instead of developer /
maintainer looking into them with their expertise knowledge..).  I'm not
sure whether it's worthwhile to do it now, but I don't feel strongly if we
can still have a reliable set of default test cases.

> 
> Besides, the main strength of CI is to catch bugs when someone makes a
> code change. If people touch migration code, then we'll run it in our CI
> anyway. If they touch device code and that device is migrated by default
> then any one of the simple tests will catch the issue when it runs via
> the migration-compat job. If the device is not enabled by default, then
> no tests will catch it.
> 
> The worst case scenario is they touch some code completely unrelated and
> their 'make check' or CI run breaks because of some race in the
> migration code. That's not what CI is for, that's just an annoyance for
> everyone. I'd rather it breaks in our hands and then we'll go fix it.
> 
> >
> > IMHO we can definitely, at least, move the last two into slow list
> > (vcpu_dirty_limit and xbzrle), then it'll already save us 40sec each run..
> 
> Agreed. I'll send a patch once I get out from under downstream stuff.
> 
> >
> >> 
> >> 1- allows adding devices to QEMU cmdline for migration-test
> >> https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de
> >> 
> 

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Sep 10, 2024 at 07:23:43PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Mon, Sep 09, 2024 at 06:54:46PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
>> >> >> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
>> >> >> >
>> >> >> > Despite the fact that the responsive CPU throttle is enabled,
>> >> >> > the dirty sync count may not always increase because this is
>> >> >> > an optimization that might not happen in any situation.
>> >> >> >
>> >> >> > This test case just making sure it doesn't interfere with any
>> >> >> > current functionality.
>> >> >> >
>> >> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
>> >> >> 
>> >> >> tests/qtest/migration-test already runs 75 different
>> >> >> subtests, takes up a massive chunk of our "make check"
>> >> >> time, and is very commonly a "times out" test on some
>> >> >> of our CI jobs. It runs on five different guest CPU
>> >> >> architectures, each one of which takes between 2 and
>> >> >> 5 minutes to complete the full migration-test.
>> >> >> 
>> >> >> Do we really need to make it even bigger?
>> >> >
>> >> > I'll try to find some time in the next few weeks looking into this to see
>> >> > whether we can further shrink migration test times after previous attemps
>> >> > from Dan.  At least a low hanging fruit is we should indeed put some more
>> >> > tests into g_test_slow(), and this new test could also be a candidate (then
>> >> > we can run "-m slow" for migration PRs only).
>> >> 
>> >> I think we could (using -m slow or any other method) separate tests
>> >> that are generic enough that every CI run should benefit from them
>> >> vs. tests that are only useful once someone starts touching migration
>> >> code. I'd say very few in the former category and most of them in the
>> >> latter.
>> >> 
>> >> For an idea of where migration bugs lie, I took a look at what was
>> >> fixed since 2022:
>> >> 
>> >> # bugs | device/subsystem/arch
>> >> ----------------------------------
>> >>     54 | migration
>> >>     10 | vfio
>> >>      6 | ppc
>> >>      3 | virtio-gpu
>> >>      2 | pcie_sriov, tpm_emulator,
>> >>           vdpa, virtio-rng-pci
>> >>      1 | arm, block, gpio, lasi,
>> >>           pci, s390, scsi-disk,
>> >>           virtio-mem, TCG
>> >
>> > Just curious; how did you collect these?
>> 
>> git log --since=2022 and then squinted at it. I wrote a warning to take
>> this with a grain of salt, but it missed the final version.
>> 
>> >
>> >> 
>> >> From these, ignoring the migration bugs, the migration-tests cover some
>> >> of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
>> >> once it is, then virtio-gpu would be covered and we could investigate
>> >> adding some of the others.
>> >> 
>> >> For actual migration code issues:
>> >> 
>> >> # bugs | (sub)subsystem | kind
>> >> ----------------------------------------------
>> >>     13 | multifd        | correctness/races
>> >>      8 | ram            | correctness
>> >>      8 | rdma:          | general programming
>> >
>> > 8 rdma bugs??? ouch..
>> 
>> Mostly caught by a cleanup from Markus. Silly stuff like of mixed signed
>> integer comparisons and bugs in error handling. I don't even want to
>> look too much at it.
>> 
>> ...hopefully this release we'll manage to resolve that situation.
>> 
>> >
>> >>      7 | qmp            | new api bugs
>> >>      5 | postcopy       | races
>> >>      4 | file:          | leaks
>> >>      3 | return path    | races
>> >>      3 | fd_cleanup     | races
>> >>      2 | savevm, aio/coroutines
>> >>      1 | xbzrle, colo, dirtyrate, exec:,
>> >>           windows, iochannel, qemufile,
>> >>           arch (ppc64le)
>> >> 
>> >> Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
>> >> file, rp, fd_cleanup, iochannel, qemufile, xbzrle.
>> >> 
>> >> My suggestion is we run per arch:
>> >> 
>> >> "/precopy/tcp/plain"
>> >> "/precopy/tcp/tls/psk/match",
>> >> "/postcopy/plain"
>> >> "/postcopy/preempt/plain"
>> >> "/postcopy/preempt/recovery/plain"
>> >> "/multifd/tcp/plain/cancel"
>> >> "/multifd/tcp/uri/plain/none"
>> >
>> > Don't you want to still keep a few multifd / file tests?
>> 
>> Not really, but I won't object if you want to add some more in there. To
>> be honest, I want to get out of people's way as much as I can because
>> having to revisit this every couple of months is stressful to me.
>
> I hope migration tests are not too obstructive yet so far :) - this
> discussion is still within a context where we might add one more slow test
> in CI, and we probably simply should make it a -m slow test.
>
>> 
>> My rationale for those is:
>> 
>> "/precopy/tcp/plain":
>>  Smoke test, the most common migration
>
> E.g. unix is missing, and I'm not sure which we use for e.g. kubevirt.
>
> And note that even if unix shares the socket iochannel with tcp, it may not
> work the same.  For example, if you remember I mentioned I was looking at
> threadify the dest qemu receive side, i have a draft there but currently it
> has a bug to hang a unix migration, not tcp..

Ok, let's add a unix one, no problem.

>
> So IMHO it's not easy to justify which we can simply drop, if we still want
> to provide some good gating in CI.

It's not exactly about dropping, it's about which ones need to be run at
*every* invocation of make check (x5 because of the multiple archs) and
at every git branch push in CI (unless -o ci.skip). For gating we don't
need all the tests. Many of them are testing the same core code with
just a few differences at the margins.

>  And I won't be 100% surprised if some
> other non-migration PR (e.g. io/) changed some slight bit that unix is
> broken and tcp keeps working, for example, then we loose some CI benefits.

IMO, these non-migration PRs are exactly what we have to worry
about. Because migration PRs would run with -m slow and we'd catch the
issue there.

>
>> 
>> "/precopy/tcp/tls/psk/match":
>>  Something might change in the distro regarding tls. Such as:
>>  https://gitlab.com/qemu-project/qemu/-/issues/1937
>> 
>> "/postcopy/plain":
>>  Smoke test for postcopy
>> 
>> "/postcopy/preempt/plain":
>>  Just to exercise the preempt thread
>> 
>> "/postcopy/preempt/recovery/plain":
>>  Recovery has had some issues with races in the past
>> 
>> "/multifd/tcp/plain/cancel":
>>  The MVP of catching concurrency issues
>> 
>> "/multifd/tcp/uri/plain/none":
>>  Smoke test for multifd
>> 
>> All in all, these will test basic funcionality and run very often. The
>> more tests we add to this set, the less return we get in relation to the
>> time they take.
>
> This is true.  We can try to discuss more on which is more important; I
> still think something might be good to be added on top of above.

Sure, just add what you think we need.

>
> There's also the other way - at some point, I still want to improve
> migration-test run speed, and please have a look if you like too at some
> point: so there's still chance (average is ~2sec as of now), IMHO, we don't
> lose anything in CI but runs simply faster.

My only idea, but it requires a bit of work, is to do unit testing on
the interfaces. Anything before migration_fd_connect(). Then we could
stop doing a full migration for those.

>
>> 
>> >
>> > IIUC some file ops can still be relevant to archs.  Multifd still has one
>> > bug that can only reproduce on arm64.. but not x86_64.  I remember it's a
>> > race condition when migration finishes, and the issue could be memory
>> > ordering relevant, but maybe not.
>> 
>> I'm not aware of anything. I believe the last arm64 bug we had was the
>> threadinfo stuff[1]. If you remember what it's about, let me know.
>> 
>> 1- 01ec0f3a92 ("migration/multifd: Protect accesses to migration_threads").
>
> https://issues.redhat.com/browse/RHEL-45628

Interesting. But if multifd/tcp/plain/cancel doesn't catch this I don't
think other tests will. Also, we don't have tests for zerocopy AFAIK.

>
> On RH side Bandan is looking at it, but he's during a long holidays
> recently.

Good luck to him.

>
>> 
>> >
>> >> 
>> >> and x86 gets extra:
>> >> 
>> >> "/precopy/unix/suspend/live"
>> >> "/precopy/unix/suspend/notlive"
>> >> "/dirty_ring"
>> >
>> > dirty ring will be disabled anyway when !x86, so probably not a major
>> > concern.
>> >
>> >> 
>> >> (the other dirty_* tests are too slow)
>> >
>> > These are the 10 slowest tests when I run locally:
>> >
>> > /x86_64/migration/multifd/tcp/tls/x509/allow-anon-client 2.41
>> > /x86_64/migration/postcopy/recovery/plain 2.43
>> > /x86_64/migration/multifd/tcp/tls/x509/default-host 2.66
>> > /x86_64/migration/multifd/tcp/tls/x509/override-host 2.86
>> > /x86_64/migration/postcopy/tls/psk 2.91
>> > /x86_64/migration/postcopy/preempt/recovery/tls/psk 3.08
>> > /x86_64/migration/postcopy/preempt/tls/psk 3.30
>> > /x86_64/migration/postcopy/recovery/tls/psk 3.81
>> > /x86_64/migration/vcpu_dirty_limit 13.29
>> > /x86_64/migration/precopy/unix/xbzrle 27.55
>> >
>> > Are you aware of people using xbzrle at all?
>> 
>> Nope.
>> 
>> >
>> >> 
>> >> All the rest go behind a knob that people touching migration code will
>> >> enable.
>> >> 
>> >> wdyt?
>> >
>> > Agree with the general idea, but I worry above exact list can be too small.
>> 
>> We won't stop running the full set of tests. We can run them in our
>> forks' CI as much as we want. There are no cases of people reporting a
>> migration bug because their 'make check' caught something that ours
>> didn't.
>
> IIUC it's hard to say - when the test is in CI maintainers can catch them
> already before sending a formal PR.
>
> If the test is run by default in make check, a developer can trigger a
> migration issue (with his/her patch applied) then one can notice it
> introduced a bug, fix it, then post the patches.  We won't know whether
> that happened.

Good point.

>
> So one thing we can do (if you think worthwhile to do it now) is we shrink
> the default test case a lot as you proposed, then we wait and see what
> breaks, and then we gradually add tests back when it can be used to find
> breakages.  But that'll also take time if it really can find such tests,
> because then we'll need to debug them one by one (instead of developer /
> maintainer looking into them with their expertise knowledge..).  I'm not
> sure whether it's worthwhile to do it now, but I don't feel strongly if we
> can still have a reliable set of default test cases.

We first need a way to enable them for the migration CI. Do we have a
variable that CI understands that can be used to enable slow tests?

>
>> 
>> Besides, the main strength of CI is to catch bugs when someone makes a
>> code change. If people touch migration code, then we'll run it in our CI
>> anyway. If they touch device code and that device is migrated by default
>> then any one of the simple tests will catch the issue when it runs via
>> the migration-compat job. If the device is not enabled by default, then
>> no tests will catch it.
>> 
>> The worst case scenario is they touch some code completely unrelated and
>> their 'make check' or CI run breaks because of some race in the
>> migration code. That's not what CI is for, that's just an annoyance for
>> everyone. I'd rather it breaks in our hands and then we'll go fix it.
>> 
>> >
>> > IMHO we can definitely, at least, move the last two into slow list
>> > (vcpu_dirty_limit and xbzrle), then it'll already save us 40sec each run..
>> 
>> Agreed. I'll send a patch once I get out from under downstream stuff.
>> 
>> >
>> >> 
>> >> 1- allows adding devices to QEMU cmdline for migration-test
>> >> https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de
>> >> 
>> 

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Wed, Sep 11, 2024 at 04:48:21PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Sep 10, 2024 at 07:23:43PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Mon, Sep 09, 2024 at 06:54:46PM -0300, Fabiano Rosas wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> 
> >> >> > On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
> >> >> >> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
> >> >> >> >
> >> >> >> > Despite the fact that the responsive CPU throttle is enabled,
> >> >> >> > the dirty sync count may not always increase because this is
> >> >> >> > an optimization that might not happen in any situation.
> >> >> >> >
> >> >> >> > This test case just making sure it doesn't interfere with any
> >> >> >> > current functionality.
> >> >> >> >
> >> >> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> >> >> >> 
> >> >> >> tests/qtest/migration-test already runs 75 different
> >> >> >> subtests, takes up a massive chunk of our "make check"
> >> >> >> time, and is very commonly a "times out" test on some
> >> >> >> of our CI jobs. It runs on five different guest CPU
> >> >> >> architectures, each one of which takes between 2 and
> >> >> >> 5 minutes to complete the full migration-test.
> >> >> >> 
> >> >> >> Do we really need to make it even bigger?
> >> >> >
> >> >> > I'll try to find some time in the next few weeks looking into this to see
> >> >> > whether we can further shrink migration test times after previous attemps
> >> >> > from Dan.  At least a low hanging fruit is we should indeed put some more
> >> >> > tests into g_test_slow(), and this new test could also be a candidate (then
> >> >> > we can run "-m slow" for migration PRs only).
> >> >> 
> >> >> I think we could (using -m slow or any other method) separate tests
> >> >> that are generic enough that every CI run should benefit from them
> >> >> vs. tests that are only useful once someone starts touching migration
> >> >> code. I'd say very few in the former category and most of them in the
> >> >> latter.
> >> >> 
> >> >> For an idea of where migration bugs lie, I took a look at what was
> >> >> fixed since 2022:
> >> >> 
> >> >> # bugs | device/subsystem/arch
> >> >> ----------------------------------
> >> >>     54 | migration
> >> >>     10 | vfio
> >> >>      6 | ppc
> >> >>      3 | virtio-gpu
> >> >>      2 | pcie_sriov, tpm_emulator,
> >> >>           vdpa, virtio-rng-pci
> >> >>      1 | arm, block, gpio, lasi,
> >> >>           pci, s390, scsi-disk,
> >> >>           virtio-mem, TCG
> >> >
> >> > Just curious; how did you collect these?
> >> 
> >> git log --since=2022 and then squinted at it. I wrote a warning to take
> >> this with a grain of salt, but it missed the final version.
> >> 
> >> >
> >> >> 
> >> >> From these, ignoring the migration bugs, the migration-tests cover some
> >> >> of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
> >> >> once it is, then virtio-gpu would be covered and we could investigate
> >> >> adding some of the others.
> >> >> 
> >> >> For actual migration code issues:
> >> >> 
> >> >> # bugs | (sub)subsystem | kind
> >> >> ----------------------------------------------
> >> >>     13 | multifd        | correctness/races
> >> >>      8 | ram            | correctness
> >> >>      8 | rdma:          | general programming
> >> >
> >> > 8 rdma bugs??? ouch..
> >> 
> >> Mostly caught by a cleanup from Markus. Silly stuff like of mixed signed
> >> integer comparisons and bugs in error handling. I don't even want to
> >> look too much at it.
> >> 
> >> ...hopefully this release we'll manage to resolve that situation.
> >> 
> >> >
> >> >>      7 | qmp            | new api bugs
> >> >>      5 | postcopy       | races
> >> >>      4 | file:          | leaks
> >> >>      3 | return path    | races
> >> >>      3 | fd_cleanup     | races
> >> >>      2 | savevm, aio/coroutines
> >> >>      1 | xbzrle, colo, dirtyrate, exec:,
> >> >>           windows, iochannel, qemufile,
> >> >>           arch (ppc64le)
> >> >> 
> >> >> Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
> >> >> file, rp, fd_cleanup, iochannel, qemufile, xbzrle.
> >> >> 
> >> >> My suggestion is we run per arch:
> >> >> 
> >> >> "/precopy/tcp/plain"
> >> >> "/precopy/tcp/tls/psk/match",
> >> >> "/postcopy/plain"
> >> >> "/postcopy/preempt/plain"
> >> >> "/postcopy/preempt/recovery/plain"
> >> >> "/multifd/tcp/plain/cancel"
> >> >> "/multifd/tcp/uri/plain/none"
> >> >
> >> > Don't you want to still keep a few multifd / file tests?
> >> 
> >> Not really, but I won't object if you want to add some more in there. To
> >> be honest, I want to get out of people's way as much as I can because
> >> having to revisit this every couple of months is stressful to me.
> >
> > I hope migration tests are not too obstructive yet so far :) - this
> > discussion is still within a context where we might add one more slow test
> > in CI, and we probably simply should make it a -m slow test.
> >
> >> 
> >> My rationale for those is:
> >> 
> >> "/precopy/tcp/plain":
> >>  Smoke test, the most common migration
> >
> > E.g. unix is missing, and I'm not sure which we use for e.g. kubevirt.
> >
> > And note that even if unix shares the socket iochannel with tcp, it may not
> > work the same.  For example, if you remember I mentioned I was looking at
> > threadify the dest qemu receive side, i have a draft there but currently it
> > has a bug to hang a unix migration, not tcp..
> 
> Ok, let's add a unix one, no problem.
> 
> >
> > So IMHO it's not easy to justify which we can simply drop, if we still want
> > to provide some good gating in CI.
> 
> It's not exactly about dropping, it's about which ones need to be run at
> *every* invocation of make check (x5 because of the multiple archs) and

Yeah, indeed we could consider reducing the number of runs, maybe.  However
I do remember we used to have migration-test bugs only reproduced with some
specific distro, well..

Now I'm looking at the pipelines..

Why 5x, btw?  I saw alphine, centos, debian, fedora, opensuse, ubuntu,
cfi-x86_64, then compat-x86_64.  So that's 8x?  They're for the same arch
(amd64) so far.

Maybe we can skip some indeed.

> at every git branch push in CI (unless -o ci.skip). For gating we don't
> need all the tests. Many of them are testing the same core code with
> just a few differences at the margins.
> 
> >  And I won't be 100% surprised if some
> > other non-migration PR (e.g. io/) changed some slight bit that unix is
> > broken and tcp keeps working, for example, then we loose some CI benefits.
> 
> IMO, these non-migration PRs are exactly what we have to worry
> about. Because migration PRs would run with -m slow and we'd catch the
> issue there.
> 
> >
> >> 
> >> "/precopy/tcp/tls/psk/match":
> >>  Something might change in the distro regarding tls. Such as:
> >>  https://gitlab.com/qemu-project/qemu/-/issues/1937
> >> 
> >> "/postcopy/plain":
> >>  Smoke test for postcopy
> >> 
> >> "/postcopy/preempt/plain":
> >>  Just to exercise the preempt thread
> >> 
> >> "/postcopy/preempt/recovery/plain":
> >>  Recovery has had some issues with races in the past
> >> 
> >> "/multifd/tcp/plain/cancel":
> >>  The MVP of catching concurrency issues
> >> 
> >> "/multifd/tcp/uri/plain/none":
> >>  Smoke test for multifd
> >> 
> >> All in all, these will test basic funcionality and run very often. The
> >> more tests we add to this set, the less return we get in relation to the
> >> time they take.
> >
> > This is true.  We can try to discuss more on which is more important; I
> > still think something might be good to be added on top of above.
> 
> Sure, just add what you think we need.

Let's leave the detailed discussion on what to choose until the patch
phase.  IIUC this can be the last we do.

We've just queued your other patch to "slow down" the two time consuming
tests, we save 40s for each CI kick (total ~90s now locally, so that's 30%
cut already all acrosss!), not so bad as a start.

Then if we can remove some out of 8 (e.g., we can choose 2-3), then if it's
3 system runs, it'll be another 62% cut, a total of:

  1.0 - 9/13 * 3/8 = ~75%

So if we keep removing 5 system running it we can reduce it to only a
quarter time comparing to before for each CI run.

Would that be good enough already for us to live for quite a few more
releases?  They're so far all low hanging fruits.

If we want to move further (and you think we should then start with
reducing test case, rather than profiling the test cases), then feel free
to go ahead and send a patch when you're ready.  But maybe you don't want
to bother after you notice that we can already shrink 75% of runtime even
without dropping anything.  Your call!

> 
> >
> > There's also the other way - at some point, I still want to improve
> > migration-test run speed, and please have a look if you like too at some
> > point: so there's still chance (average is ~2sec as of now), IMHO, we don't
> > lose anything in CI but runs simply faster.
> 
> My only idea, but it requires a bit of work, is to do unit testing on
> the interfaces. Anything before migration_fd_connect(). Then we could
> stop doing a full migration for those.

What I'm thinking is some further profiling, like what Dan used to do.  I
feel like we still have chance, considering what we run as guest image is
simply a loop.. so basically zero boot time logically.

> 
> >
> >> 
> >> >
> >> > IIUC some file ops can still be relevant to archs.  Multifd still has one
> >> > bug that can only reproduce on arm64.. but not x86_64.  I remember it's a
> >> > race condition when migration finishes, and the issue could be memory
> >> > ordering relevant, but maybe not.
> >> 
> >> I'm not aware of anything. I believe the last arm64 bug we had was the
> >> threadinfo stuff[1]. If you remember what it's about, let me know.
> >> 
> >> 1- 01ec0f3a92 ("migration/multifd: Protect accesses to migration_threads").
> >
> > https://issues.redhat.com/browse/RHEL-45628
> 
> Interesting. But if multifd/tcp/plain/cancel doesn't catch this I don't
> think other tests will. Also, we don't have tests for zerocopy AFAIK.

That'll be challenging as it requires locked_vm limit.

> 
> >
> > On RH side Bandan is looking at it, but he's during a long holidays
> > recently.
> 
> Good luck to him.
> 
> >
> >> 
> >> >
> >> >> 
> >> >> and x86 gets extra:
> >> >> 
> >> >> "/precopy/unix/suspend/live"
> >> >> "/precopy/unix/suspend/notlive"
> >> >> "/dirty_ring"
> >> >
> >> > dirty ring will be disabled anyway when !x86, so probably not a major
> >> > concern.
> >> >
> >> >> 
> >> >> (the other dirty_* tests are too slow)
> >> >
> >> > These are the 10 slowest tests when I run locally:
> >> >
> >> > /x86_64/migration/multifd/tcp/tls/x509/allow-anon-client 2.41
> >> > /x86_64/migration/postcopy/recovery/plain 2.43
> >> > /x86_64/migration/multifd/tcp/tls/x509/default-host 2.66
> >> > /x86_64/migration/multifd/tcp/tls/x509/override-host 2.86
> >> > /x86_64/migration/postcopy/tls/psk 2.91
> >> > /x86_64/migration/postcopy/preempt/recovery/tls/psk 3.08
> >> > /x86_64/migration/postcopy/preempt/tls/psk 3.30
> >> > /x86_64/migration/postcopy/recovery/tls/psk 3.81
> >> > /x86_64/migration/vcpu_dirty_limit 13.29
> >> > /x86_64/migration/precopy/unix/xbzrle 27.55
> >> >
> >> > Are you aware of people using xbzrle at all?
> >> 
> >> Nope.
> >> 
> >> >
> >> >> 
> >> >> All the rest go behind a knob that people touching migration code will
> >> >> enable.
> >> >> 
> >> >> wdyt?
> >> >
> >> > Agree with the general idea, but I worry above exact list can be too small.
> >> 
> >> We won't stop running the full set of tests. We can run them in our
> >> forks' CI as much as we want. There are no cases of people reporting a
> >> migration bug because their 'make check' caught something that ours
> >> didn't.
> >
> > IIUC it's hard to say - when the test is in CI maintainers can catch them
> > already before sending a formal PR.
> >
> > If the test is run by default in make check, a developer can trigger a
> > migration issue (with his/her patch applied) then one can notice it
> > introduced a bug, fix it, then post the patches.  We won't know whether
> > that happened.
> 
> Good point.
> 
> >
> > So one thing we can do (if you think worthwhile to do it now) is we shrink
> > the default test case a lot as you proposed, then we wait and see what
> > breaks, and then we gradually add tests back when it can be used to find
> > breakages.  But that'll also take time if it really can find such tests,
> > because then we'll need to debug them one by one (instead of developer /
> > maintainer looking into them with their expertise knowledge..).  I'm not
> > sure whether it's worthwhile to do it now, but I don't feel strongly if we
> > can still have a reliable set of default test cases.
> 
> We first need a way to enable them for the migration CI. Do we have a
> variable that CI understands that can be used to enable slow tests?

I'm not aware of.  Yes sounds like we should have one if it doesn't exist.

> 
> >
> >> 
> >> Besides, the main strength of CI is to catch bugs when someone makes a
> >> code change. If people touch migration code, then we'll run it in our CI
> >> anyway. If they touch device code and that device is migrated by default
> >> then any one of the simple tests will catch the issue when it runs via
> >> the migration-compat job. If the device is not enabled by default, then
> >> no tests will catch it.
> >> 
> >> The worst case scenario is they touch some code completely unrelated and
> >> their 'make check' or CI run breaks because of some race in the
> >> migration code. That's not what CI is for, that's just an annoyance for
> >> everyone. I'd rather it breaks in our hands and then we'll go fix it.
> >> 
> >> >
> >> > IMHO we can definitely, at least, move the last two into slow list
> >> > (vcpu_dirty_limit and xbzrle), then it'll already save us 40sec each run..
> >> 
> >> Agreed. I'll send a patch once I get out from under downstream stuff.
> >> 
> >> >
> >> >> 
> >> >> 1- allows adding devices to QEMU cmdline for migration-test
> >> >> https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de
> >> >> 
> >> 
> 

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 11, 2024 at 04:48:21PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Tue, Sep 10, 2024 at 07:23:43PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Mon, Sep 09, 2024 at 06:54:46PM -0300, Fabiano Rosas wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> 
>> >> >> > On Mon, Sep 09, 2024 at 03:02:57PM +0100, Peter Maydell wrote:
>> >> >> >> On Mon, 9 Sept 2024 at 14:51, Hyman Huang <yong.huang@smartx.com> wrote:
>> >> >> >> >
>> >> >> >> > Despite the fact that the responsive CPU throttle is enabled,
>> >> >> >> > the dirty sync count may not always increase because this is
>> >> >> >> > an optimization that might not happen in any situation.
>> >> >> >> >
>> >> >> >> > This test case just making sure it doesn't interfere with any
>> >> >> >> > current functionality.
>> >> >> >> >
>> >> >> >> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
>> >> >> >> 
>> >> >> >> tests/qtest/migration-test already runs 75 different
>> >> >> >> subtests, takes up a massive chunk of our "make check"
>> >> >> >> time, and is very commonly a "times out" test on some
>> >> >> >> of our CI jobs. It runs on five different guest CPU
>> >> >> >> architectures, each one of which takes between 2 and
>> >> >> >> 5 minutes to complete the full migration-test.
>> >> >> >> 
>> >> >> >> Do we really need to make it even bigger?
>> >> >> >
>> >> >> > I'll try to find some time in the next few weeks looking into this to see
>> >> >> > whether we can further shrink migration test times after previous attemps
>> >> >> > from Dan.  At least a low hanging fruit is we should indeed put some more
>> >> >> > tests into g_test_slow(), and this new test could also be a candidate (then
>> >> >> > we can run "-m slow" for migration PRs only).
>> >> >> 
>> >> >> I think we could (using -m slow or any other method) separate tests
>> >> >> that are generic enough that every CI run should benefit from them
>> >> >> vs. tests that are only useful once someone starts touching migration
>> >> >> code. I'd say very few in the former category and most of them in the
>> >> >> latter.
>> >> >> 
>> >> >> For an idea of where migration bugs lie, I took a look at what was
>> >> >> fixed since 2022:
>> >> >> 
>> >> >> # bugs | device/subsystem/arch
>> >> >> ----------------------------------
>> >> >>     54 | migration
>> >> >>     10 | vfio
>> >> >>      6 | ppc
>> >> >>      3 | virtio-gpu
>> >> >>      2 | pcie_sriov, tpm_emulator,
>> >> >>           vdpa, virtio-rng-pci
>> >> >>      1 | arm, block, gpio, lasi,
>> >> >>           pci, s390, scsi-disk,
>> >> >>           virtio-mem, TCG
>> >> >
>> >> > Just curious; how did you collect these?
>> >> 
>> >> git log --since=2022 and then squinted at it. I wrote a warning to take
>> >> this with a grain of salt, but it missed the final version.
>> >> 
>> >> >
>> >> >> 
>> >> >> From these, ignoring the migration bugs, the migration-tests cover some
>> >> >> of: arm, ppc, s390, TCG. The device_opts[1] patch hasn't merged yet, but
>> >> >> once it is, then virtio-gpu would be covered and we could investigate
>> >> >> adding some of the others.
>> >> >> 
>> >> >> For actual migration code issues:
>> >> >> 
>> >> >> # bugs | (sub)subsystem | kind
>> >> >> ----------------------------------------------
>> >> >>     13 | multifd        | correctness/races
>> >> >>      8 | ram            | correctness
>> >> >>      8 | rdma:          | general programming
>> >> >
>> >> > 8 rdma bugs??? ouch..
>> >> 
>> >> Mostly caught by a cleanup from Markus. Silly stuff like of mixed signed
>> >> integer comparisons and bugs in error handling. I don't even want to
>> >> look too much at it.
>> >> 
>> >> ...hopefully this release we'll manage to resolve that situation.
>> >> 
>> >> >
>> >> >>      7 | qmp            | new api bugs
>> >> >>      5 | postcopy       | races
>> >> >>      4 | file:          | leaks
>> >> >>      3 | return path    | races
>> >> >>      3 | fd_cleanup     | races
>> >> >>      2 | savevm, aio/coroutines
>> >> >>      1 | xbzrle, colo, dirtyrate, exec:,
>> >> >>           windows, iochannel, qemufile,
>> >> >>           arch (ppc64le)
>> >> >> 
>> >> >> Here, the migration-tests cover well: multifd, ram, qmp, postcopy,
>> >> >> file, rp, fd_cleanup, iochannel, qemufile, xbzrle.
>> >> >> 
>> >> >> My suggestion is we run per arch:
>> >> >> 
>> >> >> "/precopy/tcp/plain"
>> >> >> "/precopy/tcp/tls/psk/match",
>> >> >> "/postcopy/plain"
>> >> >> "/postcopy/preempt/plain"
>> >> >> "/postcopy/preempt/recovery/plain"
>> >> >> "/multifd/tcp/plain/cancel"
>> >> >> "/multifd/tcp/uri/plain/none"
>> >> >
>> >> > Don't you want to still keep a few multifd / file tests?
>> >> 
>> >> Not really, but I won't object if you want to add some more in there. To
>> >> be honest, I want to get out of people's way as much as I can because
>> >> having to revisit this every couple of months is stressful to me.
>> >
>> > I hope migration tests are not too obstructive yet so far :) - this
>> > discussion is still within a context where we might add one more slow test
>> > in CI, and we probably simply should make it a -m slow test.
>> >
>> >> 
>> >> My rationale for those is:
>> >> 
>> >> "/precopy/tcp/plain":
>> >>  Smoke test, the most common migration
>> >
>> > E.g. unix is missing, and I'm not sure which we use for e.g. kubevirt.
>> >
>> > And note that even if unix shares the socket iochannel with tcp, it may not
>> > work the same.  For example, if you remember I mentioned I was looking at
>> > threadify the dest qemu receive side, i have a draft there but currently it
>> > has a bug to hang a unix migration, not tcp..
>> 
>> Ok, let's add a unix one, no problem.
>> 
>> >
>> > So IMHO it's not easy to justify which we can simply drop, if we still want
>> > to provide some good gating in CI.
>> 
>> It's not exactly about dropping, it's about which ones need to be run at
>> *every* invocation of make check (x5 because of the multiple archs) and
>
> Yeah, indeed we could consider reducing the number of runs, maybe.  However
> I do remember we used to have migration-test bugs only reproduced with some
> specific distro, well..
>
> Now I'm looking at the pipelines..
>
> Why 5x, btw?

5x because migration-test supports 5 architectures (i386, x86_64, ppc64,
aarch64, s390x), so if your build includes all of those, make check will
run migration-test five times.

>  I saw alphine, centos, debian, fedora, opensuse, ubuntu,
> cfi-x86_64, then compat-x86_64.  So that's 8x?  They're for the same arch
> (amd64) so far.

I agree we should fine tune this in CI, but this is a different
discussion. We can't currently skip migration-test without skipping make
check-qtest. I proposed a while ago to have a separate make
check-migration, but people didn't like that, maybe we should revisit
the idea.

>
> Maybe we can skip some indeed.
>
>> at every git branch push in CI (unless -o ci.skip). For gating we don't
>> need all the tests. Many of them are testing the same core code with
>> just a few differences at the margins.
>> 
>> >  And I won't be 100% surprised if some
>> > other non-migration PR (e.g. io/) changed some slight bit that unix is
>> > broken and tcp keeps working, for example, then we loose some CI benefits.
>> 
>> IMO, these non-migration PRs are exactly what we have to worry
>> about. Because migration PRs would run with -m slow and we'd catch the
>> issue there.
>> 
>> >
>> >> 
>> >> "/precopy/tcp/tls/psk/match":
>> >>  Something might change in the distro regarding tls. Such as:
>> >>  https://gitlab.com/qemu-project/qemu/-/issues/1937
>> >> 
>> >> "/postcopy/plain":
>> >>  Smoke test for postcopy
>> >> 
>> >> "/postcopy/preempt/plain":
>> >>  Just to exercise the preempt thread
>> >> 
>> >> "/postcopy/preempt/recovery/plain":
>> >>  Recovery has had some issues with races in the past
>> >> 
>> >> "/multifd/tcp/plain/cancel":
>> >>  The MVP of catching concurrency issues
>> >> 
>> >> "/multifd/tcp/uri/plain/none":
>> >>  Smoke test for multifd
>> >> 
>> >> All in all, these will test basic funcionality and run very often. The
>> >> more tests we add to this set, the less return we get in relation to the
>> >> time they take.
>> >
>> > This is true.  We can try to discuss more on which is more important; I
>> > still think something might be good to be added on top of above.
>> 
>> Sure, just add what you think we need.
>
> Let's leave the detailed discussion on what to choose until the patch
> phase.  IIUC this can be the last we do.

ok

>
> We've just queued your other patch to "slow down" the two time consuming
> tests, we save 40s for each CI kick (total ~90s now locally, so that's 30%
> cut already all acrosss!), not so bad as a start.
>
> Then if we can remove some out of 8 (e.g., we can choose 2-3), then if it's
> 3 system runs, it'll be another 62% cut, a total of:
>
>   1.0 - 9/13 * 3/8 = ~75%
>
> So if we keep removing 5 system running it we can reduce it to only a
> quarter time comparing to before for each CI run.
>
> Would that be good enough already for us to live for quite a few more
> releases?  They're so far all low hanging fruits.

Reducing the runs in CI is not a low hanging fruit. We'd need to first
split migration-test from check-qtest somehow. But that would mean not
running migration-test at all during make check, which is worse than
just reducing migration-test while keeping it in make check.

>
> If we want to move further (and you think we should then start with
> reducing test case, rather than profiling the test cases), then feel free
> to go ahead and send a patch when you're ready.  But maybe you don't want
> to bother after you notice that we can already shrink 75% of runtime even
> without dropping anything.  Your call!

I don't think we're discussing total CI time at this point, so the math
doesn't really add up. We're not looking into making the CI finish
faster. We're looking into making migration-test finish faster. That
would reduce timeouts in CI, speed-up make check and reduce the chance
of random race conditions* affecting other people/staging runs.

*- like the one I'm debugging at this very moment in multifd+ppc, are
   you aware of that btw?

>
>> 
>> >
>> > There's also the other way - at some point, I still want to improve
>> > migration-test run speed, and please have a look if you like too at some
>> > point: so there's still chance (average is ~2sec as of now), IMHO, we don't
>> > lose anything in CI but runs simply faster.
>> 
>> My only idea, but it requires a bit of work, is to do unit testing on
>> the interfaces. Anything before migration_fd_connect(). Then we could
>> stop doing a full migration for those.
>
> What I'm thinking is some further profiling, like what Dan used to do.  I
> feel like we still have chance, considering what we run as guest image is
> simply a loop.. so basically zero boot time logically.

I started looking again today into code coverage tools to see if we can
trim the tests that are redundant. But no promises, I already have a
queue of stuff that's pending.

>
>> 
>> >
>> >> 
>> >> >
>> >> > IIUC some file ops can still be relevant to archs.  Multifd still has one
>> >> > bug that can only reproduce on arm64.. but not x86_64.  I remember it's a
>> >> > race condition when migration finishes, and the issue could be memory
>> >> > ordering relevant, but maybe not.
>> >> 
>> >> I'm not aware of anything. I believe the last arm64 bug we had was the
>> >> threadinfo stuff[1]. If you remember what it's about, let me know.
>> >> 
>> >> 1- 01ec0f3a92 ("migration/multifd: Protect accesses to migration_threads").
>> >
>> > https://issues.redhat.com/browse/RHEL-45628
>> 
>> Interesting. But if multifd/tcp/plain/cancel doesn't catch this I don't
>> think other tests will. Also, we don't have tests for zerocopy AFAIK.
>
> That'll be challenging as it requires locked_vm limit.
>
>> 
>> >
>> > On RH side Bandan is looking at it, but he's during a long holidays
>> > recently.
>> 
>> Good luck to him.
>> 
>> >
>> >> 
>> >> >
>> >> >> 
>> >> >> and x86 gets extra:
>> >> >> 
>> >> >> "/precopy/unix/suspend/live"
>> >> >> "/precopy/unix/suspend/notlive"
>> >> >> "/dirty_ring"
>> >> >
>> >> > dirty ring will be disabled anyway when !x86, so probably not a major
>> >> > concern.
>> >> >
>> >> >> 
>> >> >> (the other dirty_* tests are too slow)
>> >> >
>> >> > These are the 10 slowest tests when I run locally:
>> >> >
>> >> > /x86_64/migration/multifd/tcp/tls/x509/allow-anon-client 2.41
>> >> > /x86_64/migration/postcopy/recovery/plain 2.43
>> >> > /x86_64/migration/multifd/tcp/tls/x509/default-host 2.66
>> >> > /x86_64/migration/multifd/tcp/tls/x509/override-host 2.86
>> >> > /x86_64/migration/postcopy/tls/psk 2.91
>> >> > /x86_64/migration/postcopy/preempt/recovery/tls/psk 3.08
>> >> > /x86_64/migration/postcopy/preempt/tls/psk 3.30
>> >> > /x86_64/migration/postcopy/recovery/tls/psk 3.81
>> >> > /x86_64/migration/vcpu_dirty_limit 13.29
>> >> > /x86_64/migration/precopy/unix/xbzrle 27.55
>> >> >
>> >> > Are you aware of people using xbzrle at all?
>> >> 
>> >> Nope.
>> >> 
>> >> >
>> >> >> 
>> >> >> All the rest go behind a knob that people touching migration code will
>> >> >> enable.
>> >> >> 
>> >> >> wdyt?
>> >> >
>> >> > Agree with the general idea, but I worry above exact list can be too small.
>> >> 
>> >> We won't stop running the full set of tests. We can run them in our
>> >> forks' CI as much as we want. There are no cases of people reporting a
>> >> migration bug because their 'make check' caught something that ours
>> >> didn't.
>> >
>> > IIUC it's hard to say - when the test is in CI maintainers can catch them
>> > already before sending a formal PR.
>> >
>> > If the test is run by default in make check, a developer can trigger a
>> > migration issue (with his/her patch applied) then one can notice it
>> > introduced a bug, fix it, then post the patches.  We won't know whether
>> > that happened.
>> 
>> Good point.
>> 
>> >
>> > So one thing we can do (if you think worthwhile to do it now) is we shrink
>> > the default test case a lot as you proposed, then we wait and see what
>> > breaks, and then we gradually add tests back when it can be used to find
>> > breakages.  But that'll also take time if it really can find such tests,
>> > because then we'll need to debug them one by one (instead of developer /
>> > maintainer looking into them with their expertise knowledge..).  I'm not
>> > sure whether it's worthwhile to do it now, but I don't feel strongly if we
>> > can still have a reliable set of default test cases.
>> 
>> We first need a way to enable them for the migration CI. Do we have a
>> variable that CI understands that can be used to enable slow tests?
>
> I'm not aware of.  Yes sounds like we should have one if it doesn't exist.
>
>> 
>> >
>> >> 
>> >> Besides, the main strength of CI is to catch bugs when someone makes a
>> >> code change. If people touch migration code, then we'll run it in our CI
>> >> anyway. If they touch device code and that device is migrated by default
>> >> then any one of the simple tests will catch the issue when it runs via
>> >> the migration-compat job. If the device is not enabled by default, then
>> >> no tests will catch it.
>> >> 
>> >> The worst case scenario is they touch some code completely unrelated and
>> >> their 'make check' or CI run breaks because of some race in the
>> >> migration code. That's not what CI is for, that's just an annoyance for
>> >> everyone. I'd rather it breaks in our hands and then we'll go fix it.
>> >> 
>> >> >
>> >> > IMHO we can definitely, at least, move the last two into slow list
>> >> > (vcpu_dirty_limit and xbzrle), then it'll already save us 40sec each run..
>> >> 
>> >> Agreed. I'll send a patch once I get out from under downstream stuff.
>> >> 
>> >> >
>> >> >> 
>> >> >> 1- allows adding devices to QEMU cmdline for migration-test
>> >> >> https://lore.kernel.org/r/20240523201922.28007-4-farosas@suse.de
>> >> >> 
>> >> 
>> 

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Maydell]

On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
> I don't think we're discussing total CI time at this point, so the math
> doesn't really add up. We're not looking into making the CI finish
> faster. We're looking into making migration-test finish faster. That
> would reduce timeouts in CI, speed-up make check and reduce the chance
> of random race conditions* affecting other people/staging runs.

Right. The reason migration-test appears on my radar is because
it is very frequently the thing that shows up as "this sometimes
just fails or just times out and if you hit retry it goes away
again". That might not be migration-test's fault specifically,
because those retries tend to be certain CI configs (s390,
the i686-tci one), and I have some theories about what might be
causing it (e.g. build system runs 4 migration-tests in parallel,
which means 8 QEMU processes which is too many for the number
of host CPUs). But right now I look at CI job failures and my reaction
is "oh, it's the migration-test failing yet again" :-(

For some examples from this week:

https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
https://gitlab.com/qemu-project/qemu/-/jobs/7786579155

-- PMM

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
>> I don't think we're discussing total CI time at this point, so the math
>> doesn't really add up. We're not looking into making the CI finish
>> faster. We're looking into making migration-test finish faster. That
>> would reduce timeouts in CI, speed-up make check and reduce the chance
>> of random race conditions* affecting other people/staging runs.
>
> Right. The reason migration-test appears on my radar is because
> it is very frequently the thing that shows up as "this sometimes
> just fails or just times out and if you hit retry it goes away
> again". That might not be migration-test's fault specifically,
> because those retries tend to be certain CI configs (s390,
> the i686-tci one), and I have some theories about what might be
> causing it (e.g. build system runs 4 migration-tests in parallel,
> which means 8 QEMU processes which is too many for the number
> of host CPUs). But right now I look at CI job failures and my reaction
> is "oh, it's the migration-test failing yet again" :-(

And then I go: "oh, people complaining about migration-test again, I
thought we had fixed all the issues this time". It's frustrating for
everyone, as I said previously.

>
> For some examples from this week:
>
> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155

About these:

There are 2 instances of plain-old-SIGSEGV here. Both happen in
non-x86_64 runs and on the /multifd/tcp/plain/cancel test, which means
they're either races or memory ordering issues. Having i386 crashing
points to the former. So having the CI loaded and causing timeouts is
probably what exposed the issue.

The thread is mig/dst/recv_7 and grepping the objdump output shows:
<set_bit_atomic> 55 48 89 e5 48 89 7d e8 48 89 75 e0 48 8b 45 e8 83 e0
3f ba 01 00 00 00 89 c1 48 d3 e2 48 89 d0 48 89 45 f0 48 8b 45 e8 48 c1
e8 06 48 8d 14 c5 00 00 00 00 48 8b 45 e0 48 01 d0 48 89 45 f8 48 8b 45
f8 48 8b 55 f0 <f0> 48 09 10 90 5d c3

I tried a bisect overnight, but it seems the issue has been there since
before 9.0. I'll try to repro with gdb attached or get a core dump.

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Maydell]

On Thu, 12 Sept 2024 at 14:48, Fabiano Rosas <farosas@suse.de> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > For some examples from this week:
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> > https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
> > https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
> > https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>
> About these:
>
> There are 2 instances of plain-old-SIGSEGV here. Both happen in
> non-x86_64 runs and on the /multifd/tcp/plain/cancel test, which means
> they're either races or memory ordering issues. Having i386 crashing
> points to the former. So having the CI loaded and causing timeouts is
> probably what exposed the issue.

They're also both TCI. Would these tests be relying on
specific atomic-access behaviour in the guest code that's
running, or is all the avoidance-of-races in the migration
code in QEMU itself?

(I don't know of any particular problems with TCI's
implementation of atomic accesses, so this is just a stab
in the dark.)

thanks
-- PMM

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 12 Sept 2024 at 14:48, Fabiano Rosas <farosas@suse.de> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > For some examples from this week:
>> >
>> > https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
>> > https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
>> > https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
>> > https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>>
>> About these:
>>
>> There are 2 instances of plain-old-SIGSEGV here. Both happen in
>> non-x86_64 runs and on the /multifd/tcp/plain/cancel test, which means
>> they're either races or memory ordering issues. Having i386 crashing
>> points to the former. So having the CI loaded and causing timeouts is
>> probably what exposed the issue.
>
> They're also both TCI. Would these tests be relying on
> specific atomic-access behaviour in the guest code that's
> running, or is all the avoidance-of-races in the migration
> code in QEMU itself?

I misspoke about memory ordering, this is all just the x86 host and the
multifd threads in QEMU having synchronization issues.

>
> (I don't know of any particular problems with TCI's
> implementation of atomic accesses, so this is just a stab
> in the dark.)
>
> thanks
> -- PMM

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
> > I don't think we're discussing total CI time at this point, so the math
> > doesn't really add up. We're not looking into making the CI finish
> > faster. We're looking into making migration-test finish faster. That
> > would reduce timeouts in CI, speed-up make check and reduce the chance
> > of random race conditions* affecting other people/staging runs.
> 
> Right. The reason migration-test appears on my radar is because
> it is very frequently the thing that shows up as "this sometimes
> just fails or just times out and if you hit retry it goes away
> again". That might not be migration-test's fault specifically,
> because those retries tend to be certain CI configs (s390,
> the i686-tci one), and I have some theories about what might be
> causing it (e.g. build system runs 4 migration-tests in parallel,
> which means 8 QEMU processes which is too many for the number
> of host CPUs). But right now I look at CI job failures and my reaction
> is "oh, it's the migration-test failing yet again" :-(
> 
> For some examples from this week:
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155

Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
parallel.  It indeed sounds like no good way to finally solve.. I don't
also see how speeding up / reducing tests in migration test would help, as
that's (from some degree..) is the same as tuning the timeout value bigger.
When the tests are less it'll fit into 480s window, but maybe it's too
quick now we wonder whether we should shrink it to e.g. 90s, but then it
can timeout again when on a busy host with less capability of concurrency.

But indeed there're two ERRORs ([1,2] above)..  I collected some more info
here before the log expires:

=================================8<================================

*** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host

https://gitlab.com/qemu-project/qemu/-/jobs/7799842373

101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stderr:
warning: fd: migration to a file is deprecated. Use file: instead.
warning: fd: migration to a file is deprecated. Use file: instead.
../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
(test program exited with status code -6)
TAP parsing error: Too few tests run (expected 53, got 39)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

# Start of plain tests
# Running /i386/migration/multifd/tcp/plain/cancel
# Using machine type: pc-i440fx-9.2
# starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
# starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
----------------------------------- stderr -----------------------------------
warning: fd: migration to a file is deprecated. Use file: instead.
warning: fd: migration to a file is deprecated. Use file: instead.
../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)

*** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host

https://gitlab.com/qemu-project/qemu/-/jobs/7786579152

174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
stderr:
qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
warning: fd: migration to a file is deprecated. Use file: instead.
warning: fd: migration to a file is deprecated. Use file: instead.
../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
(test program exited with status code -6)
TAP parsing error: Too few tests run (expected 61, got 47)
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

# Start of plain tests
# Running /ppc64/migration/multifd/tcp/plain/cancel
# Using machine type: pseries-9.2
# starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
# starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
----------------------------------- stderr -----------------------------------
qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
warning: fd: migration to a file is deprecated. Use file: instead.
warning: fd: migration to a file is deprecated. Use file: instead.
../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)

(test program exited with status code -6)
=================================8<================================

So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
different host / arch being tested.  What is more weird is the two failures
are different, the 2nd failure throw out a TLS error even though the test
doesn't yet have tls involved.

Fabiano, is this the issue you're looking at?

Peter, do you think it'll be helpful if we temporarily mark this test as
"slow" too so it's not run in CI (so we still run it ourselves when prepare
migration PR, with the hope that it can reproduce)?

Thanks,

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Maydell]

On Thu, 12 Sept 2024 at 16:09, Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
> > On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
> > > I don't think we're discussing total CI time at this point, so the math
> > > doesn't really add up. We're not looking into making the CI finish
> > > faster. We're looking into making migration-test finish faster. That
> > > would reduce timeouts in CI, speed-up make check and reduce the chance
> > > of random race conditions* affecting other people/staging runs.
> >
> > Right. The reason migration-test appears on my radar is because
> > it is very frequently the thing that shows up as "this sometimes
> > just fails or just times out and if you hit retry it goes away
> > again". That might not be migration-test's fault specifically,
> > because those retries tend to be certain CI configs (s390,
> > the i686-tci one), and I have some theories about what might be
> > causing it (e.g. build system runs 4 migration-tests in parallel,
> > which means 8 QEMU processes which is too many for the number
> > of host CPUs). But right now I look at CI job failures and my reaction
> > is "oh, it's the migration-test failing yet again" :-(
> >
> > For some examples from this week:
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> > https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
> > https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
> > https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>
> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
> parallel.  It indeed sounds like no good way to finally solve.. I don't
> also see how speeding up / reducing tests in migration test would help, as
> that's (from some degree..) is the same as tuning the timeout value bigger.
> When the tests are less it'll fit into 480s window, but maybe it's too
> quick now we wonder whether we should shrink it to e.g. 90s, but then it
> can timeout again when on a busy host with less capability of concurrency.

For the TIMEOUT part on cross-i686-tci I plan to try this patch:
https://patchew.org/QEMU/20240912151003.2045031-1-peter.maydell@linaro.org/
which makes 'make check' single-threaded; that will help to see
if the parallelism is a problem. (If it is then we might want
to do a more generalised approach rather than just for that one
CI job.)

> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
> here before the log expires:

> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
> different host / arch being tested.  What is more weird is the two failures
> are different, the 2nd failure throw out a TLS error even though the test
> doesn't yet have tls involved.
>
> Fabiano, is this the issue you're looking at?
>
> Peter, do you think it'll be helpful if we temporarily mark this test as
> "slow" too so it's not run in CI (so we still run it ourselves when prepare
> migration PR, with the hope that it can reproduce)?

If you think that specific test is flaky then I think that's
probably a good idea. As usual with this kind of thing,
probably best to have a comment next to the test noting
why and with a URL to a gitlab issue for it, so we don't
forget why we disabled the test.

-- PMM

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
>> > I don't think we're discussing total CI time at this point, so the math
>> > doesn't really add up. We're not looking into making the CI finish
>> > faster. We're looking into making migration-test finish faster. That
>> > would reduce timeouts in CI, speed-up make check and reduce the chance
>> > of random race conditions* affecting other people/staging runs.
>> 
>> Right. The reason migration-test appears on my radar is because
>> it is very frequently the thing that shows up as "this sometimes
>> just fails or just times out and if you hit retry it goes away
>> again". That might not be migration-test's fault specifically,
>> because those retries tend to be certain CI configs (s390,
>> the i686-tci one), and I have some theories about what might be
>> causing it (e.g. build system runs 4 migration-tests in parallel,
>> which means 8 QEMU processes which is too many for the number
>> of host CPUs). But right now I look at CI job failures and my reaction
>> is "oh, it's the migration-test failing yet again" :-(
>> 
>> For some examples from this week:
>> 
>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>
> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
> parallel.  It indeed sounds like no good way to finally solve.. I don't
> also see how speeding up / reducing tests in migration test would help, as
> that's (from some degree..) is the same as tuning the timeout value bigger.
> When the tests are less it'll fit into 480s window, but maybe it's too
> quick now we wonder whether we should shrink it to e.g. 90s, but then it
> can timeout again when on a busy host with less capability of concurrency.
>
> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
> here before the log expires:
>
> =================================8<================================
>
> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
>
> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
>
> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
> stderr:
> warning: fd: migration to a file is deprecated. Use file: instead.
> warning: fd: migration to a file is deprecated. Use file: instead.
> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> (test program exited with status code -6)
> TAP parsing error: Too few tests run (expected 53, got 39)
> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>
> # Start of plain tests
> # Running /i386/migration/multifd/tcp/plain/cancel
> # Using machine type: pc-i440fx-9.2
> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
> ----------------------------------- stderr -----------------------------------
> warning: fd: migration to a file is deprecated. Use file: instead.
> warning: fd: migration to a file is deprecated. Use file: instead.
> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>
> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
>
> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
>
> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
> stderr:
> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> warning: fd: migration to a file is deprecated. Use file: instead.
> warning: fd: migration to a file is deprecated. Use file: instead.
> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> (test program exited with status code -6)
> TAP parsing error: Too few tests run (expected 61, got 47)
> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>
> # Start of plain tests
> # Running /ppc64/migration/multifd/tcp/plain/cancel
> # Using machine type: pseries-9.2
> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
> ----------------------------------- stderr -----------------------------------
> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> warning: fd: migration to a file is deprecated. Use file: instead.
> warning: fd: migration to a file is deprecated. Use file: instead.
> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>
> (test program exited with status code -6)
> =================================8<================================
>
> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
> different host / arch being tested.  What is more weird is the two failures
> are different, the 2nd failure throw out a TLS error even though the test
> doesn't yet have tls involved.

I think that's just a parallel test being cancelled prematurely, either
due to the crash or due to the timeout.

>
> Fabiano, is this the issue you're looking at?

Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
with make -j$(nproc) check and another loop with tcp/plain/cancel. It
takes ~1h to hit. I've seen crashes with ppc64, s390 and
aarch64.

> Peter, do you think it'll be helpful if we temporarily mark this test as
> "slow" too so it's not run in CI (so we still run it ourselves when prepare
> migration PR, with the hope that it can reproduce)?
>
> Thanks,

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
>>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
>>> > I don't think we're discussing total CI time at this point, so the math
>>> > doesn't really add up. We're not looking into making the CI finish
>>> > faster. We're looking into making migration-test finish faster. That
>>> > would reduce timeouts in CI, speed-up make check and reduce the chance
>>> > of random race conditions* affecting other people/staging runs.
>>> 
>>> Right. The reason migration-test appears on my radar is because
>>> it is very frequently the thing that shows up as "this sometimes
>>> just fails or just times out and if you hit retry it goes away
>>> again". That might not be migration-test's fault specifically,
>>> because those retries tend to be certain CI configs (s390,
>>> the i686-tci one), and I have some theories about what might be
>>> causing it (e.g. build system runs 4 migration-tests in parallel,
>>> which means 8 QEMU processes which is too many for the number
>>> of host CPUs). But right now I look at CI job failures and my reaction
>>> is "oh, it's the migration-test failing yet again" :-(
>>> 
>>> For some examples from this week:
>>> 
>>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
>>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
>>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
>>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>>
>> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
>> parallel.  It indeed sounds like no good way to finally solve.. I don't
>> also see how speeding up / reducing tests in migration test would help, as
>> that's (from some degree..) is the same as tuning the timeout value bigger.
>> When the tests are less it'll fit into 480s window, but maybe it's too
>> quick now we wonder whether we should shrink it to e.g. 90s, but then it
>> can timeout again when on a busy host with less capability of concurrency.
>>
>> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
>> here before the log expires:
>>
>> =================================8<================================
>>
>> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
>>
>> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
>>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>> stderr:
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> (test program exited with status code -6)
>> TAP parsing error: Too few tests run (expected 53, got 39)
>> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>
>> # Start of plain tests
>> # Running /i386/migration/multifd/tcp/plain/cancel
>> # Using machine type: pc-i440fx-9.2
>> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>> ----------------------------------- stderr -----------------------------------
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>>
>> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
>>
>> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
>>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>> stderr:
>> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> (test program exited with status code -6)
>> TAP parsing error: Too few tests run (expected 61, got 47)
>> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>
>> # Start of plain tests
>> # Running /ppc64/migration/multifd/tcp/plain/cancel
>> # Using machine type: pseries-9.2
>> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>> ----------------------------------- stderr -----------------------------------
>> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> warning: fd: migration to a file is deprecated. Use file: instead.
>> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>>
>> (test program exited with status code -6)
>> =================================8<================================
>>
>> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
>> different host / arch being tested.  What is more weird is the two failures
>> are different, the 2nd failure throw out a TLS error even though the test
>> doesn't yet have tls involved.
>
> I think that's just a parallel test being cancelled prematurely, either
> due to the crash or due to the timeout.
>
>>
>> Fabiano, is this the issue you're looking at?
>
> Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
> with make -j$(nproc) check and another loop with tcp/plain/cancel. It
> takes ~1h to hit. I've seen crashes with ppc64, s390 and
> aarch64.
>

Ok, the issue is that after commit 5ef7e26bdb ("migration/multifd: solve
zero page causing multiple page faults"), the multifd code started using
the rb->receivedmap bitmap, which belongs to the ram code and is
initialized and *freed* from the ram SaveVMHandlers.

process_incoming_migration_co()        ...
  qemu_loadvm_state()                  multifd_nocomp_recv()
    qemu_loadvm_state_cleanup()          ramblock_recv_bitmap_set_offset()
      rb->receivedmap = NULL               set_bit_atomic(..., rb->receivedmap)
  ...
  migration_incoming_state_destroy()
    multifd_recv_cleanup()
      multifd_recv_terminate_threads(NULL)

Multifd threads are live until migration_incoming_state_destroy(), which
is called some time later.

>> Peter, do you think it'll be helpful if we temporarily mark this test as
>> "slow" too so it's not run in CI (so we still run it ourselves when prepare
>> migration PR, with the hope that it can reproduce)?
>>
>> Thanks,

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Thu, Sep 12, 2024 at 07:52:48PM -0300, Fabiano Rosas wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
> >>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
> >>> > I don't think we're discussing total CI time at this point, so the math
> >>> > doesn't really add up. We're not looking into making the CI finish
> >>> > faster. We're looking into making migration-test finish faster. That
> >>> > would reduce timeouts in CI, speed-up make check and reduce the chance
> >>> > of random race conditions* affecting other people/staging runs.
> >>> 
> >>> Right. The reason migration-test appears on my radar is because
> >>> it is very frequently the thing that shows up as "this sometimes
> >>> just fails or just times out and if you hit retry it goes away
> >>> again". That might not be migration-test's fault specifically,
> >>> because those retries tend to be certain CI configs (s390,
> >>> the i686-tci one), and I have some theories about what might be
> >>> causing it (e.g. build system runs 4 migration-tests in parallel,
> >>> which means 8 QEMU processes which is too many for the number
> >>> of host CPUs). But right now I look at CI job failures and my reaction
> >>> is "oh, it's the migration-test failing yet again" :-(
> >>> 
> >>> For some examples from this week:
> >>> 
> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
> >>
> >> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
> >> parallel.  It indeed sounds like no good way to finally solve.. I don't
> >> also see how speeding up / reducing tests in migration test would help, as
> >> that's (from some degree..) is the same as tuning the timeout value bigger.
> >> When the tests are less it'll fit into 480s window, but maybe it's too
> >> quick now we wonder whether we should shrink it to e.g. 90s, but then it
> >> can timeout again when on a busy host with less capability of concurrency.
> >>
> >> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
> >> here before the log expires:
> >>
> >> =================================8<================================
> >>
> >> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
> >>
> >> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
> >>
> >> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
> >>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
> >> stderr:
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >> (test program exited with status code -6)
> >> TAP parsing error: Too few tests run (expected 53, got 39)
> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >>
> >> # Start of plain tests
> >> # Running /i386/migration/multifd/tcp/plain/cancel
> >> # Using machine type: pc-i440fx-9.2
> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
> >> ----------------------------------- stderr -----------------------------------
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >>
> >> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
> >>
> >> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
> >>
> >> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
> >>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
> >> stderr:
> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >> (test program exited with status code -6)
> >> TAP parsing error: Too few tests run (expected 61, got 47)
> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >>
> >> # Start of plain tests
> >> # Running /ppc64/migration/multifd/tcp/plain/cancel
> >> # Using machine type: pseries-9.2
> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
> >> ----------------------------------- stderr -----------------------------------
> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >>
> >> (test program exited with status code -6)
> >> =================================8<================================
> >>
> >> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
> >> different host / arch being tested.  What is more weird is the two failures
> >> are different, the 2nd failure throw out a TLS error even though the test
> >> doesn't yet have tls involved.
> >
> > I think that's just a parallel test being cancelled prematurely, either
> > due to the crash or due to the timeout.
> >
> >>
> >> Fabiano, is this the issue you're looking at?
> >
> > Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
> > with make -j$(nproc) check and another loop with tcp/plain/cancel. It
> > takes ~1h to hit. I've seen crashes with ppc64, s390 and
> > aarch64.
> >
> 
> Ok, the issue is that after commit 5ef7e26bdb ("migration/multifd: solve
> zero page causing multiple page faults"), the multifd code started using
> the rb->receivedmap bitmap, which belongs to the ram code and is
> initialized and *freed* from the ram SaveVMHandlers.
> 
> process_incoming_migration_co()        ...
>   qemu_loadvm_state()                  multifd_nocomp_recv()
>     qemu_loadvm_state_cleanup()          ramblock_recv_bitmap_set_offset()
>       rb->receivedmap = NULL               set_bit_atomic(..., rb->receivedmap)
>   ...
>   migration_incoming_state_destroy()
>     multifd_recv_cleanup()
>       multifd_recv_terminate_threads(NULL)
> 
> Multifd threads are live until migration_incoming_state_destroy(), which
> is called some time later.

Thanks for the debugging.  Hmm I would expect loadvm should wait until all
ram is received somehow..

And when looking, I also found we're ambiguous on the cleanups
now.. probably a separate issue that not yet cause crashes.  I meant we
even don't have a consistent order to clean these in precopy / postcopy,
as:

We have this:

  qemu_loadvm_state
    qemu_loadvm_state_cleanup              <------------- [1]
  migration_bh_schedule(process_incoming_migration_bh, mis);
  (then in bh...)
  process_incoming_migration_bh
    migration_incoming_state_destroy       <------------- [2]

But then:
  
  postcopy_ram_listen_thread
    migration_incoming_state_destroy       <------------- [2]
    qemu_loadvm_state_cleanup              <------------- [1]

I think it makes more sense to do [2] after [1], so maybe postcopy needs
changing too..

> 
> >> Peter, do you think it'll be helpful if we temporarily mark this test as
> >> "slow" too so it's not run in CI (so we still run it ourselves when prepare
> >> migration PR, with the hope that it can reproduce)?
> >>
> >> Thanks,
> 

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Thu, Sep 12, 2024 at 04:14:20PM +0100, Peter Maydell wrote:
> On Thu, 12 Sept 2024 at 16:09, Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
> > > On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
> > > > I don't think we're discussing total CI time at this point, so the math
> > > > doesn't really add up. We're not looking into making the CI finish
> > > > faster. We're looking into making migration-test finish faster. That
> > > > would reduce timeouts in CI, speed-up make check and reduce the chance
> > > > of random race conditions* affecting other people/staging runs.
> > >
> > > Right. The reason migration-test appears on my radar is because
> > > it is very frequently the thing that shows up as "this sometimes
> > > just fails or just times out and if you hit retry it goes away
> > > again". That might not be migration-test's fault specifically,
> > > because those retries tend to be certain CI configs (s390,
> > > the i686-tci one), and I have some theories about what might be
> > > causing it (e.g. build system runs 4 migration-tests in parallel,
> > > which means 8 QEMU processes which is too many for the number
> > > of host CPUs). But right now I look at CI job failures and my reaction
> > > is "oh, it's the migration-test failing yet again" :-(
> > >
> > > For some examples from this week:
> > >
> > > https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> > > https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
> > > https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
> > > https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
> >
> > Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
> > parallel.  It indeed sounds like no good way to finally solve.. I don't
> > also see how speeding up / reducing tests in migration test would help, as
> > that's (from some degree..) is the same as tuning the timeout value bigger.
> > When the tests are less it'll fit into 480s window, but maybe it's too
> > quick now we wonder whether we should shrink it to e.g. 90s, but then it
> > can timeout again when on a busy host with less capability of concurrency.
> 
> For the TIMEOUT part on cross-i686-tci I plan to try this patch:
> https://patchew.org/QEMU/20240912151003.2045031-1-peter.maydell@linaro.org/
> which makes 'make check' single-threaded; that will help to see
> if the parallelism is a problem. (If it is then we might want
> to do a more generalised approach rather than just for that one
> CI job.)

Sounds good.

> 
> > But indeed there're two ERRORs ([1,2] above)..  I collected some more info
> > here before the log expires:
> 
> > So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
> > different host / arch being tested.  What is more weird is the two failures
> > are different, the 2nd failure throw out a TLS error even though the test
> > doesn't yet have tls involved.
> >
> > Fabiano, is this the issue you're looking at?
> >
> > Peter, do you think it'll be helpful if we temporarily mark this test as
> > "slow" too so it's not run in CI (so we still run it ourselves when prepare
> > migration PR, with the hope that it can reproduce)?
> 
> If you think that specific test is flaky then I think that's
> probably a good idea. As usual with this kind of thing,
> probably best to have a comment next to the test noting
> why and with a URL to a gitlab issue for it, so we don't
> forget why we disabled the test.

Looks like Fabiano root-caused the issue.  We'll see how that goes, or we
can prepare a patch to make it optional with the comments in place.

Thanks,

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Sep 12, 2024 at 07:52:48PM -0300, Fabiano Rosas wrote:
>> Fabiano Rosas <farosas@suse.de> writes:
>> 
>> > Peter Xu <peterx@redhat.com> writes:
>> >
>> >> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
>> >>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
>> >>> > I don't think we're discussing total CI time at this point, so the math
>> >>> > doesn't really add up. We're not looking into making the CI finish
>> >>> > faster. We're looking into making migration-test finish faster. That
>> >>> > would reduce timeouts in CI, speed-up make check and reduce the chance
>> >>> > of random race conditions* affecting other people/staging runs.
>> >>> 
>> >>> Right. The reason migration-test appears on my radar is because
>> >>> it is very frequently the thing that shows up as "this sometimes
>> >>> just fails or just times out and if you hit retry it goes away
>> >>> again". That might not be migration-test's fault specifically,
>> >>> because those retries tend to be certain CI configs (s390,
>> >>> the i686-tci one), and I have some theories about what might be
>> >>> causing it (e.g. build system runs 4 migration-tests in parallel,
>> >>> which means 8 QEMU processes which is too many for the number
>> >>> of host CPUs). But right now I look at CI job failures and my reaction
>> >>> is "oh, it's the migration-test failing yet again" :-(
>> >>> 
>> >>> For some examples from this week:
>> >>> 
>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>> >>
>> >> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
>> >> parallel.  It indeed sounds like no good way to finally solve.. I don't
>> >> also see how speeding up / reducing tests in migration test would help, as
>> >> that's (from some degree..) is the same as tuning the timeout value bigger.
>> >> When the tests are less it'll fit into 480s window, but maybe it's too
>> >> quick now we wonder whether we should shrink it to e.g. 90s, but then it
>> >> can timeout again when on a busy host with less capability of concurrency.
>> >>
>> >> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
>> >> here before the log expires:
>> >>
>> >> =================================8<================================
>> >>
>> >> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
>> >>
>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
>> >>
>> >> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
>> >>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>> >> stderr:
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >> (test program exited with status code -6)
>> >> TAP parsing error: Too few tests run (expected 53, got 39)
>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> >>
>> >> # Start of plain tests
>> >> # Running /i386/migration/multifd/tcp/plain/cancel
>> >> # Using machine type: pc-i440fx-9.2
>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>> >> ----------------------------------- stderr -----------------------------------
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >>
>> >> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
>> >>
>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
>> >>
>> >> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
>> >>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>> >> stderr:
>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >> (test program exited with status code -6)
>> >> TAP parsing error: Too few tests run (expected 61, got 47)
>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> >>
>> >> # Start of plain tests
>> >> # Running /ppc64/migration/multifd/tcp/plain/cancel
>> >> # Using machine type: pseries-9.2
>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>> >> ----------------------------------- stderr -----------------------------------
>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >>
>> >> (test program exited with status code -6)
>> >> =================================8<================================
>> >>
>> >> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
>> >> different host / arch being tested.  What is more weird is the two failures
>> >> are different, the 2nd failure throw out a TLS error even though the test
>> >> doesn't yet have tls involved.
>> >
>> > I think that's just a parallel test being cancelled prematurely, either
>> > due to the crash or due to the timeout.
>> >
>> >>
>> >> Fabiano, is this the issue you're looking at?
>> >
>> > Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
>> > with make -j$(nproc) check and another loop with tcp/plain/cancel. It
>> > takes ~1h to hit. I've seen crashes with ppc64, s390 and
>> > aarch64.
>> >
>> 
>> Ok, the issue is that after commit 5ef7e26bdb ("migration/multifd: solve
>> zero page causing multiple page faults"), the multifd code started using
>> the rb->receivedmap bitmap, which belongs to the ram code and is
>> initialized and *freed* from the ram SaveVMHandlers.
>> 
>> process_incoming_migration_co()        ...
>>   qemu_loadvm_state()                  multifd_nocomp_recv()
>>     qemu_loadvm_state_cleanup()          ramblock_recv_bitmap_set_offset()
>>       rb->receivedmap = NULL               set_bit_atomic(..., rb->receivedmap)
>>   ...
>>   migration_incoming_state_destroy()
>>     multifd_recv_cleanup()
>>       multifd_recv_terminate_threads(NULL)
>> 
>> Multifd threads are live until migration_incoming_state_destroy(), which
>> is called some time later.
>
> Thanks for the debugging.  Hmm I would expect loadvm should wait until all
> ram is received somehow..

Looks like a similar issue as when we didn't have the multifd_send sync
working correctly and ram code would run and do cleanup.

>
> And when looking, I also found we're ambiguous on the cleanups
> now.. probably a separate issue that not yet cause crashes.  I meant we
> even don't have a consistent order to clean these in precopy / postcopy,
> as:
>
> We have this:
>
>   qemu_loadvm_state
>     qemu_loadvm_state_cleanup              <------------- [1]
>   migration_bh_schedule(process_incoming_migration_bh, mis);
>   (then in bh...)
>   process_incoming_migration_bh
>     migration_incoming_state_destroy       <------------- [2]
>
> But then:
>   
>   postcopy_ram_listen_thread
>     migration_incoming_state_destroy       <------------- [2]
>     qemu_loadvm_state_cleanup              <------------- [1]
>
> I think it makes more sense to do [2] after [1], so maybe postcopy needs
> changing too..

Yes, ram_load_cleanup doesn't do much:

static int ram_load_cleanup(void *opaque)
{
    RAMBlock *rb;

    RAMBLOCK_FOREACH_NOT_IGNORED(rb) {
        qemu_ram_block_writeback(rb);
    }

    xbzrle_load_cleanup();

    return 0;
}

We can probably change the order just fine.

For the bug I'm now trying to add a helper to init/clean the receivedmap
and only call it from code that uses it (postcopy, multifd_recv),
instead of always init/clean along with ram_load_setup/cleanup.

>
>> 
>> >> Peter, do you think it'll be helpful if we temporarily mark this test as
>> >> "slow" too so it's not run in CI (so we still run it ourselves when prepare
>> >> migration PR, with the hope that it can reproduce)?
>> >>
>> >> Thanks,
>> 

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Thu, Sep 12, 2024 at 07:52:48PM -0300, Fabiano Rosas wrote:
>>> Fabiano Rosas <farosas@suse.de> writes:
>>> 
>>> > Peter Xu <peterx@redhat.com> writes:
>>> >
>>> >> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
>>> >>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
>>> >>> > I don't think we're discussing total CI time at this point, so the math
>>> >>> > doesn't really add up. We're not looking into making the CI finish
>>> >>> > faster. We're looking into making migration-test finish faster. That
>>> >>> > would reduce timeouts in CI, speed-up make check and reduce the chance
>>> >>> > of random race conditions* affecting other people/staging runs.
>>> >>> 
>>> >>> Right. The reason migration-test appears on my radar is because
>>> >>> it is very frequently the thing that shows up as "this sometimes
>>> >>> just fails or just times out and if you hit retry it goes away
>>> >>> again". That might not be migration-test's fault specifically,
>>> >>> because those retries tend to be certain CI configs (s390,
>>> >>> the i686-tci one), and I have some theories about what might be
>>> >>> causing it (e.g. build system runs 4 migration-tests in parallel,
>>> >>> which means 8 QEMU processes which is too many for the number
>>> >>> of host CPUs). But right now I look at CI job failures and my reaction
>>> >>> is "oh, it's the migration-test failing yet again" :-(
>>> >>> 
>>> >>> For some examples from this week:
>>> >>> 
>>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
>>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
>>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
>>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>>> >>
>>> >> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
>>> >> parallel.  It indeed sounds like no good way to finally solve.. I don't
>>> >> also see how speeding up / reducing tests in migration test would help, as
>>> >> that's (from some degree..) is the same as tuning the timeout value bigger.
>>> >> When the tests are less it'll fit into 480s window, but maybe it's too
>>> >> quick now we wonder whether we should shrink it to e.g. 90s, but then it
>>> >> can timeout again when on a busy host with less capability of concurrency.
>>> >>
>>> >> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
>>> >> here before the log expires:
>>> >>
>>> >> =================================8<================================
>>> >>
>>> >> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
>>> >>
>>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
>>> >>
>>> >> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
>>> >>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>>> >> stderr:
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>>> >> (test program exited with status code -6)
>>> >> TAP parsing error: Too few tests run (expected 53, got 39)
>>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>> >>
>>> >> # Start of plain tests
>>> >> # Running /i386/migration/multifd/tcp/plain/cancel
>>> >> # Using machine type: pc-i440fx-9.2
>>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>>> >> ----------------------------------- stderr -----------------------------------
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>>> >>
>>> >> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
>>> >>
>>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
>>> >>
>>> >> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
>>> >>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>>> >> stderr:
>>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>>> >> (test program exited with status code -6)
>>> >> TAP parsing error: Too few tests run (expected 61, got 47)
>>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>> >>
>>> >> # Start of plain tests
>>> >> # Running /ppc64/migration/multifd/tcp/plain/cancel
>>> >> # Using machine type: pseries-9.2
>>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>>> >> ----------------------------------- stderr -----------------------------------
>>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>>> >>
>>> >> (test program exited with status code -6)
>>> >> =================================8<================================
>>> >>
>>> >> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
>>> >> different host / arch being tested.  What is more weird is the two failures
>>> >> are different, the 2nd failure throw out a TLS error even though the test
>>> >> doesn't yet have tls involved.
>>> >
>>> > I think that's just a parallel test being cancelled prematurely, either
>>> > due to the crash or due to the timeout.
>>> >
>>> >>
>>> >> Fabiano, is this the issue you're looking at?
>>> >
>>> > Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
>>> > with make -j$(nproc) check and another loop with tcp/plain/cancel. It
>>> > takes ~1h to hit. I've seen crashes with ppc64, s390 and
>>> > aarch64.
>>> >
>>> 
>>> Ok, the issue is that after commit 5ef7e26bdb ("migration/multifd: solve
>>> zero page causing multiple page faults"), the multifd code started using
>>> the rb->receivedmap bitmap, which belongs to the ram code and is
>>> initialized and *freed* from the ram SaveVMHandlers.
>>> 
>>> process_incoming_migration_co()        ...
>>>   qemu_loadvm_state()                  multifd_nocomp_recv()
>>>     qemu_loadvm_state_cleanup()          ramblock_recv_bitmap_set_offset()
>>>       rb->receivedmap = NULL               set_bit_atomic(..., rb->receivedmap)
>>>   ...
>>>   migration_incoming_state_destroy()
>>>     multifd_recv_cleanup()
>>>       multifd_recv_terminate_threads(NULL)
>>> 
>>> Multifd threads are live until migration_incoming_state_destroy(), which
>>> is called some time later.
>>
>> Thanks for the debugging.  Hmm I would expect loadvm should wait until all
>> ram is received somehow..
>
> Looks like a similar issue as when we didn't have the multifd_send sync
> working correctly and ram code would run and do cleanup.

Btw, this is hard to debug, but I bet what's happening is that the
ram_load code itself is exiting due to qemufile error. So there wouldn't
be a way to make it wait for multifd.

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Peter Xu]

On Fri, Sep 13, 2024 at 12:17:40PM -0300, Fabiano Rosas wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> On Thu, Sep 12, 2024 at 07:52:48PM -0300, Fabiano Rosas wrote:
> >>> Fabiano Rosas <farosas@suse.de> writes:
> >>> 
> >>> > Peter Xu <peterx@redhat.com> writes:
> >>> >
> >>> >> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
> >>> >>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
> >>> >>> > I don't think we're discussing total CI time at this point, so the math
> >>> >>> > doesn't really add up. We're not looking into making the CI finish
> >>> >>> > faster. We're looking into making migration-test finish faster. That
> >>> >>> > would reduce timeouts in CI, speed-up make check and reduce the chance
> >>> >>> > of random race conditions* affecting other people/staging runs.
> >>> >>> 
> >>> >>> Right. The reason migration-test appears on my radar is because
> >>> >>> it is very frequently the thing that shows up as "this sometimes
> >>> >>> just fails or just times out and if you hit retry it goes away
> >>> >>> again". That might not be migration-test's fault specifically,
> >>> >>> because those retries tend to be certain CI configs (s390,
> >>> >>> the i686-tci one), and I have some theories about what might be
> >>> >>> causing it (e.g. build system runs 4 migration-tests in parallel,
> >>> >>> which means 8 QEMU processes which is too many for the number
> >>> >>> of host CPUs). But right now I look at CI job failures and my reaction
> >>> >>> is "oh, it's the migration-test failing yet again" :-(
> >>> >>> 
> >>> >>> For some examples from this week:
> >>> >>> 
> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
> >>> >>
> >>> >> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
> >>> >> parallel.  It indeed sounds like no good way to finally solve.. I don't
> >>> >> also see how speeding up / reducing tests in migration test would help, as
> >>> >> that's (from some degree..) is the same as tuning the timeout value bigger.
> >>> >> When the tests are less it'll fit into 480s window, but maybe it's too
> >>> >> quick now we wonder whether we should shrink it to e.g. 90s, but then it
> >>> >> can timeout again when on a busy host with less capability of concurrency.
> >>> >>
> >>> >> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
> >>> >> here before the log expires:
> >>> >>
> >>> >> =================================8<================================
> >>> >>
> >>> >> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
> >>> >>
> >>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
> >>> >>
> >>> >> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
> >>> >>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
> >>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
> >>> >> stderr:
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >>> >> (test program exited with status code -6)
> >>> >> TAP parsing error: Too few tests run (expected 53, got 39)
> >>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >>> >>
> >>> >> # Start of plain tests
> >>> >> # Running /i386/migration/multifd/tcp/plain/cancel
> >>> >> # Using machine type: pc-i440fx-9.2
> >>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
> >>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
> >>> >> ----------------------------------- stderr -----------------------------------
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >>> >>
> >>> >> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
> >>> >>
> >>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
> >>> >>
> >>> >> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
> >>> >>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
> >>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
> >>> >> stderr:
> >>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >>> >> (test program exited with status code -6)
> >>> >> TAP parsing error: Too few tests run (expected 61, got 47)
> >>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >>> >>
> >>> >> # Start of plain tests
> >>> >> # Running /ppc64/migration/multifd/tcp/plain/cancel
> >>> >> # Using machine type: pseries-9.2
> >>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
> >>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
> >>> >> ----------------------------------- stderr -----------------------------------
> >>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
> >>> >>
> >>> >> (test program exited with status code -6)
> >>> >> =================================8<================================
> >>> >>
> >>> >> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
> >>> >> different host / arch being tested.  What is more weird is the two failures
> >>> >> are different, the 2nd failure throw out a TLS error even though the test
> >>> >> doesn't yet have tls involved.
> >>> >
> >>> > I think that's just a parallel test being cancelled prematurely, either
> >>> > due to the crash or due to the timeout.
> >>> >
> >>> >>
> >>> >> Fabiano, is this the issue you're looking at?
> >>> >
> >>> > Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
> >>> > with make -j$(nproc) check and another loop with tcp/plain/cancel. It
> >>> > takes ~1h to hit. I've seen crashes with ppc64, s390 and
> >>> > aarch64.
> >>> >
> >>> 
> >>> Ok, the issue is that after commit 5ef7e26bdb ("migration/multifd: solve
> >>> zero page causing multiple page faults"), the multifd code started using
> >>> the rb->receivedmap bitmap, which belongs to the ram code and is
> >>> initialized and *freed* from the ram SaveVMHandlers.
> >>> 
> >>> process_incoming_migration_co()        ...
> >>>   qemu_loadvm_state()                  multifd_nocomp_recv()
> >>>     qemu_loadvm_state_cleanup()          ramblock_recv_bitmap_set_offset()
> >>>       rb->receivedmap = NULL               set_bit_atomic(..., rb->receivedmap)
> >>>   ...
> >>>   migration_incoming_state_destroy()
> >>>     multifd_recv_cleanup()
> >>>       multifd_recv_terminate_threads(NULL)
> >>> 
> >>> Multifd threads are live until migration_incoming_state_destroy(), which
> >>> is called some time later.
> >>
> >> Thanks for the debugging.  Hmm I would expect loadvm should wait until all
> >> ram is received somehow..
> >
> > Looks like a similar issue as when we didn't have the multifd_send sync
> > working correctly and ram code would run and do cleanup.
> 
> Btw, this is hard to debug, but I bet what's happening is that the
> ram_load code itself is exiting due to qemufile error. So there wouldn't
> be a way to make it wait for multifd.

One more thing is I remember one of the error is not the crash but some TLS
disconnection error.  I wonder which one you can reproduce and why that TLS
code can got kicked off in the multifd cancel test.  Perhaps the memory was
simply corrupted around, so bitmap ops can write to some other memory?

-- 
Peter Xu

Re: [PATCH RFC 10/10] tests/migration-tests: Add test case for responsive CPU throttle

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Sep 13, 2024 at 12:17:40PM -0300, Fabiano Rosas wrote:
>> Fabiano Rosas <farosas@suse.de> writes:
>> 
>> > Peter Xu <peterx@redhat.com> writes:
>> >
>> >> On Thu, Sep 12, 2024 at 07:52:48PM -0300, Fabiano Rosas wrote:
>> >>> Fabiano Rosas <farosas@suse.de> writes:
>> >>> 
>> >>> > Peter Xu <peterx@redhat.com> writes:
>> >>> >
>> >>> >> On Thu, Sep 12, 2024 at 09:13:16AM +0100, Peter Maydell wrote:
>> >>> >>> On Wed, 11 Sept 2024 at 22:26, Fabiano Rosas <farosas@suse.de> wrote:
>> >>> >>> > I don't think we're discussing total CI time at this point, so the math
>> >>> >>> > doesn't really add up. We're not looking into making the CI finish
>> >>> >>> > faster. We're looking into making migration-test finish faster. That
>> >>> >>> > would reduce timeouts in CI, speed-up make check and reduce the chance
>> >>> >>> > of random race conditions* affecting other people/staging runs.
>> >>> >>> 
>> >>> >>> Right. The reason migration-test appears on my radar is because
>> >>> >>> it is very frequently the thing that shows up as "this sometimes
>> >>> >>> just fails or just times out and if you hit retry it goes away
>> >>> >>> again". That might not be migration-test's fault specifically,
>> >>> >>> because those retries tend to be certain CI configs (s390,
>> >>> >>> the i686-tci one), and I have some theories about what might be
>> >>> >>> causing it (e.g. build system runs 4 migration-tests in parallel,
>> >>> >>> which means 8 QEMU processes which is too many for the number
>> >>> >>> of host CPUs). But right now I look at CI job failures and my reaction
>> >>> >>> is "oh, it's the migration-test failing yet again" :-(
>> >>> >>> 
>> >>> >>> For some examples from this week:
>> >>> >>> 
>> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7802183144
>> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373  <--------[1]
>> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152  <--------[2]
>> >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7786579155
>> >>> >>
>> >>> >> Ah right, the TIMEOUT is unfortunate, especially if tests can be run in
>> >>> >> parallel.  It indeed sounds like no good way to finally solve.. I don't
>> >>> >> also see how speeding up / reducing tests in migration test would help, as
>> >>> >> that's (from some degree..) is the same as tuning the timeout value bigger.
>> >>> >> When the tests are less it'll fit into 480s window, but maybe it's too
>> >>> >> quick now we wonder whether we should shrink it to e.g. 90s, but then it
>> >>> >> can timeout again when on a busy host with less capability of concurrency.
>> >>> >>
>> >>> >> But indeed there're two ERRORs ([1,2] above)..  I collected some more info
>> >>> >> here before the log expires:
>> >>> >>
>> >>> >> =================================8<================================
>> >>> >>
>> >>> >> *** /i386/migration/multifd/tcp/plain/cancel, qtest-i386 on s390 host
>> >>> >>
>> >>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7799842373
>> >>> >>
>> >>> >> 101/953 qemu:qtest+qtest-i386 / qtest-i386/migration-test                         ERROR          144.32s   killed by signal 6 SIGABRT
>> >>> >>>>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon G_TEST_DBUS_DAEMON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/tests/dbus-vmstate-daemon.sh PYTHON=/home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img MALLOC_PERTURB_=144 QTEST_QEMU_BINARY=./qemu-system-i386 /home/gitlab-runner/builds/zEr9wY_L/0/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>> >>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>> >>> >> stderr:
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >>> >> (test program exited with status code -6)
>> >>> >> TAP parsing error: Too few tests run (expected 53, got 39)
>> >>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> >>> >>
>> >>> >> # Start of plain tests
>> >>> >> # Running /i386/migration/multifd/tcp/plain/cancel
>> >>> >> # Using machine type: pc-i440fx-9.2
>> >>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name source,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/src_serial -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>> >>> >> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3273509.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3273509.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pc-i440fx-9.2, -name target,debug-threads=on -m 150M -serial file:/tmp/migration-test-4112T2/dest_serial -incoming defer -drive if=none,id=d0,file=/tmp/migration-test-4112T2/bootsect,format=raw -device ide-hd,drive=d0,secs=1,cyls=1,heads=1    2>/dev/null -accel qtest
>> >>> >> ----------------------------------- stderr -----------------------------------
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >>> >>
>> >>> >> *** /ppc64/migration/multifd/tcp/plain/cancel, qtest-ppc64 on i686 host
>> >>> >>
>> >>> >> https://gitlab.com/qemu-project/qemu/-/jobs/7786579152
>> >>> >>
>> >>> >> 174/315 qemu:qtest+qtest-ppc64 / qtest-ppc64/migration-test                       ERROR          381.00s   killed by signal 6 SIGABRT
>> >>> >>>>> PYTHON=/builds/qemu-project/qemu/build/pyvenv/bin/python3 QTEST_QEMU_IMG=./qemu-img G_TEST_DBUS_DAEMON=/builds/qemu-project/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_BINARY=./qemu-system-ppc64 MALLOC_PERTURB_=178 QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon /builds/qemu-project/qemu/build/tests/qtest/migration-test --tap -k
>> >>> >> ――――――――――――――――――――――――――――――――――――― ✀  ―――――――――――――――――――――――――――――――――――――
>> >>> >> stderr:
>> >>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >>> >> (test program exited with status code -6)
>> >>> >> TAP parsing error: Too few tests run (expected 61, got 47)
>> >>> >> ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> >>> >>
>> >>> >> # Start of plain tests
>> >>> >> # Running /ppc64/migration/multifd/tcp/plain/cancel
>> >>> >> # Using machine type: pseries-9.2
>> >>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name source,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/src_serial -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>> >>> >> # starting QEMU: exec ./qemu-system-ppc64 -qtest unix:/tmp/qtest-40766.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-40766.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine pseries-9.2,vsmt=8 -name target,debug-threads=on -m 256M -serial file:/tmp/migration-test-H0Z1T2/dest_serial -incoming defer -nodefaults -machine cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off, -bios /tmp/migration-test-H0Z1T2/bootsect    2>/dev/null -accel qtest
>> >>> >> ----------------------------------- stderr -----------------------------------
>> >>> >> qemu-system-ppc64: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> warning: fd: migration to a file is deprecated. Use file: instead.
>> >>> >> ../tests/qtest/libqtest.c:205: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped)
>> >>> >>
>> >>> >> (test program exited with status code -6)
>> >>> >> =================================8<================================
>> >>> >>
>> >>> >> So.. it's the same test (multifd/tcp/plain/cancel) that is failing on
>> >>> >> different host / arch being tested.  What is more weird is the two failures
>> >>> >> are different, the 2nd failure throw out a TLS error even though the test
>> >>> >> doesn't yet have tls involved.
>> >>> >
>> >>> > I think that's just a parallel test being cancelled prematurely, either
>> >>> > due to the crash or due to the timeout.
>> >>> >
>> >>> >>
>> >>> >> Fabiano, is this the issue you're looking at?
>> >>> >
>> >>> > Yes. I can reproduce locally by running 2 processes in parallel: 1 loop
>> >>> > with make -j$(nproc) check and another loop with tcp/plain/cancel. It
>> >>> > takes ~1h to hit. I've seen crashes with ppc64, s390 and
>> >>> > aarch64.
>> >>> >
>> >>> 
>> >>> Ok, the issue is that after commit 5ef7e26bdb ("migration/multifd: solve
>> >>> zero page causing multiple page faults"), the multifd code started using
>> >>> the rb->receivedmap bitmap, which belongs to the ram code and is
>> >>> initialized and *freed* from the ram SaveVMHandlers.
>> >>> 
>> >>> process_incoming_migration_co()        ...
>> >>>   qemu_loadvm_state()                  multifd_nocomp_recv()
>> >>>     qemu_loadvm_state_cleanup()          ramblock_recv_bitmap_set_offset()
>> >>>       rb->receivedmap = NULL               set_bit_atomic(..., rb->receivedmap)
>> >>>   ...
>> >>>   migration_incoming_state_destroy()
>> >>>     multifd_recv_cleanup()
>> >>>       multifd_recv_terminate_threads(NULL)
>> >>> 
>> >>> Multifd threads are live until migration_incoming_state_destroy(), which
>> >>> is called some time later.
>> >>
>> >> Thanks for the debugging.  Hmm I would expect loadvm should wait until all
>> >> ram is received somehow..
>> >
>> > Looks like a similar issue as when we didn't have the multifd_send sync
>> > working correctly and ram code would run and do cleanup.
>> 
>> Btw, this is hard to debug, but I bet what's happening is that the
>> ram_load code itself is exiting due to qemufile error. So there wouldn't
>> be a way to make it wait for multifd.
>
> One more thing is I remember one of the error is not the crash but some TLS
> disconnection error.  I wonder which one you can reproduce and why that TLS
> code can got kicked off in the multifd cancel test.  Perhaps the memory was
> simply corrupted around, so bitmap ops can write to some other memory?

I haven't seen that error. I said in another email that I think that is
due to the timeout cancelling the tests forcefully.