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
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
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.
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 :|
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
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!
© 2016 - 2024 Red Hat, Inc.