[RFC PATCH 0/3] migration: support query migration thread information

[RFC PATCH 0/3] migration: support query migration thread information

[Jiang Jiacheng via]

We need get more migration thread information to support
migration pin, especially thread name and its pid. Add an
qmp interface to query migration thread information by its
name and report thread name when migration thread is started.


Jiang Jiacheng (3):
  migration: report migration thread name to libvirt
  migration: implement query migration threadinfo by name
  migration: save/delete migration thread info

 migration/meson.build  |  1 +
 migration/migration.c  |  7 +++++
 migration/multifd.c    |  9 +++++-
 migration/threadinfo.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 migration/threadinfo.h | 30 ++++++++++++++++++
 qapi/migration.json    | 42 +++++++++++++++++++++++++
 6 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 migration/threadinfo.c
 create mode 100644 migration/threadinfo.h

-- 
2.33.0

[PATCH 1/3] migration: report migration thread name to libvirt

[Jiang Jiacheng via]

Report migration thread name to libvirt in order to
support query migration thread infomation by its name.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
---
 migration/migration.c |  3 +++
 migration/multifd.c   |  5 ++++-
 qapi/migration.json   | 12 ++++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 52b5d39244..b4ce458bb9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3955,6 +3955,9 @@ static void *migration_thread(void *opaque)
     MigThrError thr_error;
     bool urgent = false;
 
+    /* report migration thread name to libvirt */
+    qapi_event_send_migration_name("live_migration");
+
     rcu_register_thread();
 
     object_ref(OBJECT(s));
diff --git a/migration/multifd.c b/migration/multifd.c
index 000ca4d4ec..6e834c7111 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -17,6 +17,7 @@
 #include "exec/ramblock.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-migration.h"
 #include "ram.h"
 #include "migration.h"
 #include "socket.h"
@@ -24,7 +25,6 @@
 #include "qemu-file.h"
 #include "trace.h"
 #include "multifd.h"
-
 #include "qemu/yank.h"
 #include "io/channel-socket.h"
 #include "yank_functions.h"
@@ -650,6 +650,9 @@ static void *multifd_send_thread(void *opaque)
     int ret = 0;
     bool use_zero_copy_send = migrate_use_zero_copy_send();
 
+    /* report multifd thread name to libvirt */
+    qapi_event_send_migration_name(p->name);
+
     trace_multifd_send_thread_start(p->id);
     rcu_register_thread();
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 88ecf86ac8..b0cf366ac0 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1286,6 +1286,18 @@
 { 'event': 'MIGRATION_PASS',
   'data': { 'pass': 'int' } }
 
+##
+# @MIGRATION_NAME:
+#
+# Emitted when migration thread appear
+#
+# @name: name of migration thread
+#
+# Since: 7.2
+##
+{ 'event': 'MIGRATION_NAME',
+  'data': { 'name': 'str' } }
+
 ##
 # @COLOMessage:
 #
-- 
2.33.0

[PATCH 2/3] migration: implement query migration threadinfo by name

[Jiang Jiacheng via]

Introduce interface query-migrationthreads. The interface is use
with the migration thread name reported by qemu and returns with
migration thread name and its pid.
Introduce threadinfo.c to manage threads with migration.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
---
 migration/meson.build  |  1 +
 migration/threadinfo.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 migration/threadinfo.h | 30 ++++++++++++++++++
 qapi/migration.json    | 30 ++++++++++++++++++
 4 files changed, 131 insertions(+)
 create mode 100644 migration/threadinfo.c
 create mode 100644 migration/threadinfo.h

diff --git a/migration/meson.build b/migration/meson.build
index 690487cf1a..ed7d27f11a 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -25,6 +25,7 @@ softmmu_ss.add(files(
   'savevm.c',
   'socket.c',
   'tls.c',
+  'threadinfo.c',
 ), gnutls)
 
 softmmu_ss.add(when: rdma, if_true: files('rdma.c'))
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
new file mode 100644
index 0000000000..0df668d427
--- /dev/null
+++ b/migration/threadinfo.c
@@ -0,0 +1,70 @@
+/*
+ *  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 "threadinfo.h"
+
+static QLIST_HEAD(, MigrationThread) migration_threads;
+
+MigrationThread* MigrationThreadAdd(const char *name, int thread_id)
+{
+    MigrationThread *thread = NULL;
+
+    thread = g_new0(MigrationThread, 1);
+    thread->name = (char*)name;
+    thread->thread_id = thread_id;
+
+    QLIST_INSERT_HEAD(&migration_threads, thread, node);
+
+    return thread;
+}
+
+void MigrationThreadDel(MigrationThread* thread)
+{
+    if (thread) {
+        QLIST_REMOVE(thread, node);
+	    g_free(thread);
+    }
+}
+
+MigrationThread* MigrationThreadQuery(const char* name)
+{
+    MigrationThread *thread = NULL;
+
+    QLIST_FOREACH(thread, &migration_threads, node) {
+        if (!strcmp(thread->name, name)) {
+            return thread;
+        }
+    }
+
+    return NULL;
+}
+
+MigrationThreadInfo* qmp_query_migrationthreads(const char* name, Error **errp)
+{
+    MigrationThread *thread = NULL;
+    MigrationThreadInfo *info = NULL;
+
+    thread = MigrationThreadQuery(name);
+    if (!thread) {
+        goto err;
+    }
+
+    info = g_new0(MigrationThreadInfo, 1);
+    info->name = g_strdup(thread->name);
+    info->thread_id = thread->thread_id;
+
+    return info;
+
+err:
+    error_setg(errp, "thread '%s' doesn't exist", name);
+    return NULL;
+}
diff --git a/migration/threadinfo.h b/migration/threadinfo.h
new file mode 100644
index 0000000000..f62a6ff261
--- /dev/null
+++ b/migration/threadinfo.h
@@ -0,0 +1,30 @@
+/*
+ *  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 "qemu/queue.h"
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+
+typedef struct MigrationThread MigrationThread;
+
+struct MigrationThread {
+    char *name; /* the name of migration thread */
+    int thread_id; /* ID of the underlying host thread */
+    QLIST_ENTRY(MigrationThread) node;
+};
+
+MigrationThread *MigrationThreadAdd(const char *name, int thread_id);
+
+void MigrationThreadDel(MigrationThread* info);
+
+MigrationThread* MigrationThreadQuery(const char* name);
diff --git a/qapi/migration.json b/qapi/migration.json
index b0cf366ac0..e93c3e560a 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1970,6 +1970,36 @@
 { 'command': 'query-vcpu-dirty-limit',
   'returns': [ 'DirtyLimitInfo' ] }
 
+##
+# @MigrationThreadInfo:
+#
+# Information about migrationthreads
+#
+# @name: the name of migration thread
+#
+# @thread-id: ID of the underlying host thread
+#
+# Since: 7.2
+##
+{ 'struct': 'MigrationThreadInfo',
+  'data': {'name': 'str',
+           'thread-id': 'int'} }
+
+##
+# @query-migrationthreads:
+#
+# Returns threadInfo about the thread specified by name
+#
+# data: migration thread name
+#
+# returns: information about the specified thread
+#
+# Since: 7.2
+##
+{ 'command': 'query-migrationthreads',
+  'data': { 'name': 'str' },
+  'returns': 'MigrationThreadInfo' }
+
 ##
 # @snapshot-save:
 #
-- 
2.33.0

[PATCH 3/3] migration: save/delete migration thread info

[Jiang Jiacheng via]

To support query migration thread infomation, save and delete
thread information at thread creation and end.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
---
 migration/migration.c | 4 ++++
 migration/multifd.c   | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index b4ce458bb9..957205e693 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -57,6 +57,7 @@
 #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"
@@ -3951,10 +3952,12 @@ 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;
 
+    thread = MigrationThreadAdd("live_migration", qemu_get_thread_id());
     /* report migration thread name to libvirt */
     qapi_event_send_migration_name("live_migration");
 
@@ -4034,6 +4037,7 @@ static void *migration_thread(void *opaque)
     migration_iteration_finish(s);
     object_unref(OBJECT(s));
     rcu_unregister_thread();
+    MigrationThreadDel(thread);
     return NULL;
 }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index 6e834c7111..fca06284de 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -25,6 +25,7 @@
 #include "qemu-file.h"
 #include "trace.h"
 #include "multifd.h"
+#include "threadinfo.h"
 #include "qemu/yank.h"
 #include "io/channel-socket.h"
 #include "yank_functions.h"
@@ -646,10 +647,12 @@ int multifd_send_sync_main(QEMUFile *f)
 static void *multifd_send_thread(void *opaque)
 {
     MultiFDSendParams *p = opaque;
+    MigrationThread *thread = NULL;
     Error *local_err = NULL;
     int ret = 0;
     bool use_zero_copy_send = migrate_use_zero_copy_send();
 
+    thread = MigrationThreadAdd(p->name, qemu_get_thread_id());
     /* report multifd thread name to libvirt */
     qapi_event_send_migration_name(p->name);
 
@@ -762,6 +765,7 @@ out:
     qemu_mutex_unlock(&p->mutex);
 
     rcu_unregister_thread();
+    MigrationThreadDel(thread);
     trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
 
     return NULL;
-- 
2.33.0

Re: [PATCH 1/3] migration: report migration thread name to libvirt

[Juan Quintela]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> Report migration thread name to libvirt in order to
> support query migration thread infomation by its name.
>
> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
> ---
>  migration/migration.c |  3 +++
>  migration/multifd.c   |  5 ++++-
>  qapi/migration.json   | 12 ++++++++++++
>  3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 52b5d39244..b4ce458bb9 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3955,6 +3955,9 @@ static void *migration_thread(void *opaque)
>      MigThrError thr_error;
>      bool urgent = false;
>  
> +    /* report migration thread name to libvirt */
> +    qapi_event_send_migration_name("live_migration");
> +
>      rcu_register_thread();
>  
>      object_ref(OBJECT(s));

I am not sure about this.
This is not an event, in my point of view.

What is the problem adding it to info migrate or similar?
Looks more logical to me.

Later, Juan.

Re: [PATCH 2/3] migration: implement query migration threadinfo by name

[Juan Quintela]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> Introduce interface query-migrationthreads. The interface is use
> with the migration thread name reported by qemu and returns with
> migration thread name and its pid.
> Introduce threadinfo.c to manage threads with migration.
>
> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>

I like this query interface better than the way you expose the thread
name in the 1st place.

But once that we are here, why don't we just "tweak" abit the interface:

> diff --git a/qapi/migration.json b/qapi/migration.json
> index b0cf366ac0..e93c3e560a 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1970,6 +1970,36 @@
>  { 'command': 'query-vcpu-dirty-limit',
>    'returns': [ 'DirtyLimitInfo' ] }
>  
> +##
> +# @MigrationThreadInfo:
> +#
> +# Information about migrationthreads
> +#
> +# @name: the name of migration thread
> +#
> +# @thread-id: ID of the underlying host thread
> +#
> +# Since: 7.2
> +##
> +{ 'struct': 'MigrationThreadInfo',
> +  'data': {'name': 'str',
> +           'thread-id': 'int'} }

1st, it is an int enough for all architectures?  I know that for linux
and friends it is, but not sure about windows and other weird systems.

> +##
> +# @query-migrationthreads:

What about renaming this to:

@query-migrationthread (I removed the last 's')

because it returns the info of a single thread.

> +#
> +# Returns threadInfo about the thread specified by name
> +#
> +# data: migration thread name
> +#
> +# returns: information about the specified thread
> +#
> +# Since: 7.2
> +##
> +{ 'command': 'query-migrationthreads',
> +  'data': { 'name': 'str' },
> +  'returns': 'MigrationThreadInfo' }
> +
>  ##
>  # @snapshot-save:
>  #

And leaves the @query-migrationthreads name for something in the spirit of

> +{ 'command': 'query-migrationthreads',
> +  'returns': ['str'] }

That returns all the migration threads names.

Or perhaps even

> +{ 'command': 'query-migrationthreads',
> +  'returns': ['MigrationThreadInfo'] }

And call it a day?

Later, Juan.

Re: [PATCH 3/3] migration: save/delete migration thread info

[Juan Quintela]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> To support query migration thread infomation, save and delete
> thread information at thread creation and end.
>
> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>

Don't disagree with this, but if we create this on the sending side, why
this is not needed for the multifd_recv_threads?

Later, Juan.

> ---
>  migration/migration.c | 4 ++++
>  migration/multifd.c   | 4 ++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b4ce458bb9..957205e693 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -57,6 +57,7 @@
>  #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"
> @@ -3951,10 +3952,12 @@ 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;
>  
> +    thread = MigrationThreadAdd("live_migration", qemu_get_thread_id());
>      /* report migration thread name to libvirt */
>      qapi_event_send_migration_name("live_migration");
>  
> @@ -4034,6 +4037,7 @@ static void *migration_thread(void *opaque)
>      migration_iteration_finish(s);
>      object_unref(OBJECT(s));
>      rcu_unregister_thread();
> +    MigrationThreadDel(thread);
>      return NULL;
>  }


>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 6e834c7111..fca06284de 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -25,6 +25,7 @@
>  #include "qemu-file.h"
>  #include "trace.h"
>  #include "multifd.h"
> +#include "threadinfo.h"
>  #include "qemu/yank.h"
>  #include "io/channel-socket.h"
>  #include "yank_functions.h"
> @@ -646,10 +647,12 @@ int multifd_send_sync_main(QEMUFile *f)
>  static void *multifd_send_thread(void *opaque)
>  {
>      MultiFDSendParams *p = opaque;
> +    MigrationThread *thread = NULL;
>      Error *local_err = NULL;
>      int ret = 0;
>      bool use_zero_copy_send = migrate_use_zero_copy_send();
>  
> +    thread = MigrationThreadAdd(p->name, qemu_get_thread_id());
>      /* report multifd thread name to libvirt */
>      qapi_event_send_migration_name(p->name);
>  
> @@ -762,6 +765,7 @@ out:
>      qemu_mutex_unlock(&p->mutex);
>  
>      rcu_unregister_thread();
> +    MigrationThreadDel(thread);
>      trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
>  
>      return NULL;

Re: [PATCH 1/3] migration: report migration thread name to libvirt

[Jiang Jiacheng via]

On 2023/1/30 12:19, Juan Quintela wrote:
> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>> Report migration thread name to libvirt in order to
>> support query migration thread infomation by its name.
>>
>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>> ---
>>  migration/migration.c |  3 +++
>>  migration/multifd.c   |  5 ++++-
>>  qapi/migration.json   | 12 ++++++++++++
>>  3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 52b5d39244..b4ce458bb9 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3955,6 +3955,9 @@ static void *migration_thread(void *opaque)
>>      MigThrError thr_error;
>>      bool urgent = false;
>>  
>> +    /* report migration thread name to libvirt */
>> +    qapi_event_send_migration_name("live_migration");
>> +
>>      rcu_register_thread();
>>  
>>      object_ref(OBJECT(s));
> 
> I am not sure about this.
> This is not an event, in my point of view.
> 
> What is the problem adding it to info migrate or similar?
> Looks more logical to me.

I want to implement an interface for libvirt to proactively query
information about migration threads. And this event is used to notify
libvirt that the thread has been created and to provide the thread name
for subsequent operations of querying specified thread's information.

Thanks, Jiang Jiacheng

> 
> Later, Juan.
> 

Re: [PATCH 2/3] migration: implement query migration threadinfo by name

[Jiang Jiacheng via]

On 2023/1/30 12:27, Juan Quintela wrote:
> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>> Introduce interface query-migrationthreads. The interface is use
>> with the migration thread name reported by qemu and returns with
>> migration thread name and its pid.
>> Introduce threadinfo.c to manage threads with migration.
>>
>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
> 
> I like this query interface better than the way you expose the thread
> name in the 1st place.

The event in patch1 is used to pass the thread name since libvirt
doesn't know the name of the migration thread but the interface below
need its name.
I think the event can be dropped if we store the thread name in
libvirt(if the migration thread name is fixed in qemu) or using the
'query-migrationthreads' you mentioned below.

> 
> But once that we are here, why don't we just "tweak" abit the interface:
> 
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index b0cf366ac0..e93c3e560a 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1970,6 +1970,36 @@
>>  { 'command': 'query-vcpu-dirty-limit',
>>    'returns': [ 'DirtyLimitInfo' ] }
>>  
>> +##
>> +# @MigrationThreadInfo:
>> +#
>> +# Information about migrationthreads
>> +#
>> +# @name: the name of migration thread
>> +#
>> +# @thread-id: ID of the underlying host thread
>> +#
>> +# Since: 7.2
>> +##
>> +{ 'struct': 'MigrationThreadInfo',
>> +  'data': {'name': 'str',
>> +           'thread-id': 'int'} }
> 
> 1st, it is an int enough for all architectures?  I know that for linux
> and friends it is, but not sure about windows and other weird systems.
> 

It is only enough for migration pin which I want to implement. But I
think this struct can be easily expand if we need other information
about migration thread.

>> +##
>> +# @query-migrationthreads:
> 
> What about renaming this to:
> 
> @query-migrationthread (I removed the last 's')
> 
> because it returns the info of a single thread.
> 
>> +#
>> +# Returns threadInfo about the thread specified by name
>> +#
>> +# data: migration thread name
>> +#
>> +# returns: information about the specified thread
>> +#
>> +# Since: 7.2
>> +##
>> +{ 'command': 'query-migrationthreads',
>> +  'data': { 'name': 'str' },
>> +  'returns': 'MigrationThreadInfo' }
>> +
>>  ##
>>  # @snapshot-save:
>>  #
> 
> And leaves the @query-migrationthreads name for something in the spirit of
> 
>> +{ 'command': 'query-migrationthreads',
>> +  'returns': ['str'] }
> 
> That returns all the migration threads names.
> 
> Or perhaps even
> 
>> +{ 'command': 'query-migrationthreads',
>> +  'returns': ['MigrationThreadInfo'] }
> 
> And call it a day?

I think it's the best. If in this way, should we keep the interface to
query specified thread?

Thanks, Jiang Jiacheng

> 
> Later, Juan.
> 

Re: [PATCH 3/3] migration: save/delete migration thread info

[Jiang Jiacheng via]

On 2023/1/30 12:28, Juan Quintela wrote:
> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>> To support query migration thread infomation, save and delete
>> thread information at thread creation and end.
>>
>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
> 
> Don't disagree with this, but if we create this on the sending side, why
> this is not needed for the multifd_recv_threads?
> 

I only add several threads which i'm interested in into the list till
now, whose information will be used for setting cpu affinity for
migration thread.
For consistency, we can add other threads to the list, but those
information won't be used so far.

Thanks, Jiang Jiacheng

> Later, Juan.
> 
>> ---
>>  migration/migration.c | 4 ++++
>>  migration/multifd.c   | 4 ++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b4ce458bb9..957205e693 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -57,6 +57,7 @@
>>  #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"
>> @@ -3951,10 +3952,12 @@ 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;
>>  
>> +    thread = MigrationThreadAdd("live_migration", qemu_get_thread_id());
>>      /* report migration thread name to libvirt */
>>      qapi_event_send_migration_name("live_migration");
>>  
>> @@ -4034,6 +4037,7 @@ static void *migration_thread(void *opaque)
>>      migration_iteration_finish(s);
>>      object_unref(OBJECT(s));
>>      rcu_unregister_thread();
>> +    MigrationThreadDel(thread);
>>      return NULL;
>>  }
> 
> 
>>  
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 6e834c7111..fca06284de 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -25,6 +25,7 @@
>>  #include "qemu-file.h"
>>  #include "trace.h"
>>  #include "multifd.h"
>> +#include "threadinfo.h"
>>  #include "qemu/yank.h"
>>  #include "io/channel-socket.h"
>>  #include "yank_functions.h"
>> @@ -646,10 +647,12 @@ int multifd_send_sync_main(QEMUFile *f)
>>  static void *multifd_send_thread(void *opaque)
>>  {
>>      MultiFDSendParams *p = opaque;
>> +    MigrationThread *thread = NULL;
>>      Error *local_err = NULL;
>>      int ret = 0;
>>      bool use_zero_copy_send = migrate_use_zero_copy_send();
>>  
>> +    thread = MigrationThreadAdd(p->name, qemu_get_thread_id());
>>      /* report multifd thread name to libvirt */
>>      qapi_event_send_migration_name(p->name);
>>  
>> @@ -762,6 +765,7 @@ out:
>>      qemu_mutex_unlock(&p->mutex);
>>  
>>      rcu_unregister_thread();
>> +    MigrationThreadDel(thread);
>>      trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
>>  
>>      return NULL;
> 

Re: [PATCH 2/3] migration: implement query migration threadinfo by name

[Juan Quintela]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> On 2023/1/30 12:27, Juan Quintela wrote:
>> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>>> Introduce interface query-migrationthreads. The interface is use
>>> with the migration thread name reported by qemu and returns with
>>> migration thread name and its pid.
>>> Introduce threadinfo.c to manage threads with migration.
>>>
>>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>> 
>> I like this query interface better than the way you expose the thread
>> name in the 1st place.
>
> The event in patch1 is used to pass the thread name since libvirt
> doesn't know the name of the migration thread but the interface below
> need its name.
> I think the event can be dropped if we store the thread name in
> libvirt(if the migration thread name is fixed in qemu) or using the
> 'query-migrationthreads' you mentioned below.

I preffer the query migrationthreads, thanks.
>
>> 
>> But once that we are here, why don't we just "tweak" abit the interface:
>> 
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index b0cf366ac0..e93c3e560a 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1970,6 +1970,36 @@
>>>  { 'command': 'query-vcpu-dirty-limit',
>>>    'returns': [ 'DirtyLimitInfo' ] }
>>>  
>>> +##
>>> +# @MigrationThreadInfo:
>>> +#
>>> +# Information about migrationthreads
>>> +#
>>> +# @name: the name of migration thread
>>> +#
>>> +# @thread-id: ID of the underlying host thread
>>> +#
>>> +# Since: 7.2
>>> +##
>>> +{ 'struct': 'MigrationThreadInfo',
>>> +  'data': {'name': 'str',
>>> +           'thread-id': 'int'} }
>> 
>> 1st, it is an int enough for all architectures?  I know that for linux
>> and friends it is, but not sure about windows and other weird systems.
>> 
>
> It is only enough for migration pin which I want to implement. But I
> think this struct can be easily expand if we need other information
> about migration thread.

I mean that pthread_t (what you are passing here) is an int on linux.
Not sure on other OS's.

>>> +##
>>> +# @query-migrationthreads:
>> 
>> What about renaming this to:
>> 
>> @query-migrationthread (I removed the last 's')
>> 
>> because it returns the info of a single thread.
>> 
>>> +#
>>> +# Returns threadInfo about the thread specified by name
>>> +#
>>> +# data: migration thread name
>>> +#
>>> +# returns: information about the specified thread
>>> +#
>>> +# Since: 7.2
>>> +##
>>> +{ 'command': 'query-migrationthreads',
>>> +  'data': { 'name': 'str' },
>>> +  'returns': 'MigrationThreadInfo' }
>>> +
>>>  ##
>>>  # @snapshot-save:
>>>  #
>> 
>> And leaves the @query-migrationthreads name for something in the spirit of
>> 
>>> +{ 'command': 'query-migrationthreads',
>>> +  'returns': ['str'] }
>> 
>> That returns all the migration threads names.
>> 
>> Or perhaps even
>> 
>>> +{ 'command': 'query-migrationthreads',
>>> +  'returns': ['MigrationThreadInfo'] }
>> 
>> And call it a day?
>
> I think it's the best. If in this way, should we keep the interface to
> query specified thread?

I wouldn't do it, but it is up to you.

My (little) understanding of what you want to do is that you want all
the threads, so I see no reason to have a query for a single one.

Later, Juan.

Re: [PATCH 3/3] migration: save/delete migration thread info

[Juan Quintela]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> On 2023/1/30 12:28, Juan Quintela wrote:
>> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>>> To support query migration thread infomation, save and delete
>>> thread information at thread creation and end.
>>>
>>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>> 
>> Don't disagree with this, but if we create this on the sending side, why
>> this is not needed for the multifd_recv_threads?
>> 
>
> I only add several threads which i'm interested in into the list till
> now, whose information will be used for setting cpu affinity for
> migration thread.
> For consistency, we can add other threads to the list, but those
> information won't be used so far.

It is just curiosity, why do you want to set cpu affinity on the source
but not on destination?

Later, Juan.

Re: [PATCH 2/3] migration: implement query migration threadinfo by name

[Jiang Jiacheng via]

On 2023/1/30 22:03, Juan Quintela wrote:
> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>> On 2023/1/30 12:27, Juan Quintela wrote:
>>> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>>>> Introduce interface query-migrationthreads. The interface is use
>>>> with the migration thread name reported by qemu and returns with
>>>> migration thread name and its pid.
>>>> Introduce threadinfo.c to manage threads with migration.
>>>>
>>>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>>>
>>> I like this query interface better than the way you expose the thread
>>> name in the 1st place.
>>
>> The event in patch1 is used to pass the thread name since libvirt
>> doesn't know the name of the migration thread but the interface below
>> need its name.
>> I think the event can be dropped if we store the thread name in
>> libvirt(if the migration thread name is fixed in qemu) or using the
>> 'query-migrationthreads' you mentioned below.
> 
> I preffer the query migrationthreads, thanks.
>>
>>>
>>> But once that we are here, why don't we just "tweak" abit the interface:
>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index b0cf366ac0..e93c3e560a 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1970,6 +1970,36 @@
>>>>  { 'command': 'query-vcpu-dirty-limit',
>>>>    'returns': [ 'DirtyLimitInfo' ] }
>>>>  
>>>> +##
>>>> +# @MigrationThreadInfo:
>>>> +#
>>>> +# Information about migrationthreads
>>>> +#
>>>> +# @name: the name of migration thread
>>>> +#
>>>> +# @thread-id: ID of the underlying host thread
>>>> +#
>>>> +# Since: 7.2
>>>> +##
>>>> +{ 'struct': 'MigrationThreadInfo',
>>>> +  'data': {'name': 'str',
>>>> +           'thread-id': 'int'} }
>>>
>>> 1st, it is an int enough for all architectures?  I know that for linux
>>> and friends it is, but not sure about windows and other weird systems.
>>>
>>
>> It is only enough for migration pin which I want to implement. But I
>> think this struct can be easily expand if we need other information
>> about migration thread.
> 
> I mean that pthread_t (what you are passing here) is an int on linux.
> Not sure on other OS's.
> 

I'm sorry about my misunderstanding. I use 'int' for thread-id just like
cpu or iothread's thread-id, and it is get from 'qemu_get_thread_id'. So
I think it is enough.

>>>> +##
>>>> +# @query-migrationthreads:
>>>
>>> What about renaming this to:
>>>
>>> @query-migrationthread (I removed the last 's')
>>>
>>> because it returns the info of a single thread.
>>>
>>>> +#
>>>> +# Returns threadInfo about the thread specified by name
>>>> +#
>>>> +# data: migration thread name
>>>> +#
>>>> +# returns: information about the specified thread
>>>> +#
>>>> +# Since: 7.2
>>>> +##
>>>> +{ 'command': 'query-migrationthreads',
>>>> +  'data': { 'name': 'str' },
>>>> +  'returns': 'MigrationThreadInfo' }
>>>> +
>>>>  ##
>>>>  # @snapshot-save:
>>>>  #
>>>
>>> And leaves the @query-migrationthreads name for something in the spirit of
>>>
>>>> +{ 'command': 'query-migrationthreads',
>>>> +  'returns': ['str'] }
>>>
>>> That returns all the migration threads names.
>>>
>>> Or perhaps even
>>>
>>>> +{ 'command': 'query-migrationthreads',
>>>> +  'returns': ['MigrationThreadInfo'] }
>>>
>>> And call it a day?
>>
>> I think it's the best. If in this way, should we keep the interface to
>> query specified thread?
> 
> I wouldn't do it, but it is up to you.
> 
> My (little) understanding of what you want to do is that you want all
> the threads, so I see no reason to have a query for a single one.
> 

Thanks for your suggestion. As you said, I need all the threads info,
and the specified thread info can be got easily from the new interface.
The old interface seems to be redundant indeed.

Thanks, Jiang Jiacheng

> Later, Juan.
> 

Re: [PATCH 3/3] migration: save/delete migration thread info

[Jiang Jiacheng via]

On 2023/1/30 22:04, Juan Quintela wrote:
> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>> On 2023/1/30 12:28, Juan Quintela wrote:
>>> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>>>> To support query migration thread infomation, save and delete
>>>> thread information at thread creation and end.
>>>>
>>>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>>>
>>> Don't disagree with this, but if we create this on the sending side, why
>>> this is not needed for the multifd_recv_threads?
>>>
>>
>> I only add several threads which i'm interested in into the list till
>> now, whose information will be used for setting cpu affinity for
>> migration thread.
>> For consistency, we can add other threads to the list, but those
>> information won't be used so far.
> 
> It is just curiosity, why do you want to set cpu affinity on the source
> but not on destination?
> 

Mainly considering the application scenarios. To improve the migration
performance, I want to set cpu affinity for migration thread when
migration a VM whose service threads preempt most of the CPU resources.
So the source side can benefit more from cpu affinity.
And on the destination side, the pressure is lighter (generally),
setting cpu affinity may not be so much useful as source side.

Thanks, Jiang Jiacheng

> Later, Juan.
> 
> 

Re: [PATCH 2/3] migration: implement query migration threadinfo by name

[Juan Quintela]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> On 2023/1/30 22:03, Juan Quintela wrote:
>> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>>> On 2023/1/30 12:27, Juan Quintela wrote:

>>>>
>>>> 1st, it is an int enough for all architectures?  I know that for linux
>>>> and friends it is, but not sure about windows and other weird systems.
>>>>
>>>
>>> It is only enough for migration pin which I want to implement. But I
>>> think this struct can be easily expand if we need other information
>>> about migration thread.
>> 
>> I mean that pthread_t (what you are passing here) is an int on linux.
>> Not sure on other OS's.
>> 
>
> I'm sorry about my misunderstanding. I use 'int' for thread-id just like
> cpu or iothread's thread-id, and it is get from 'qemu_get_thread_id'. So
> I think it is enough.

Ok, fine enough.  Thanks.

Later, Juan.