[PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[Peter Xu]

Prasad reported a misalignment issue with query-migrationthreads v.s. the
recently migration thread name changes.  So I prepared patch 1, which will
make the main thread on src be named the same way reported either in
pthread or QMP query-migrationthreads API.

Then I found quite something missing around query-migrationthreads.  E.g.,
it so far only accounts multifd sender threads, while it ignored too many
other kinds of migration threads (either postcopy ones, destination multifd
threads, temporary threads etc.).  It means even if an admin can get TIDs
on src QEMU and does pinning (assuming that was the goal of the original
API), it won't be able to do the same for dest QEMUs, which seems to lose
its purpose.

HMP is also missing, I added it too, as thread IDs can definitely be good
candidates during debugging.  If we have QMP ready, it sounds like it
should naturally fit the HMP one too.

I did some cleanups here and there all around.  Feel free to have a look,
thanks.

CI: https://gitlab.com/peterx/qemu/-/pipelines/1475958754

Peter Xu (7):
  migration: Unify names of migration src main thread
  migration: Put thread names together with macros
  migration: Remove thread_id in migration_threads_add()
  migration: Simplify migration-threads API
  migration: Add all threads with QMP query-migrationthreads
  migration: Remove MigrationThread and threadinfo.h
  hmp: Add "info migrationthreads"

 include/monitor/hmp.h          |  1 +
 migration/migration.h          | 17 ++++++++++
 migration/threadinfo.h         | 25 --------------
 migration/colo.c               | 10 ++++--
 migration/dirtyrate.c          | 13 ++++++--
 migration/migration-hmp-cmds.c | 25 ++++++++++++++
 migration/migration.c          | 19 ++++++-----
 migration/multifd.c            | 18 ++++++----
 migration/postcopy-ram.c       | 16 ++++++---
 migration/savevm.c             | 13 +++++---
 migration/threadinfo.c         | 61 ++++++++++++++++++++--------------
 hmp-commands-info.hx           | 13 ++++++++
 12 files changed, 150 insertions(+), 81 deletions(-)
 delete mode 100644 migration/threadinfo.h

-- 
2.45.0

[PATCH 1/7] migration: Unify names of migration src main thread

[Peter Xu]

Make pthread name match with what we report in query-migrationthreads.

We used to report the same, but not anymore after 60ce47675d ("migration:
Rename thread debug names").  Not a huge deal but it was still good to
follow.

Multifd threads are still matched because at least on the src the QMP
facility reused p->name.  The QMP command missed dest multifd threads
though, but that's another thing to address later.

Not copy stable, as we'd better keep the names not changed for stable
branches in QMP responses (even though we shouldn't guarantee ABI stability
on the names anyway).

Cc: Juraj Marcin <jmarcin@redhat.com>
Reported-by: Prasad Pandit <ppandit@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h | 2 ++
 migration/migration.c | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 38aa1402d5..519455796d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -28,6 +28,8 @@
 #include "sysemu/runstate.h"
 #include "migration/misc.h"
 
+#define  MIGRATION_THREAD_SRC_MAIN  "mig/src/main"
+
 struct PostcopyBlocktimeContext;
 
 #define  MIGRATION_RESUME_ACK_VALUE  (1)
diff --git a/migration/migration.c b/migration/migration.c
index ae2be31557..505b62c8f3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3467,7 +3467,8 @@ static void *migration_thread(void *opaque)
     Error *local_err = NULL;
     int ret;
 
-    thread = migration_threads_add("live_migration", qemu_get_thread_id());
+    thread = migration_threads_add(MIGRATION_THREAD_SRC_MAIN,
+                                   qemu_get_thread_id());
 
     rcu_register_thread();
 
@@ -3820,7 +3821,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         qemu_thread_create(&s->thread, "mig/snapshot",
                 bg_migration_thread, s, QEMU_THREAD_JOINABLE);
     } else {
-        qemu_thread_create(&s->thread, "mig/src/main",
+        qemu_thread_create(&s->thread, MIGRATION_THREAD_SRC_MAIN,
                 migration_thread, s, QEMU_THREAD_JOINABLE);
     }
     s->migration_thread_running = true;
-- 
2.45.0

[PATCH 2/7] migration: Put thread names together with macros

[Peter Xu]

Since we have places where there can be more than one reference to the
thread names, time to clean up the thread names with macros so that they're
even cleaner when put together.

Still two functional changes below:

  - There's one dirty rate thread that we overlooked before, now we add
  that too and name it as "mig/dirtyrate" following the old rules.

  - The old name "mig/src/rp-thr" has "-thr" but it may not be useful if
  it's a thread name anyway, while "rp" can be slightly hard to read.
  Taking this chance to rename it to "mig/src/return", hopefully a better
  name.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h    | 14 +++++++++++++-
 migration/colo.c         |  3 ++-
 migration/dirtyrate.c    |  6 ++++--
 migration/migration.c    |  4 ++--
 migration/multifd.c      |  6 +++---
 migration/postcopy-ram.c |  6 ++++--
 migration/savevm.c       |  3 ++-
 7 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 519455796d..b9ce5aa4ff 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -28,7 +28,19 @@
 #include "sysemu/runstate.h"
 #include "migration/misc.h"
 
-#define  MIGRATION_THREAD_SRC_MAIN  "mig/src/main"
+#define  MIGRATION_THREAD_SNAPSHOT          "mig/snapshot"
+#define  MIGRATION_THREAD_DIRTY_RATE        "mig/dirtyrate"
+
+#define  MIGRATION_THREAD_SRC_MAIN          "mig/src/main"
+#define  MIGRATION_THREAD_SRC_MULTIFD       "mig/src/send_%d"
+#define  MIGRATION_THREAD_SRC_RETURN        "mig/src/return"
+#define  MIGRATION_THREAD_SRC_TLS           "mig/src/tls"
+
+#define  MIGRATION_THREAD_DST_COLO          "mig/dst/colo"
+#define  MIGRATION_THREAD_DST_MULTIFD       "mig/src/recv_%d"
+#define  MIGRATION_THREAD_DST_FAULT         "mig/dst/fault"
+#define  MIGRATION_THREAD_DST_LISTEN        "mig/dst/listen"
+#define  MIGRATION_THREAD_DST_PREEMPT       "mig/dst/preempt"
 
 struct PostcopyBlocktimeContext;
 
diff --git a/migration/colo.c b/migration/colo.c
index 6449490221..9590f281d0 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -935,7 +935,8 @@ void coroutine_fn colo_incoming_co(void)
     assert(bql_locked());
     assert(migration_incoming_colo_enabled());
 
-    qemu_thread_create(&th, "mig/dst/colo", colo_process_incoming_thread,
+    qemu_thread_create(&th, MIGRATION_THREAD_DST_COLO,
+                       colo_process_incoming_thread,
                        mis, QEMU_THREAD_JOINABLE);
 
     mis->colo_incoming_co = qemu_coroutine_self();
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 5478d58de3..2339ba400d 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -29,6 +29,7 @@
 #include "sysemu/runstate.h"
 #include "exec/memory.h"
 #include "qemu/xxhash.h"
+#include "migration.h"
 
 /*
  * total_dirty_pages is procted by BQL and is used
@@ -839,8 +840,9 @@ void qmp_calc_dirty_rate(int64_t calc_time,
 
     init_dirtyrate_stat(config);
 
-    qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
-                       (void *)&config, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&thread, MIGRATION_THREAD_DIRTY_RATE,
+                       get_dirtyrate_thread, (void *)&config,
+                       QEMU_THREAD_DETACHED);
 }
 
 
diff --git a/migration/migration.c b/migration/migration.c
index 505b62c8f3..813c45ad04 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2478,7 +2478,7 @@ static int open_return_path_on_source(MigrationState *ms)
 
     trace_open_return_path_on_source();
 
-    qemu_thread_create(&ms->rp_state.rp_thread, "mig/src/rp-thr",
+    qemu_thread_create(&ms->rp_state.rp_thread, MIGRATION_THREAD_SRC_RETURN,
                        source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
     ms->rp_state.rp_thread_created = true;
 
@@ -3818,7 +3818,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     }
 
     if (migrate_background_snapshot()) {
-        qemu_thread_create(&s->thread, "mig/snapshot",
+        qemu_thread_create(&s->thread, MIGRATION_THREAD_SNAPSHOT,
                 bg_migration_thread, s, QEMU_THREAD_JOINABLE);
     } else {
         qemu_thread_create(&s->thread, MIGRATION_THREAD_SRC_MAIN,
diff --git a/migration/multifd.c b/migration/multifd.c
index 9b200f4ad9..697fe86fdf 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -723,7 +723,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     args->p = p;
 
     p->tls_thread_created = true;
-    qemu_thread_create(&p->tls_thread, "mig/src/tls",
+    qemu_thread_create(&p->tls_thread, MIGRATION_THREAD_SRC_TLS,
                        multifd_tls_handshake_thread, args,
                        QEMU_THREAD_JOINABLE);
     return true;
@@ -841,7 +841,7 @@ bool multifd_send_setup(void)
                           + sizeof(uint64_t) * page_count;
             p->packet = g_malloc0(p->packet_len);
         }
-        p->name = g_strdup_printf("mig/src/send_%d", i);
+        p->name = g_strdup_printf(MIGRATION_THREAD_SRC_MULTIFD, i);
         p->write_flags = 0;
 
         if (!multifd_new_send_channel_create(p, &local_err)) {
@@ -1259,7 +1259,7 @@ int multifd_recv_setup(Error **errp)
                 + sizeof(uint64_t) * page_count;
             p->packet = g_malloc0(p->packet_len);
         }
-        p->name = g_strdup_printf("mig/dst/recv_%d", i);
+        p->name = g_strdup_printf(MIGRATION_THREAD_DST_MULTIFD, i);
         p->normal = g_new0(ram_addr_t, page_count);
         p->zero = g_new0(ram_addr_t, page_count);
     }
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 83f6160a36..a535fd2e30 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1230,7 +1230,8 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
         return -1;
     }
 
-    postcopy_thread_create(mis, &mis->fault_thread, "mig/dst/fault",
+    postcopy_thread_create(mis, &mis->fault_thread,
+                           MIGRATION_THREAD_DST_FAULT,
                            postcopy_ram_fault_thread, QEMU_THREAD_JOINABLE);
     mis->have_fault_thread = true;
 
@@ -1250,7 +1251,8 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
          * This thread needs to be created after the temp pages because
          * it'll fetch RAM_CHANNEL_POSTCOPY PostcopyTmpPage immediately.
          */
-        postcopy_thread_create(mis, &mis->postcopy_prio_thread, "mig/dst/preempt",
+        postcopy_thread_create(mis, &mis->postcopy_prio_thread,
+                               MIGRATION_THREAD_DST_PREEMPT,
                                postcopy_preempt_thread, QEMU_THREAD_JOINABLE);
         mis->preempt_thread_status = PREEMPT_THREAD_CREATED;
     }
diff --git a/migration/savevm.c b/migration/savevm.c
index 7e1e27182a..e796436979 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2131,7 +2131,8 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
     }
 
     mis->have_listen_thread = true;
-    postcopy_thread_create(mis, &mis->listen_thread, "mig/dst/listen",
+    postcopy_thread_create(mis, &mis->listen_thread,
+                           MIGRATION_THREAD_DST_LISTEN,
                            postcopy_ram_listen_thread, QEMU_THREAD_DETACHED);
     trace_loadvm_postcopy_handle_listen("return");
 
-- 
2.45.0

[PATCH 3/7] migration: Remove thread_id in migration_threads_add()

[Peter Xu]

It always fetches the ID of the curren thread, so there's no point passing
it over.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/threadinfo.h | 2 +-
 migration/migration.c  | 3 +--
 migration/multifd.c    | 2 +-
 migration/threadinfo.c | 5 +++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/threadinfo.h b/migration/threadinfo.h
index 2f356ff312..d0e4ab0aa3 100644
--- a/migration/threadinfo.h
+++ b/migration/threadinfo.h
@@ -21,5 +21,5 @@ struct MigrationThread {
     QLIST_ENTRY(MigrationThread) node;
 };
 
-MigrationThread *migration_threads_add(const char *name, int thread_id);
+MigrationThread *migration_threads_add(const char *name);
 void migration_threads_remove(MigrationThread *info);
diff --git a/migration/migration.c b/migration/migration.c
index 813c45ad04..1ddcf54a70 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3467,8 +3467,7 @@ static void *migration_thread(void *opaque)
     Error *local_err = NULL;
     int ret;
 
-    thread = migration_threads_add(MIGRATION_THREAD_SRC_MAIN,
-                                   qemu_get_thread_id());
+    thread = migration_threads_add(MIGRATION_THREAD_SRC_MAIN);
 
     rcu_register_thread();
 
diff --git a/migration/multifd.c b/migration/multifd.c
index 697fe86fdf..04db886c7e 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -575,7 +575,7 @@ static void *multifd_send_thread(void *opaque)
     int ret = 0;
     bool use_packets = multifd_use_packets();
 
-    thread = migration_threads_add(p->name, qemu_get_thread_id());
+    thread = migration_threads_add(p->name);
 
     trace_multifd_send_thread_start(p->id);
     rcu_register_thread();
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 262990dd75..8069413091 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -23,11 +23,12 @@ static void __attribute__((constructor)) migration_threads_init(void)
     qemu_mutex_init(&migration_threads_lock);
 }
 
-MigrationThread *migration_threads_add(const char *name, int thread_id)
+MigrationThread *migration_threads_add(const char *name)
 {
     MigrationThread *thread =  g_new0(MigrationThread, 1);
+
     thread->name = name;
-    thread->thread_id = thread_id;
+    thread->thread_id = qemu_get_thread_id();
 
     WITH_QEMU_LOCK_GUARD(&migration_threads_lock) {
         QLIST_INSERT_HEAD(&migration_threads, thread, node);
-- 
2.45.0

[PATCH 4/7] migration: Simplify migration-threads API

[Peter Xu]

There's no need to return a thread struct, because both the add/remove APIs
must be used in the working threads so tid would work.

Similar to the fact we don't need to pass in tid in each call sites, we
don't need the thread struct for removal too because tid is always in the
context.  Remove it in both add & remove APIs.  Instead making sure when
remove a thread the tid is always there.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/threadinfo.h |  4 ++--
 migration/migration.c  |  5 ++---
 migration/multifd.c    |  5 ++---
 migration/threadinfo.c | 19 ++++++++++++++-----
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/migration/threadinfo.h b/migration/threadinfo.h
index d0e4ab0aa3..7c86ae8763 100644
--- a/migration/threadinfo.h
+++ b/migration/threadinfo.h
@@ -21,5 +21,5 @@ struct MigrationThread {
     QLIST_ENTRY(MigrationThread) node;
 };
 
-MigrationThread *migration_threads_add(const char *name);
-void migration_threads_remove(MigrationThread *info);
+void migration_threads_add(const char *name);
+void migration_threads_remove(void);
diff --git a/migration/migration.c b/migration/migration.c
index 1ddcf54a70..74b2c1c627 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3460,14 +3460,13 @@ static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
 static void *migration_thread(void *opaque)
 {
     MigrationState *s = opaque;
-    MigrationThread *thread = NULL;
     int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     MigThrError thr_error;
     bool urgent = false;
     Error *local_err = NULL;
     int ret;
 
-    thread = migration_threads_add(MIGRATION_THREAD_SRC_MAIN);
+    migration_threads_add(MIGRATION_THREAD_SRC_MAIN);
 
     rcu_register_thread();
 
@@ -3566,7 +3565,7 @@ out:
     migration_iteration_finish(s);
     object_unref(OBJECT(s));
     rcu_unregister_thread();
-    migration_threads_remove(thread);
+    migration_threads_remove();
     return NULL;
 }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index 04db886c7e..2738d78407 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -570,12 +570,11 @@ int multifd_send_sync_main(void)
 static void *multifd_send_thread(void *opaque)
 {
     MultiFDSendParams *p = opaque;
-    MigrationThread *thread = NULL;
     Error *local_err = NULL;
     int ret = 0;
     bool use_packets = multifd_use_packets();
 
-    thread = migration_threads_add(p->name);
+    migration_threads_add(p->name);
 
     trace_multifd_send_thread_start(p->id);
     rcu_register_thread();
@@ -669,7 +668,7 @@ out:
     }
 
     rcu_unregister_thread();
-    migration_threads_remove(thread);
+    migration_threads_remove();
     trace_multifd_send_thread_end(p->id, p->packets_sent);
 
     return NULL;
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 8069413091..25e77404e2 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -23,7 +23,7 @@ static void __attribute__((constructor)) migration_threads_init(void)
     qemu_mutex_init(&migration_threads_lock);
 }
 
-MigrationThread *migration_threads_add(const char *name)
+void migration_threads_add(const char *name)
 {
     MigrationThread *thread =  g_new0(MigrationThread, 1);
 
@@ -33,17 +33,26 @@ MigrationThread *migration_threads_add(const char *name)
     WITH_QEMU_LOCK_GUARD(&migration_threads_lock) {
         QLIST_INSERT_HEAD(&migration_threads, thread, node);
     }
-
-    return thread;
 }
 
-void migration_threads_remove(MigrationThread *thread)
+void migration_threads_remove(void)
 {
+    int tid = qemu_get_thread_id();
+    MigrationThread *thread;
+
     QEMU_LOCK_GUARD(&migration_threads_lock);
-    if (thread) {
+
+    QLIST_FOREACH(thread, &migration_threads, node) {
+        if (tid != thread->thread_id) {
+            continue;
+        }
+
         QLIST_REMOVE(thread, node);
         g_free(thread);
+        return;
     }
+
+    g_assert_not_reached();
 }
 
 MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
-- 
2.45.0

[PATCH 5/7] migration: Add all threads with QMP query-migrationthreads

[Peter Xu]

The QMP interface query-migrationthreads lacks a lot of migration threads
but only reports multifd sender threads.  That's incomplete.

Since I'm at this, make all threads available to the QMP responses.

NOTE: there're a few changes that should fix some bugs on e.g. not
unregister rcu threads on failure paths, but I didn't make them separate
because they do not exist in live migration main logics but only either
COLO or dirty rate thread on rare failures.  I'd not bother, but if anyone
things we should split it out feel free to shoot.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h    |  3 +++
 migration/threadinfo.h   |  3 ---
 migration/colo.c         |  7 +++++--
 migration/dirtyrate.c    |  7 ++++++-
 migration/migration.c    |  7 ++++++-
 migration/multifd.c      |  6 ++++++
 migration/postcopy-ram.c | 10 +++++++---
 migration/savevm.c       | 10 ++++++----
 migration/threadinfo.c   |  1 +
 9 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index b9ce5aa4ff..85ec203a01 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -551,4 +551,7 @@ int migration_rp_wait(MigrationState *s);
  */
 void migration_rp_kick(MigrationState *s);
 
+void migration_threads_add(const char *name);
+void migration_threads_remove(void);
+
 #endif
diff --git a/migration/threadinfo.h b/migration/threadinfo.h
index 7c86ae8763..59f334af21 100644
--- a/migration/threadinfo.h
+++ b/migration/threadinfo.h
@@ -20,6 +20,3 @@ struct MigrationThread {
     int thread_id; /* ID of the underlying host thread */
     QLIST_ENTRY(MigrationThread) node;
 };
-
-void migration_threads_add(const char *name);
-void migration_threads_remove(void);
diff --git a/migration/colo.c b/migration/colo.c
index 9590f281d0..2f5cb96a90 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -823,6 +823,7 @@ static void *colo_process_incoming_thread(void *opaque)
     QIOChannelBuffer *bioc = NULL; /* Cache incoming device state */
     Error *local_err = NULL;
 
+    migration_threads_add(MIGRATION_THREAD_DST_COLO);
     rcu_register_thread();
     qemu_sem_init(&mis->colo_incoming_sem, 0);
 
@@ -831,7 +832,7 @@ static void *colo_process_incoming_thread(void *opaque)
 
     if (get_colo_mode() != COLO_MODE_SECONDARY) {
         error_report("COLO mode must be COLO_MODE_SECONDARY");
-        return NULL;
+        goto out_last;
     }
 
     /* Make sure all file formats throw away their mutable metadata */
@@ -840,7 +841,7 @@ static void *colo_process_incoming_thread(void *opaque)
     bql_unlock();
     if (local_err) {
         error_report_err(local_err);
-        return NULL;
+        goto out_last;
     }
 
     failover_init_state();
@@ -923,7 +924,9 @@ out:
     qemu_sem_wait(&mis->colo_incoming_sem);
     qemu_sem_destroy(&mis->colo_incoming_sem);
 
+out_last:
     rcu_unregister_thread();
+    migration_threads_remove();
     return NULL;
 }
 
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 2339ba400d..555c599c17 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -729,13 +729,15 @@ void *get_dirtyrate_thread(void *arg)
 {
     struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
     int ret;
+
+    migration_threads_add(MIGRATION_THREAD_DIRTY_RATE);
     rcu_register_thread();
 
     ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
                               DIRTY_RATE_STATUS_MEASURING);
     if (ret == -1) {
         error_report("change dirtyrate state failed.");
-        return NULL;
+        goto out;
     }
 
     calculate_dirtyrate(config);
@@ -746,7 +748,10 @@ void *get_dirtyrate_thread(void *arg)
         error_report("change dirtyrate state failed.");
     }
 
+out:
     rcu_unregister_thread();
+    migration_threads_remove();
+
     return NULL;
 }
 
diff --git a/migration/migration.c b/migration/migration.c
index 74b2c1c627..04c4272e46 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2321,6 +2321,7 @@ static void *source_return_path_thread(void *opaque)
     int res;
 
     trace_source_return_path_thread_entry();
+    migration_threads_add(MIGRATION_THREAD_SRC_RETURN);
     rcu_register_thread();
 
     while (migration_is_setup_or_active()) {
@@ -2463,8 +2464,9 @@ out:
         migration_rp_kick(ms);
     }
 
-    trace_source_return_path_thread_end();
     rcu_unregister_thread();
+    migration_threads_remove();
+    trace_source_return_path_thread_end();
 
     return NULL;
 }
@@ -3602,6 +3604,8 @@ static void *bg_migration_thread(void *opaque)
     Error *local_err = NULL;
     int ret;
 
+    migration_threads_add(MIGRATION_THREAD_SNAPSHOT);
+
     rcu_register_thread();
     object_ref(OBJECT(s));
 
@@ -3726,6 +3730,7 @@ fail_setup:
     qemu_fclose(fb);
     object_unref(OBJECT(s));
     rcu_unregister_thread();
+    migration_threads_remove();
 
     return NULL;
 }
diff --git a/migration/multifd.c b/migration/multifd.c
index 2738d78407..b96aaffebb 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -685,6 +685,8 @@ static void *multifd_tls_handshake_thread(void *opaque)
 {
     MultiFDTLSThreadArgs *args = opaque;
 
+    migration_threads_add(MIGRATION_THREAD_SRC_TLS);
+
     qio_channel_tls_handshake(args->tioc,
                               multifd_new_send_channel_async,
                               args->p,
@@ -692,6 +694,8 @@ static void *multifd_tls_handshake_thread(void *opaque)
                               NULL);
     g_free(args);
 
+    migration_threads_remove();
+
     return NULL;
 }
 
@@ -1122,6 +1126,7 @@ static void *multifd_recv_thread(void *opaque)
     int ret;
 
     trace_multifd_recv_thread_start(p->id);
+    migration_threads_add(MIGRATION_THREAD_DST_MULTIFD);
     rcu_register_thread();
 
     while (true) {
@@ -1209,6 +1214,7 @@ static void *multifd_recv_thread(void *opaque)
     }
 
     rcu_unregister_thread();
+    migration_threads_remove();
     trace_multifd_recv_thread_end(p->id, p->packets_recved);
 
     return NULL;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a535fd2e30..ee8f2eac77 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -962,6 +962,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
     RAMBlock *rb = NULL;
 
     trace_postcopy_ram_fault_thread_entry();
+    migration_threads_add(MIGRATION_THREAD_DST_FAULT);
     rcu_register_thread();
     mis->last_rb = NULL; /* last RAMBlock we sent part of */
     qemu_sem_post(&mis->thread_sync_sem);
@@ -1142,9 +1143,12 @@ retry:
             }
         }
     }
+
+    g_free(pfd);
     rcu_unregister_thread();
+    migration_threads_remove();
     trace_postcopy_ram_fault_thread_exit();
-    g_free(pfd);
+
     return NULL;
 }
 
@@ -1713,7 +1717,7 @@ void *postcopy_preempt_thread(void *opaque)
     int ret;
 
     trace_postcopy_preempt_thread_entry();
-
+    migration_threads_add(MIGRATION_THREAD_DST_PREEMPT);
     rcu_register_thread();
 
     qemu_sem_post(&mis->thread_sync_sem);
@@ -1740,7 +1744,7 @@ void *postcopy_preempt_thread(void *opaque)
     qemu_mutex_unlock(&mis->postcopy_prio_thread_mutex);
 
     rcu_unregister_thread();
-
+    migration_threads_remove();
     trace_postcopy_preempt_thread_exit();
 
     return NULL;
diff --git a/migration/savevm.c b/migration/savevm.c
index e796436979..90f87a90c9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2002,14 +2002,15 @@ static void *postcopy_ram_listen_thread(void *opaque)
     int load_res;
     MigrationState *migr = migrate_get_current();
 
-    object_ref(OBJECT(migr));
+    trace_postcopy_ram_listen_thread_start();
+    migration_threads_add(MIGRATION_THREAD_DST_LISTEN);
+    rcu_register_thread();
 
+    object_ref(OBJECT(migr));
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                                    MIGRATION_STATUS_POSTCOPY_ACTIVE);
     qemu_sem_post(&mis->thread_sync_sem);
-    trace_postcopy_ram_listen_thread_start();
 
-    rcu_register_thread();
     /*
      * Because we're a thread and not a coroutine we can't yield
      * in qemu_file, and thus we must be blocking now.
@@ -2078,11 +2079,12 @@ static void *postcopy_ram_listen_thread(void *opaque)
     migration_incoming_state_destroy();
     qemu_loadvm_state_cleanup();
 
-    rcu_unregister_thread();
     mis->have_listen_thread = false;
     postcopy_state_set(POSTCOPY_INCOMING_END);
 
     object_unref(OBJECT(migr));
+    rcu_unregister_thread();
+    migration_threads_remove();
 
     return NULL;
 }
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 25e77404e2..73db26dc82 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -14,6 +14,7 @@
 #include "qemu/queue.h"
 #include "qemu/lockable.h"
 #include "threadinfo.h"
+#include "migration.h"
 
 QemuMutex migration_threads_lock;
 static QLIST_HEAD(, MigrationThread) migration_threads;
-- 
2.45.0

[PATCH 6/7] migration: Remove MigrationThread and threadinfo.h

[Peter Xu]

The MigrationThread struct is mostly the same as MigrationThreadInfoList,
except that it's a double-linked list so removal doesn't need to remember
prev pointer.

That might not be necessary, especially considering that the defintion of
that struct is the only thing in threadinfo.h now.

Reuse MigrationThreadInfoList, then let's drop the header.  Need to manage
a single list entry removal, but after I saw the diff, it's not so bad.

With that, query-migrationthreads is much easier, as we can simply do
QAPI_CLONE() now.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/threadinfo.h | 22 -------------------
 migration/migration.c  |  1 -
 migration/multifd.c    |  1 -
 migration/threadinfo.c | 50 +++++++++++++++++++++---------------------
 4 files changed, 25 insertions(+), 49 deletions(-)
 delete mode 100644 migration/threadinfo.h

diff --git a/migration/threadinfo.h b/migration/threadinfo.h
deleted file mode 100644
index 59f334af21..0000000000
--- a/migration/threadinfo.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- *  Migration Threads info
- *
- *  Copyright (c) 2022 HUAWEI TECHNOLOGIES CO., LTD.
- *
- *  Authors:
- *  Jiang Jiacheng <jiangjiacheng@huawei.com>
- *
- *  This work is licensed under the terms of the GNU GPL, version 2 or later.
- *  See the COPYING file in the top-level directory.
- */
-
-#include "qapi/error.h"
-#include "qapi/qapi-commands-migration.h"
-
-typedef struct MigrationThread MigrationThread;
-
-struct MigrationThread {
-    const char *name; /* the name of migration thread */
-    int thread_id; /* ID of the underlying host thread */
-    QLIST_ENTRY(MigrationThread) node;
-};
diff --git a/migration/migration.c b/migration/migration.c
index 04c4272e46..1034668a90 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -57,7 +57,6 @@
 #include "net/announce.h"
 #include "qemu/queue.h"
 #include "multifd.h"
-#include "threadinfo.h"
 #include "qemu/yank.h"
 #include "sysemu/cpus.h"
 #include "yank_functions.h"
diff --git a/migration/multifd.c b/migration/multifd.c
index b96aaffebb..0baa760ccf 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -26,7 +26,6 @@
 #include "qemu-file.h"
 #include "trace.h"
 #include "multifd.h"
-#include "threadinfo.h"
 #include "options.h"
 #include "qemu/yank.h"
 #include "io/channel-file.h"
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 73db26dc82..6c1278b4ea 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -13,11 +13,13 @@
 #include "qemu/osdep.h"
 #include "qemu/queue.h"
 #include "qemu/lockable.h"
-#include "threadinfo.h"
 #include "migration.h"
+#include "qapi/qapi-commands-migration.h"
+#include "qapi/qapi-visit-migration.h"
+#include "qapi/clone-visitor.h"
 
 QemuMutex migration_threads_lock;
-static QLIST_HEAD(, MigrationThread) migration_threads;
+static MigrationThreadInfoList *migration_threads;
 
 static void __attribute__((constructor)) migration_threads_init(void)
 {
@@ -26,31 +28,40 @@ static void __attribute__((constructor)) migration_threads_init(void)
 
 void migration_threads_add(const char *name)
 {
-    MigrationThread *thread =  g_new0(MigrationThread, 1);
+    MigrationThreadInfo *thread = g_new0(MigrationThreadInfo, 1);
 
-    thread->name = name;
+    thread->name = g_strdup(name);
     thread->thread_id = qemu_get_thread_id();
 
     WITH_QEMU_LOCK_GUARD(&migration_threads_lock) {
-        QLIST_INSERT_HEAD(&migration_threads, thread, node);
+        QAPI_LIST_PREPEND(migration_threads, thread);
     }
 }
 
 void migration_threads_remove(void)
 {
     int tid = qemu_get_thread_id();
-    MigrationThread *thread;
+    MigrationThreadInfoList *thread, *prev;
 
     QEMU_LOCK_GUARD(&migration_threads_lock);
 
-    QLIST_FOREACH(thread, &migration_threads, node) {
-        if (tid != thread->thread_id) {
-            continue;
-        }
+    prev = NULL;
+    thread = migration_threads;
 
-        QLIST_REMOVE(thread, node);
-        g_free(thread);
-        return;
+    while (thread) {
+        if (tid == thread->value->thread_id) {
+            if (!prev) {
+                migration_threads = thread->next;
+            } else {
+                prev->next = thread->next;
+            }
+            /* Terminate this single object to not free the rest */
+            thread->next = NULL;
+            qapi_free_MigrationThreadInfoList(thread);
+            return;
+        }
+        prev = thread;
+        thread = thread->next;
     }
 
     g_assert_not_reached();
@@ -58,18 +69,7 @@ void migration_threads_remove(void)
 
 MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
 {
-    MigrationThreadInfoList *head = NULL;
-    MigrationThreadInfoList **tail = &head;
-    MigrationThread *thread = NULL;
-
     QEMU_LOCK_GUARD(&migration_threads_lock);
-    QLIST_FOREACH(thread, &migration_threads, node) {
-        MigrationThreadInfo *info = g_new0(MigrationThreadInfo, 1);
-        info->name = g_strdup(thread->name);
-        info->thread_id = thread->thread_id;
-
-        QAPI_LIST_APPEND(tail, info);
-    }
 
-    return head;
+    return QAPI_CLONE(MigrationThreadInfoList, migration_threads);
 }
-- 
2.45.0

[PATCH 7/7] hmp: Add "info migrationthreads"

[Peter Xu]

The QMP command was added in 671326201d ("migration: Introduce interface
query-migrationthreads", v8.0).  Add the HMP version of it.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Dr. David Alan Gilbert <dave@treblig.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/monitor/hmp.h          |  1 +
 migration/migration-hmp-cmds.c | 25 +++++++++++++++++++++++++
 hmp-commands-info.hx           | 13 +++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index ae116d9804..e44a399e4a 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -31,6 +31,7 @@ void hmp_info_mice(Monitor *mon, const QDict *qdict);
 void hmp_info_migrate(Monitor *mon, const QDict *qdict);
 void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
 void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
+void hmp_info_migrationthreads(Monitor *mon, const QDict *qdict);
 void hmp_info_cpus(Monitor *mon, const QDict *qdict);
 void hmp_info_vnc(Monitor *mon, const QDict *qdict);
 void hmp_info_spice(Monitor *mon, const QDict *qdict);
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 20d1a6e219..63a6ea61f2 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -814,3 +814,28 @@ void loadvm_completion(ReadLineState *rs, int nb_args, const char *str)
         vm_completion(rs, str);
     }
 }
+
+void hmp_info_migrationthreads(Monitor *mon, const QDict *qdict)
+{
+    MigrationThreadInfoList *list;
+    MigrationThreadInfo *entry;
+    Error *err = NULL;
+
+    list = qmp_query_migrationthreads(&err);
+
+    if (!list) {
+        monitor_printf(mon, "No migration threads found\n");
+        return;
+    }
+
+    monitor_printf(mon, "%-16s%s\n", "TID", "Thread Name");
+    while (list) {
+        entry = list->value;
+        monitor_printf(mon, "%-16" PRId64 "%s\n",
+                       entry->thread_id, entry->name);
+        list = list->next;
+    }
+
+    qapi_free_MigrationThreadInfoList(list);
+    hmp_handle_error(mon, err);
+}
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index c59cd6637b..a8dc55dbd2 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -512,6 +512,19 @@ SRST
     Show current migration parameters.
 ERST
 
+    {
+        .name       = "migrationthreads",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show migration threads information",
+        .cmd        = hmp_info_migrationthreads,
+    },
+
+SRST
+  ``info migrationthreads``
+    Show migration threads information.
+ERST
+
     {
         .name       = "balloon",
         .args_type  = "",
-- 
2.45.0

Re: [PATCH 7/7] hmp: Add "info migrationthreads"

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> The QMP command was added in 671326201d ("migration: Introduce interface
> query-migrationthreads", v8.0).  Add the HMP version of it.
> 
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/monitor/hmp.h          |  1 +
>  migration/migration-hmp-cmds.c | 25 +++++++++++++++++++++++++
>  hmp-commands-info.hx           | 13 +++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index ae116d9804..e44a399e4a 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -31,6 +31,7 @@ void hmp_info_mice(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
> +void hmp_info_migrationthreads(Monitor *mon, const QDict *qdict);
>  void hmp_info_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_vnc(Monitor *mon, const QDict *qdict);
>  void hmp_info_spice(Monitor *mon, const QDict *qdict);
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 20d1a6e219..63a6ea61f2 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -814,3 +814,28 @@ void loadvm_completion(ReadLineState *rs, int nb_args, const char *str)
>          vm_completion(rs, str);
>      }
>  }
> +
> +void hmp_info_migrationthreads(Monitor *mon, const QDict *qdict)
> +{
> +    MigrationThreadInfoList *list;
> +    MigrationThreadInfo *entry;
> +    Error *err = NULL;
> +
> +    list = qmp_query_migrationthreads(&err);
> +

I think you need to do the hmp_handle_error here rather than at
the end, and return if there was one.

Other than that it looks OK.

Dave

> +    if (!list) {
> +        monitor_printf(mon, "No migration threads found\n");
> +        return;
> +    }
> +
> +    monitor_printf(mon, "%-16s%s\n", "TID", "Thread Name");
> +    while (list) {
> +        entry = list->value;
> +        monitor_printf(mon, "%-16" PRId64 "%s\n",
> +                       entry->thread_id, entry->name);
> +        list = list->next;
> +    }
> +
> +    qapi_free_MigrationThreadInfoList(list);
> +    hmp_handle_error(mon, err);
> +}
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index c59cd6637b..a8dc55dbd2 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -512,6 +512,19 @@ SRST
>      Show current migration parameters.
>  ERST
>  
> +    {
> +        .name       = "migrationthreads",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show migration threads information",
> +        .cmd        = hmp_info_migrationthreads,
> +    },
> +
> +SRST
> +  ``info migrationthreads``
> +    Show migration threads information.
> +ERST
> +
>      {
>          .name       = "balloon",
>          .args_type  = "",
> -- 
> 2.45.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[Markus Armbruster]

Command query-migrationthreads went in without a QAPI ACK.  Issues
review should have caught:

* Flawed documentation.  Fixed in commit e6c60bf02d1.

* It should have been spelled query-migration-threads.  Not worth fixing
  now, I guess.

* What are the use cases?  The commit message doesn't tell!  If it's
  just for debugging, the command should be marked unstable.

Re: [PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[Daniel P. Berrangé]

On Tue, Oct 01, 2024 at 07:46:09AM +0200, Markus Armbruster wrote:
> Command query-migrationthreads went in without a QAPI ACK.  Issues
> review should have caught:
> 
> * Flawed documentation.  Fixed in commit e6c60bf02d1.
> 
> * It should have been spelled query-migration-threads.  Not worth fixing
>   now, I guess.
> 
> * What are the use cases?  The commit message doesn't tell!  If it's
>   just for debugging, the command should be marked unstable.

It is hard to use too.

Lets say a mgmt app wants to restrict migration threads to some
certain pCPUs. It can't call query-migrationthreads beforehand
as the threads don't exist until migration is started. If it
calls after migration is started, then there's a window where
threads are running on arbitrary pCPUs that QEMU has access
to. There's no synchronization point where threads have been
created & can be queried, but are not yet sending data (and
thus burning CPU time)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[Peter Xu]

On Tue, Oct 01, 2024 at 03:25:14PM +0100, Daniel P. Berrangé wrote:
> On Tue, Oct 01, 2024 at 07:46:09AM +0200, Markus Armbruster wrote:
> > Command query-migrationthreads went in without a QAPI ACK.  Issues
> > review should have caught:
> > 
> > * Flawed documentation.  Fixed in commit e6c60bf02d1.
> > 
> > * It should have been spelled query-migration-threads.  Not worth fixing
> >   now, I guess.
> > 
> > * What are the use cases?  The commit message doesn't tell!  If it's
> >   just for debugging, the command should be marked unstable.
> 
> It is hard to use too.
> 
> Lets say a mgmt app wants to restrict migration threads to some
> certain pCPUs. It can't call query-migrationthreads beforehand
> as the threads don't exist until migration is started. If it
> calls after migration is started, then there's a window where
> threads are running on arbitrary pCPUs that QEMU has access
> to. There's no synchronization point where threads have been
> created & can be queried, but are not yet sending data (and
> thus burning CPU time)

Indeed, I suppose tricks needed if to work with such model, e.g., mgmt
needs to turn bw=0, start migration, query TIDs, then restore bw.

However that still lacks at least the dest multifd threads, as currently it
only reports src multifd threads TIDs.  I don't see why a serious mgmt
would like to pin and care only src threads, not dest threads, which can
also eat as much (or even more) pCPU resources.

For real debugging purpose, I actually don't see a major value out of it
either, because GDB can provide all information that this API wants to
provide, and only better with thread stacks if we want.

Since I don't see how this can be used right, it didn't get proper QAPI
reviews, and further I highly suspect whether this API is consumed by
anyone at all.. in any serious way.  Shall we remove this API (with/without
going through the deprecation process)?

I added the author Jiacheng too.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Oct 01, 2024 at 03:25:14PM +0100, Daniel P. Berrangé wrote:
>> On Tue, Oct 01, 2024 at 07:46:09AM +0200, Markus Armbruster wrote:
>> > Command query-migrationthreads went in without a QAPI ACK.  Issues
>> > review should have caught:
>> > 
>> > * Flawed documentation.  Fixed in commit e6c60bf02d1.
>> > 
>> > * It should have been spelled query-migration-threads.  Not worth fixing
>> >   now, I guess.
>> > 
>> > * What are the use cases?  The commit message doesn't tell!  If it's
>> >   just for debugging, the command should be marked unstable.
>> 
>> It is hard to use too.
>> 
>> Lets say a mgmt app wants to restrict migration threads to some
>> certain pCPUs. It can't call query-migrationthreads beforehand
>> as the threads don't exist until migration is started. If it
>> calls after migration is started, then there's a window where
>> threads are running on arbitrary pCPUs that QEMU has access
>> to. There's no synchronization point where threads have been
>> created & can be queried, but are not yet sending data (and
>> thus burning CPU time)
>
> Indeed, I suppose tricks needed if to work with such model, e.g., mgmt
> needs to turn bw=0, start migration, query TIDs, then restore bw.
>
> However that still lacks at least the dest multifd threads, as currently it
> only reports src multifd threads TIDs.  I don't see why a serious mgmt
> would like to pin and care only src threads, not dest threads, which can
> also eat as much (or even more) pCPU resources.

Sounds like there's a use case for management applications querying
TIDs, but query-migrationthreads falls short of serving it.

> For real debugging purpose, I actually don't see a major value out of it
> either, because GDB can provide all information that this API wants to
> provide, and only better with thread stacks if we want.

True.

> Since I don't see how this can be used right, it didn't get proper QAPI
> reviews, and further I highly suspect whether this API is consumed by
> anyone at all.. in any serious way.  Shall we remove this API (with/without
> going through the deprecation process)?

If we decide we want to serve the management application use case now,
we should provide a suitable interface, then deprecate
query-migrationthreads.

If we decide not now or not at all, we can deprecate it right away.
Removal without deprecation is also possible, but I doubt breaking our
compatibility promise is justified.

> I added the author Jiacheng too.

Users of query-migrationthreads, please speak up!

Re: [PATCH 0/7] migration: query-migrationthreads enhancements and cleanups

[Peter Xu]

On Wed, Oct 02, 2024 at 07:58:48AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Oct 01, 2024 at 03:25:14PM +0100, Daniel P. Berrangé wrote:
> >> On Tue, Oct 01, 2024 at 07:46:09AM +0200, Markus Armbruster wrote:
> >> > Command query-migrationthreads went in without a QAPI ACK.  Issues
> >> > review should have caught:
> >> > 
> >> > * Flawed documentation.  Fixed in commit e6c60bf02d1.
> >> > 
> >> > * It should have been spelled query-migration-threads.  Not worth fixing
> >> >   now, I guess.
> >> > 
> >> > * What are the use cases?  The commit message doesn't tell!  If it's
> >> >   just for debugging, the command should be marked unstable.
> >> 
> >> It is hard to use too.
> >> 
> >> Lets say a mgmt app wants to restrict migration threads to some
> >> certain pCPUs. It can't call query-migrationthreads beforehand
> >> as the threads don't exist until migration is started. If it
> >> calls after migration is started, then there's a window where
> >> threads are running on arbitrary pCPUs that QEMU has access
> >> to. There's no synchronization point where threads have been
> >> created & can be queried, but are not yet sending data (and
> >> thus burning CPU time)
> >
> > Indeed, I suppose tricks needed if to work with such model, e.g., mgmt
> > needs to turn bw=0, start migration, query TIDs, then restore bw.
> >
> > However that still lacks at least the dest multifd threads, as currently it
> > only reports src multifd threads TIDs.  I don't see why a serious mgmt
> > would like to pin and care only src threads, not dest threads, which can
> > also eat as much (or even more) pCPU resources.
> 
> Sounds like there's a use case for management applications querying
> TIDs, but query-migrationthreads falls short of serving it.
> 
> > For real debugging purpose, I actually don't see a major value out of it
> > either, because GDB can provide all information that this API wants to
> > provide, and only better with thread stacks if we want.
> 
> True.
> 
> > Since I don't see how this can be used right, it didn't get proper QAPI
> > reviews, and further I highly suspect whether this API is consumed by
> > anyone at all.. in any serious way.  Shall we remove this API (with/without
> > going through the deprecation process)?
> 
> If we decide we want to serve the management application use case now,
> we should provide a suitable interface, then deprecate
> query-migrationthreads.
> 
> If we decide not now or not at all, we can deprecate it right away.
> Removal without deprecation is also possible, but I doubt breaking our
> compatibility promise is justified.
> 
> > I added the author Jiacheng too.
> 
> Users of query-migrationthreads, please speak up!

I'll go ahead and remove it.  The current plan is I'll skip the deprecation
procedure for this one because I don't expect anyone would read the
deprecation notification at all... aka, no real user I can ever think of,
who only cares about source pinning not dest.

I'll pick patch 2 out and send separately, which is still a cleanup without
the query-migrationthreads API.

We can keep the discussion going in the new patch.

Thanks,

-- 
Peter Xu