Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Tue, Jan 20, 2026 at 03:54:19PM -0300, Fabiano Rosas wrote: > Peter Xu writes: > > > On Fri, Jan 16, 2026 at 06:48:21PM -0300, Fabiano Rosas wrote: > >> Another one for the pile: > >> > >> #5 0x7f0beda4fb32 in __assert_fail (assertion=0x55b8c3ed8cb8 "mode >= > >> 0 ...) at assert.c:103 > >> #6 0x55b8c3a1d7a9 in migrate_mode () at ../migration/options.c:882 > >> #7 0x55b8c3a1084e in fill_source_migration_info (info=0x55b8f1291650) > >> at ../migration/migration.c:1322 > >> #8 0x55b8c3a10cae in qmp_query_migrate (errp=0x7fff5742ae80) at > >> ../migration/migration.c:1438 > >> #9 0x55b8c3d4bc2a in qmp_marshal_query_migrate (args=0x55b8f11f2280, > >> ret=0x7f0becd25da8, errp=0x7f0becd25da0) at > >> qapi/qapi-commands-migration.c:48 > >> #10 0x55b8c3d9a13b in do_qmp_dispatch_bh (opaque=0x7f0becd25e40) at > >> ../qapi/qmp-dispatch.c:128 > >> #11 0x55b8c3dc6366 in aio_bh_call (bh=0x55b8f12922d0) at > >> ../util/async.c:173 > >> #12 0x55b8c3dc6482 in aio_bh_poll (ctx=0x55b8f10741e0) at > >> ../util/async.c:220 > >> #13 0x55b8c3da9832 in aio_poll (ctx=0x55b8f10741e0, blocking=false) at > >> ../util/aio-posix.c:719 > >> #14 0x55b8c3cea017 in monitor_cleanup () at ../monitor/monitor.c:676 > >> #15 0x55b8c39ef646 in qemu_cleanup (status=0) at > >> ../system/runstate.c:999 > >> #16 0x55b8c3cec38e in qemu_default_main (opaque=0x0) at > >> ../system/main.c:51 > >> #17 0x55b8c3cec430 in main (argc=33, argv=0x7fff5742b208) at > >> #../system/main.c:93 > >> > >> (gdb) p/x mode > >> $8 = 0x > > > > What's the reproducer? Is it easy to reproduce? > > > > Just hammering some make check instances in parallel, as usual. > > > I wonder if current_migration released already, or if monitor should still > > process any QMP handler if the VM is shutting down.. > > > > monitor_cleanup says it will continue dispatching and just never send a > response to the command. It seems migration will need it's own way of > tracking this. > > Maybe as a first step we could clear current_migration at > migration_instance_finalize(). > (I see device-introspect-test doesn't like it. I'll look into it) > > > Is this only happening after this series applied? I can't yet see how the > > threadify affected it.. > > No, sorry, I posted here because I think this affects this series' > assumptions about the BQL. Aha, you reminded me on this work, remember to check it out (I forgot most of it.. as usual..): https://lore.kernel.org/qemu-devel/[email protected] I also recall besides the singleton idea, we can also take a mutex and release current_migration at finalize(), then fetch current_migration will instead need to take a refcount and also with mutex held. Anyway, thanks for looking into it. Now, it's all yours. :) -- Peter Xu
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
Peter Xu writes: > On Fri, Jan 16, 2026 at 06:48:21PM -0300, Fabiano Rosas wrote: >> Another one for the pile: >> >> #5 0x7f0beda4fb32 in __assert_fail (assertion=0x55b8c3ed8cb8 "mode >= 0 >> ...) at assert.c:103 >> #6 0x55b8c3a1d7a9 in migrate_mode () at ../migration/options.c:882 >> #7 0x55b8c3a1084e in fill_source_migration_info (info=0x55b8f1291650) >> at ../migration/migration.c:1322 >> #8 0x55b8c3a10cae in qmp_query_migrate (errp=0x7fff5742ae80) at >> ../migration/migration.c:1438 >> #9 0x55b8c3d4bc2a in qmp_marshal_query_migrate (args=0x55b8f11f2280, >> ret=0x7f0becd25da8, errp=0x7f0becd25da0) at qapi/qapi-commands-migration.c:48 >> #10 0x55b8c3d9a13b in do_qmp_dispatch_bh (opaque=0x7f0becd25e40) at >> ../qapi/qmp-dispatch.c:128 >> #11 0x55b8c3dc6366 in aio_bh_call (bh=0x55b8f12922d0) at >> ../util/async.c:173 >> #12 0x55b8c3dc6482 in aio_bh_poll (ctx=0x55b8f10741e0) at >> ../util/async.c:220 >> #13 0x55b8c3da9832 in aio_poll (ctx=0x55b8f10741e0, blocking=false) at >> ../util/aio-posix.c:719 >> #14 0x55b8c3cea017 in monitor_cleanup () at ../monitor/monitor.c:676 >> #15 0x55b8c39ef646 in qemu_cleanup (status=0) at ../system/runstate.c:999 >> #16 0x55b8c3cec38e in qemu_default_main (opaque=0x0) at >> ../system/main.c:51 >> #17 0x55b8c3cec430 in main (argc=33, argv=0x7fff5742b208) at >> #../system/main.c:93 >> >> (gdb) p/x mode >> $8 = 0x > > What's the reproducer? Is it easy to reproduce? > Just hammering some make check instances in parallel, as usual. > I wonder if current_migration released already, or if monitor should still > process any QMP handler if the VM is shutting down.. > monitor_cleanup says it will continue dispatching and just never send a response to the command. It seems migration will need it's own way of tracking this. Maybe as a first step we could clear current_migration at migration_instance_finalize(). (I see device-introspect-test doesn't like it. I'll look into it) > Is this only happening after this series applied? I can't yet see how the > threadify affected it.. No, sorry, I posted here because I think this affects this series' assumptions about the BQL.
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Fri, Jan 16, 2026 at 06:48:21PM -0300, Fabiano Rosas wrote: > Another one for the pile: > > #5 0x7f0beda4fb32 in __assert_fail (assertion=0x55b8c3ed8cb8 "mode >= 0 > ...) at assert.c:103 > #6 0x55b8c3a1d7a9 in migrate_mode () at ../migration/options.c:882 > #7 0x55b8c3a1084e in fill_source_migration_info (info=0x55b8f1291650) at > ../migration/migration.c:1322 > #8 0x55b8c3a10cae in qmp_query_migrate (errp=0x7fff5742ae80) at > ../migration/migration.c:1438 > #9 0x55b8c3d4bc2a in qmp_marshal_query_migrate (args=0x55b8f11f2280, > ret=0x7f0becd25da8, errp=0x7f0becd25da0) at qapi/qapi-commands-migration.c:48 > #10 0x55b8c3d9a13b in do_qmp_dispatch_bh (opaque=0x7f0becd25e40) at > ../qapi/qmp-dispatch.c:128 > #11 0x55b8c3dc6366 in aio_bh_call (bh=0x55b8f12922d0) at > ../util/async.c:173 > #12 0x55b8c3dc6482 in aio_bh_poll (ctx=0x55b8f10741e0) at > ../util/async.c:220 > #13 0x55b8c3da9832 in aio_poll (ctx=0x55b8f10741e0, blocking=false) at > ../util/aio-posix.c:719 > #14 0x55b8c3cea017 in monitor_cleanup () at ../monitor/monitor.c:676 > #15 0x55b8c39ef646 in qemu_cleanup (status=0) at ../system/runstate.c:999 > #16 0x55b8c3cec38e in qemu_default_main (opaque=0x0) at > ../system/main.c:51 > #17 0x55b8c3cec430 in main (argc=33, argv=0x7fff5742b208) at > #../system/main.c:93 > > (gdb) p/x mode > $8 = 0x What's the reproducer? Is it easy to reproduce? I wonder if current_migration released already, or if monitor should still process any QMP handler if the VM is shutting down.. Is this only happening after this series applied? I can't yet see how the threadify affected it.. -- Peter Xu
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Wed, 22 Oct 2025 15:26:07 -0400 Peter Xu wrote: > Migration module was there for 10+ years. Initially, it was in most cases > based on coroutines. As more features were added into the framework, like > postcopy, multifd, etc.. it became a mixture of threads and coroutines. > > I'm guessing coroutines just can't fix all issues that migration want to > resolve. > > After all these years, migration is now heavily based on a threaded model. > > Now there's still a major part of migration framework that is still not > thread-based, which is precopy load. We do load in a separate thread in > postcopy since the 1st day postcopy was introduced, however that requires a > separate state transition from precopy loading all devices first, which > still happens in the main thread of a coroutine. > > This patch tries to move the migration incoming side to be run inside a > separate thread (mig/dst/main) just like the src (mig/src/main). The > entrance to be migration_incoming_thread(). > > Quite a few things are needed to make it fly.. One note here is we need to > change all these things in one patch to not break anything. The other way > to do this is add code to make all paths (that this patch touched) be ready > for either coroutine or thread. That may cause confusions in another way. > So reviewers, please take my sincere apology on the hardness of reviewing > this patch: it covers a few modules at the same time, and with some risky > changes. > > BQL Analysis > > > Firstly, when moving it over to the thread, it means the thread cannot take > BQL during the whole process of loading anymore, because otherwise it can > block main thread from using the BQL for all kinds of other concurrent > tasks (for example, processing QMP / HMP commands). > > Here the first question to ask is: what needs BQL during precopy load, and > what doesn't? > > Most of the load process shouldn't need BQL, especially when it's about > RAM. After all, RAM is still the major chunk of data to move for a live > migration process. VFIO started to change that, though, but still, VFIO is > per-device so that shouldn't need BQL either in most cases. > > Generic device loads will need BQL, likely not when receiving VMSDs, but > when applying them. One example is any post_load() could potentially > inject memory regions causing memory transactions to happen. That'll need > to update the global address spaces, hence requires BQL. The other one is > CPU sync operations, even if the sync alone may not need BQL (which is > still to be further justified), run_on_cpu() will need it. > > For that, qemu_loadvm_state() and qemu_loadvm_state_main() functions need > to now take a "bql_held" parameter saying whether bql is held. We could > use things like BQL_LOCK_GUARD(), but this patch goes with explicit > lockings rather than relying on bql_locked TLS variable. In case of > migration, we always know whether BQL is held in different context as long > as we can still pass that information downwards. > > COLO > > > COLO assumed the dest VM load happens in a coroutine. After this patch, > it's not anymore. Change that by invoking colo_incoming_co() directly from > the migration_incoming_thread(). > > The name (colo_incoming_co()) isn't proper anymore. Change it to > colo_incoming_wait(), removing the coroutine annotation alongside. > > Remove all the bql_lock() implications in COLO, e.g., colo_incoming_co() > used to release the lock for a short period while join(). Now it's not > needed. Instead, taking BQL but only when needed (colo_release_ram_cache). > > At the meantime, there's colo_incoming_co variable that used to store the > COLO incoming coroutine, only to be kicked off when a secondary failover > happens. > > To recap, what should happen for such failover should be (taking example of > a QMP command x-colo-lost-heartbeat triggering on dest QEMU): > > - The QMP command will kick off both the coroutine and the COLO > thread (colo_process_incoming_thread()), with something like: > > /* Notify COLO incoming thread that failover work is finished */ > qemu_event_set(&mis->colo_incoming_event); > > qemu_coroutine_enter(mis->colo_incoming_co); > > - The coroutine, which yielded itself before, now resumes after enter(), > then it'll wait for the join(): > > mis->colo_incoming_co = qemu_coroutine_self(); > qemu_coroutine_yield(); > mis->colo_incoming_co = NULL; > > /* Wait checkpoint incoming thread exit before free resource */ > qemu_thread_join(&th); > > Here, when switching to a thread model, it should be fine removing > colo_incoming_co variable completely, because if so, the incoming thread > will (instead of yielding the coroutine) wait at qemu_thread_join() until > the colo thread completes execution (after receiving colo_incoming_event). > > [...] > > Signed-off-by: Peter Xu > --- > include/migration/colo.h | 6 ++-- > migration/migration.h
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
Peter Xu writes:
> On Tue, Jan 13, 2026 at 10:04:02AM -0300, Fabiano Rosas wrote:
>> Peter Xu writes:
>>
>> > On Mon, Jan 12, 2026 at 04:04:12PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu writes:
>> >>
>> >> > On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
>> >> >> Peter Xu writes:
>> >> >>
>> >> >> > Migration module was there for 10+ years. Initially, it was in most
>> >> >> > cases
>> >> >> > based on coroutines. As more features were added into the
>> >> >> > framework, like
>> >> >> > postcopy, multifd, etc.. it became a mixture of threads and
>> >> >> > coroutines.
>> >> >> >
>> >> >> > I'm guessing coroutines just can't fix all issues that migration
>> >> >> > want to
>> >> >> > resolve.
>> >> >> >
>> >> >> > After all these years, migration is now heavily based on a threaded
>> >> >> > model.
>> >> >> >
>> >> >> > Now there's still a major part of migration framework that is still
>> >> >> > not
>> >> >> > thread-based, which is precopy load. We do load in a separate
>> >> >> > thread in
>> >> >> > postcopy since the 1st day postcopy was introduced, however that
>> >> >> > requires a
>> >> >> > separate state transition from precopy loading all devices first,
>> >> >> > which
>> >> >> > still happens in the main thread of a coroutine.
>> >> >> >
>> >> >> > This patch tries to move the migration incoming side to be run
>> >> >> > inside a
>> >> >> > separate thread (mig/dst/main) just like the src (mig/src/main). The
>> >> >> > entrance to be migration_incoming_thread().
>> >> >> >
>> >> >> > Quite a few things are needed to make it fly.. One note here is we
>> >> >> > need to
>> >> >> > change all these things in one patch to not break anything. The
>> >> >> > other way
>> >> >> > to do this is add code to make all paths (that this patch touched)
>> >> >> > be ready
>> >> >> > for either coroutine or thread. That may cause confusions in
>> >> >> > another way.
>> >> >> > So reviewers, please take my sincere apology on the hardness of
>> >> >> > reviewing
>> >> >> > this patch: it covers a few modules at the same time, and with some
>> >> >> > risky
>> >> >> > changes.
>> >> >> >
>> >> >> > BQL Analysis
>> >> >> >
>> >> >> >
>> >> >> > Firstly, when moving it over to the thread, it means the thread
>> >> >> > cannot take
>> >> >> > BQL during the whole process of loading anymore, because otherwise
>> >> >> > it can
>> >> >> > block main thread from using the BQL for all kinds of other
>> >> >> > concurrent
>> >> >> > tasks (for example, processing QMP / HMP commands).
>> >> >> >
>> >> >> > Here the first question to ask is: what needs BQL during precopy
>> >> >> > load, and
>> >> >> > what doesn't?
>> >> >> >
>> >> >>
>> >> >> I just noticed that the BQL held at process_incoming_migration_co is
>> >> >> also responsible for stopping qmp_migrate_set_capabilities from being
>> >> >> dispatched.
>> >> >
>> >> > I don't know if it is by design, or even if it will be guaranteed to
>> >> > work..
>> >> >
>> >>
>> >> Regardless, we shouldn't rely on the BQL for this. The BQL should be
>> >> left as last resort for things that interact across subsystems. If
>> >> someone is issuing a migration command during a migration, the migration
>> >> code is exquisitely positioned to handle that itself.
>> >
>> > Yes I agree.
>> >
>> >>
>> >> > Consider the migration incoming rocoutine runs into qemu_get_byte(), and
>> >> > then proactively yield the migration coroutine (qemu_coroutine_yield())
>> >> > when the incoming port is blocked on read..
>> >> >
>> >> > AFAIU, a proper fix for that (note, this will currently break tests) is:
>> >> >
>> >> > bool migration_is_running(void)
>> >> > {
>> >> > -MigrationState *s = current_migration;
>> >> > +MigrationStatus state;
>> >> >
>> >> > -if (!s) {
>> >> > -return false;
>> >> > +if (runstate_check(RUN_STATE_INMIGRATE)) {
>> >> > +MigrationIncomingState *mis = migration_incoming_get_current();
>> >> > +
>> >> > +if (!mis) {
>> >> > +return false;
>> >> > +}
>> >> > +
>> >> > +state = mis->state;
>> >> > +} else {
>> >> > +MigrationState *s = migrate_get_current();
>> >> > +
>> >> > +if (!s) {
>> >> > +return false;
>> >> > +}
>> >> > +
>> >> > +state = s->state;
>> >> > }
>> >> >
>> >> > -switch (s->state) {
>> >> > +switch (state) {
>> >> > case MIGRATION_STATUS_ACTIVE:
>> >> > case MIGRATION_STATUS_POSTCOPY_DEVICE:
>> >> > case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> >> >
>> >>
>> >> LGTM
>> >>
>> >> >>
>> >> >> Any point during incoming migration when BQL is unlocked we have a
>> >> >> window where a capability could be changed. Same for parameters, for
>> >> >> that matter.
>> >> >>
>> >> >> To make matters worse, the -incoming cmdline will trigger
>> >> >> qmp_migrate_incoming->...->migration_transport_compatible early on, but
>> >> >> until the channe
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Tue, Jan 13, 2026 at 10:04:02AM -0300, Fabiano Rosas wrote:
> Peter Xu writes:
>
> > On Mon, Jan 12, 2026 at 04:04:12PM -0300, Fabiano Rosas wrote:
> >> Peter Xu writes:
> >>
> >> > On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
> >> >> Peter Xu writes:
> >> >>
> >> >> > Migration module was there for 10+ years. Initially, it was in most
> >> >> > cases
> >> >> > based on coroutines. As more features were added into the framework,
> >> >> > like
> >> >> > postcopy, multifd, etc.. it became a mixture of threads and
> >> >> > coroutines.
> >> >> >
> >> >> > I'm guessing coroutines just can't fix all issues that migration want
> >> >> > to
> >> >> > resolve.
> >> >> >
> >> >> > After all these years, migration is now heavily based on a threaded
> >> >> > model.
> >> >> >
> >> >> > Now there's still a major part of migration framework that is still
> >> >> > not
> >> >> > thread-based, which is precopy load. We do load in a separate thread
> >> >> > in
> >> >> > postcopy since the 1st day postcopy was introduced, however that
> >> >> > requires a
> >> >> > separate state transition from precopy loading all devices first,
> >> >> > which
> >> >> > still happens in the main thread of a coroutine.
> >> >> >
> >> >> > This patch tries to move the migration incoming side to be run inside
> >> >> > a
> >> >> > separate thread (mig/dst/main) just like the src (mig/src/main). The
> >> >> > entrance to be migration_incoming_thread().
> >> >> >
> >> >> > Quite a few things are needed to make it fly.. One note here is we
> >> >> > need to
> >> >> > change all these things in one patch to not break anything. The
> >> >> > other way
> >> >> > to do this is add code to make all paths (that this patch touched) be
> >> >> > ready
> >> >> > for either coroutine or thread. That may cause confusions in another
> >> >> > way.
> >> >> > So reviewers, please take my sincere apology on the hardness of
> >> >> > reviewing
> >> >> > this patch: it covers a few modules at the same time, and with some
> >> >> > risky
> >> >> > changes.
> >> >> >
> >> >> > BQL Analysis
> >> >> >
> >> >> >
> >> >> > Firstly, when moving it over to the thread, it means the thread
> >> >> > cannot take
> >> >> > BQL during the whole process of loading anymore, because otherwise it
> >> >> > can
> >> >> > block main thread from using the BQL for all kinds of other concurrent
> >> >> > tasks (for example, processing QMP / HMP commands).
> >> >> >
> >> >> > Here the first question to ask is: what needs BQL during precopy
> >> >> > load, and
> >> >> > what doesn't?
> >> >> >
> >> >>
> >> >> I just noticed that the BQL held at process_incoming_migration_co is
> >> >> also responsible for stopping qmp_migrate_set_capabilities from being
> >> >> dispatched.
> >> >
> >> > I don't know if it is by design, or even if it will be guaranteed to
> >> > work..
> >> >
> >>
> >> Regardless, we shouldn't rely on the BQL for this. The BQL should be
> >> left as last resort for things that interact across subsystems. If
> >> someone is issuing a migration command during a migration, the migration
> >> code is exquisitely positioned to handle that itself.
> >
> > Yes I agree.
> >
> >>
> >> > Consider the migration incoming rocoutine runs into qemu_get_byte(), and
> >> > then proactively yield the migration coroutine (qemu_coroutine_yield())
> >> > when the incoming port is blocked on read..
> >> >
> >> > AFAIU, a proper fix for that (note, this will currently break tests) is:
> >> >
> >> > bool migration_is_running(void)
> >> > {
> >> > -MigrationState *s = current_migration;
> >> > +MigrationStatus state;
> >> >
> >> > -if (!s) {
> >> > -return false;
> >> > +if (runstate_check(RUN_STATE_INMIGRATE)) {
> >> > +MigrationIncomingState *mis = migration_incoming_get_current();
> >> > +
> >> > +if (!mis) {
> >> > +return false;
> >> > +}
> >> > +
> >> > +state = mis->state;
> >> > +} else {
> >> > +MigrationState *s = migrate_get_current();
> >> > +
> >> > +if (!s) {
> >> > +return false;
> >> > +}
> >> > +
> >> > +state = s->state;
> >> > }
> >> >
> >> > -switch (s->state) {
> >> > +switch (state) {
> >> > case MIGRATION_STATUS_ACTIVE:
> >> > case MIGRATION_STATUS_POSTCOPY_DEVICE:
> >> > case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >> >
> >>
> >> LGTM
> >>
> >> >>
> >> >> Any point during incoming migration when BQL is unlocked we have a
> >> >> window where a capability could be changed. Same for parameters, for
> >> >> that matter.
> >> >>
> >> >> To make matters worse, the -incoming cmdline will trigger
> >> >> qmp_migrate_incoming->...->migration_transport_compatible early on, but
> >> >> until the channels finally connect and process_incoming_migration_co
> >> >> starts it's possible to just change a capability in an incompatible way
> >> >> and the transport will
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
Peter Xu writes:
> On Mon, Jan 12, 2026 at 04:04:12PM -0300, Fabiano Rosas wrote:
>> Peter Xu writes:
>>
>> > On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu writes:
>> >>
>> >> > Migration module was there for 10+ years. Initially, it was in most
>> >> > cases
>> >> > based on coroutines. As more features were added into the framework,
>> >> > like
>> >> > postcopy, multifd, etc.. it became a mixture of threads and coroutines.
>> >> >
>> >> > I'm guessing coroutines just can't fix all issues that migration want to
>> >> > resolve.
>> >> >
>> >> > After all these years, migration is now heavily based on a threaded
>> >> > model.
>> >> >
>> >> > Now there's still a major part of migration framework that is still not
>> >> > thread-based, which is precopy load. We do load in a separate thread in
>> >> > postcopy since the 1st day postcopy was introduced, however that
>> >> > requires a
>> >> > separate state transition from precopy loading all devices first, which
>> >> > still happens in the main thread of a coroutine.
>> >> >
>> >> > This patch tries to move the migration incoming side to be run inside a
>> >> > separate thread (mig/dst/main) just like the src (mig/src/main). The
>> >> > entrance to be migration_incoming_thread().
>> >> >
>> >> > Quite a few things are needed to make it fly.. One note here is we
>> >> > need to
>> >> > change all these things in one patch to not break anything. The other
>> >> > way
>> >> > to do this is add code to make all paths (that this patch touched) be
>> >> > ready
>> >> > for either coroutine or thread. That may cause confusions in another
>> >> > way.
>> >> > So reviewers, please take my sincere apology on the hardness of
>> >> > reviewing
>> >> > this patch: it covers a few modules at the same time, and with some
>> >> > risky
>> >> > changes.
>> >> >
>> >> > BQL Analysis
>> >> >
>> >> >
>> >> > Firstly, when moving it over to the thread, it means the thread cannot
>> >> > take
>> >> > BQL during the whole process of loading anymore, because otherwise it
>> >> > can
>> >> > block main thread from using the BQL for all kinds of other concurrent
>> >> > tasks (for example, processing QMP / HMP commands).
>> >> >
>> >> > Here the first question to ask is: what needs BQL during precopy load,
>> >> > and
>> >> > what doesn't?
>> >> >
>> >>
>> >> I just noticed that the BQL held at process_incoming_migration_co is
>> >> also responsible for stopping qmp_migrate_set_capabilities from being
>> >> dispatched.
>> >
>> > I don't know if it is by design, or even if it will be guaranteed to work..
>> >
>>
>> Regardless, we shouldn't rely on the BQL for this. The BQL should be
>> left as last resort for things that interact across subsystems. If
>> someone is issuing a migration command during a migration, the migration
>> code is exquisitely positioned to handle that itself.
>
> Yes I agree.
>
>>
>> > Consider the migration incoming rocoutine runs into qemu_get_byte(), and
>> > then proactively yield the migration coroutine (qemu_coroutine_yield())
>> > when the incoming port is blocked on read..
>> >
>> > AFAIU, a proper fix for that (note, this will currently break tests) is:
>> >
>> > bool migration_is_running(void)
>> > {
>> > -MigrationState *s = current_migration;
>> > +MigrationStatus state;
>> >
>> > -if (!s) {
>> > -return false;
>> > +if (runstate_check(RUN_STATE_INMIGRATE)) {
>> > +MigrationIncomingState *mis = migration_incoming_get_current();
>> > +
>> > +if (!mis) {
>> > +return false;
>> > +}
>> > +
>> > +state = mis->state;
>> > +} else {
>> > +MigrationState *s = migrate_get_current();
>> > +
>> > +if (!s) {
>> > +return false;
>> > +}
>> > +
>> > +state = s->state;
>> > }
>> >
>> > -switch (s->state) {
>> > +switch (state) {
>> > case MIGRATION_STATUS_ACTIVE:
>> > case MIGRATION_STATUS_POSTCOPY_DEVICE:
>> > case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> >
>>
>> LGTM
>>
>> >>
>> >> Any point during incoming migration when BQL is unlocked we have a
>> >> window where a capability could be changed. Same for parameters, for
>> >> that matter.
>> >>
>> >> To make matters worse, the -incoming cmdline will trigger
>> >> qmp_migrate_incoming->...->migration_transport_compatible early on, but
>> >> until the channels finally connect and process_incoming_migration_co
>> >> starts it's possible to just change a capability in an incompatible way
>> >> and the transport will never be validated again.
>> >
>> > Right. Above should fix it, but I believe it also means after "-incoming
>> > tcp:xxx" (or anything not "defer") we should forbid changing migration caps
>> > or params on destination.
>> >
>>
>> Parameters are never forbidden, right? And we cannot forbid them with
>> is_running because some parameters are allowed to be changed while
>> running.
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Mon, Jan 12, 2026 at 04:04:12PM -0300, Fabiano Rosas wrote:
> Peter Xu writes:
>
> > On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
> >> Peter Xu writes:
> >>
> >> > Migration module was there for 10+ years. Initially, it was in most
> >> > cases
> >> > based on coroutines. As more features were added into the framework,
> >> > like
> >> > postcopy, multifd, etc.. it became a mixture of threads and coroutines.
> >> >
> >> > I'm guessing coroutines just can't fix all issues that migration want to
> >> > resolve.
> >> >
> >> > After all these years, migration is now heavily based on a threaded
> >> > model.
> >> >
> >> > Now there's still a major part of migration framework that is still not
> >> > thread-based, which is precopy load. We do load in a separate thread in
> >> > postcopy since the 1st day postcopy was introduced, however that
> >> > requires a
> >> > separate state transition from precopy loading all devices first, which
> >> > still happens in the main thread of a coroutine.
> >> >
> >> > This patch tries to move the migration incoming side to be run inside a
> >> > separate thread (mig/dst/main) just like the src (mig/src/main). The
> >> > entrance to be migration_incoming_thread().
> >> >
> >> > Quite a few things are needed to make it fly.. One note here is we need
> >> > to
> >> > change all these things in one patch to not break anything. The other
> >> > way
> >> > to do this is add code to make all paths (that this patch touched) be
> >> > ready
> >> > for either coroutine or thread. That may cause confusions in another
> >> > way.
> >> > So reviewers, please take my sincere apology on the hardness of reviewing
> >> > this patch: it covers a few modules at the same time, and with some risky
> >> > changes.
> >> >
> >> > BQL Analysis
> >> >
> >> >
> >> > Firstly, when moving it over to the thread, it means the thread cannot
> >> > take
> >> > BQL during the whole process of loading anymore, because otherwise it can
> >> > block main thread from using the BQL for all kinds of other concurrent
> >> > tasks (for example, processing QMP / HMP commands).
> >> >
> >> > Here the first question to ask is: what needs BQL during precopy load,
> >> > and
> >> > what doesn't?
> >> >
> >>
> >> I just noticed that the BQL held at process_incoming_migration_co is
> >> also responsible for stopping qmp_migrate_set_capabilities from being
> >> dispatched.
> >
> > I don't know if it is by design, or even if it will be guaranteed to work..
> >
>
> Regardless, we shouldn't rely on the BQL for this. The BQL should be
> left as last resort for things that interact across subsystems. If
> someone is issuing a migration command during a migration, the migration
> code is exquisitely positioned to handle that itself.
Yes I agree.
>
> > Consider the migration incoming rocoutine runs into qemu_get_byte(), and
> > then proactively yield the migration coroutine (qemu_coroutine_yield())
> > when the incoming port is blocked on read..
> >
> > AFAIU, a proper fix for that (note, this will currently break tests) is:
> >
> > bool migration_is_running(void)
> > {
> > -MigrationState *s = current_migration;
> > +MigrationStatus state;
> >
> > -if (!s) {
> > -return false;
> > +if (runstate_check(RUN_STATE_INMIGRATE)) {
> > +MigrationIncomingState *mis = migration_incoming_get_current();
> > +
> > +if (!mis) {
> > +return false;
> > +}
> > +
> > +state = mis->state;
> > +} else {
> > +MigrationState *s = migrate_get_current();
> > +
> > +if (!s) {
> > +return false;
> > +}
> > +
> > +state = s->state;
> > }
> >
> > -switch (s->state) {
> > +switch (state) {
> > case MIGRATION_STATUS_ACTIVE:
> > case MIGRATION_STATUS_POSTCOPY_DEVICE:
> > case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >
>
> LGTM
>
> >>
> >> Any point during incoming migration when BQL is unlocked we have a
> >> window where a capability could be changed. Same for parameters, for
> >> that matter.
> >>
> >> To make matters worse, the -incoming cmdline will trigger
> >> qmp_migrate_incoming->...->migration_transport_compatible early on, but
> >> until the channels finally connect and process_incoming_migration_co
> >> starts it's possible to just change a capability in an incompatible way
> >> and the transport will never be validated again.
> >
> > Right. Above should fix it, but I believe it also means after "-incoming
> > tcp:xxx" (or anything not "defer") we should forbid changing migration caps
> > or params on destination.
> >
>
> Parameters are never forbidden, right? And we cannot forbid them with
> is_running because some parameters are allowed to be changed while
> running.
Right, my above statement was definitely inaccurate.
After merging caps and params we only have params. We should only allow
some params to be changed anytime. Most of the p
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
Peter Xu writes:
> On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
>> Peter Xu writes:
>>
>> > Migration module was there for 10+ years. Initially, it was in most cases
>> > based on coroutines. As more features were added into the framework, like
>> > postcopy, multifd, etc.. it became a mixture of threads and coroutines.
>> >
>> > I'm guessing coroutines just can't fix all issues that migration want to
>> > resolve.
>> >
>> > After all these years, migration is now heavily based on a threaded model.
>> >
>> > Now there's still a major part of migration framework that is still not
>> > thread-based, which is precopy load. We do load in a separate thread in
>> > postcopy since the 1st day postcopy was introduced, however that requires a
>> > separate state transition from precopy loading all devices first, which
>> > still happens in the main thread of a coroutine.
>> >
>> > This patch tries to move the migration incoming side to be run inside a
>> > separate thread (mig/dst/main) just like the src (mig/src/main). The
>> > entrance to be migration_incoming_thread().
>> >
>> > Quite a few things are needed to make it fly.. One note here is we need to
>> > change all these things in one patch to not break anything. The other way
>> > to do this is add code to make all paths (that this patch touched) be ready
>> > for either coroutine or thread. That may cause confusions in another way.
>> > So reviewers, please take my sincere apology on the hardness of reviewing
>> > this patch: it covers a few modules at the same time, and with some risky
>> > changes.
>> >
>> > BQL Analysis
>> >
>> >
>> > Firstly, when moving it over to the thread, it means the thread cannot take
>> > BQL during the whole process of loading anymore, because otherwise it can
>> > block main thread from using the BQL for all kinds of other concurrent
>> > tasks (for example, processing QMP / HMP commands).
>> >
>> > Here the first question to ask is: what needs BQL during precopy load, and
>> > what doesn't?
>> >
>>
>> I just noticed that the BQL held at process_incoming_migration_co is
>> also responsible for stopping qmp_migrate_set_capabilities from being
>> dispatched.
>
> I don't know if it is by design, or even if it will be guaranteed to work..
>
Regardless, we shouldn't rely on the BQL for this. The BQL should be
left as last resort for things that interact across subsystems. If
someone is issuing a migration command during a migration, the migration
code is exquisitely positioned to handle that itself.
> Consider the migration incoming rocoutine runs into qemu_get_byte(), and
> then proactively yield the migration coroutine (qemu_coroutine_yield())
> when the incoming port is blocked on read..
>
> AFAIU, a proper fix for that (note, this will currently break tests) is:
>
> bool migration_is_running(void)
> {
> -MigrationState *s = current_migration;
> +MigrationStatus state;
>
> -if (!s) {
> -return false;
> +if (runstate_check(RUN_STATE_INMIGRATE)) {
> +MigrationIncomingState *mis = migration_incoming_get_current();
> +
> +if (!mis) {
> +return false;
> +}
> +
> +state = mis->state;
> +} else {
> +MigrationState *s = migrate_get_current();
> +
> +if (!s) {
> +return false;
> +}
> +
> +state = s->state;
> }
>
> -switch (s->state) {
> +switch (state) {
> case MIGRATION_STATUS_ACTIVE:
> case MIGRATION_STATUS_POSTCOPY_DEVICE:
> case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>
LGTM
>>
>> Any point during incoming migration when BQL is unlocked we have a
>> window where a capability could be changed. Same for parameters, for
>> that matter.
>>
>> To make matters worse, the -incoming cmdline will trigger
>> qmp_migrate_incoming->...->migration_transport_compatible early on, but
>> until the channels finally connect and process_incoming_migration_co
>> starts it's possible to just change a capability in an incompatible way
>> and the transport will never be validated again.
>
> Right. Above should fix it, but I believe it also means after "-incoming
> tcp:xxx" (or anything not "defer") we should forbid changing migration caps
> or params on destination.
>
Parameters are never forbidden, right? And we cannot forbid them with
is_running because some parameters are allowed to be changed while
running.
I feel we should have a more fine grained way of saying "this option
cannot be set at this moment", instead of just using the state as a
proxy. States can change, while the fact that from a certain point on,
certain options should not be touched anymore doesn't change.
Maybe a little infra like bdrv_op_is_blocked, i.e, a list of blocked
operations. It could be set in qmp_migrate and checked in
qmp_set_parameters/caps.
> As discussed above, that'll at least break our qtests. But frankly
> speaking I think that's the right thing to do.. I hope libvirt alwa
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
> Peter Xu writes:
>
> > Migration module was there for 10+ years. Initially, it was in most cases
> > based on coroutines. As more features were added into the framework, like
> > postcopy, multifd, etc.. it became a mixture of threads and coroutines.
> >
> > I'm guessing coroutines just can't fix all issues that migration want to
> > resolve.
> >
> > After all these years, migration is now heavily based on a threaded model.
> >
> > Now there's still a major part of migration framework that is still not
> > thread-based, which is precopy load. We do load in a separate thread in
> > postcopy since the 1st day postcopy was introduced, however that requires a
> > separate state transition from precopy loading all devices first, which
> > still happens in the main thread of a coroutine.
> >
> > This patch tries to move the migration incoming side to be run inside a
> > separate thread (mig/dst/main) just like the src (mig/src/main). The
> > entrance to be migration_incoming_thread().
> >
> > Quite a few things are needed to make it fly.. One note here is we need to
> > change all these things in one patch to not break anything. The other way
> > to do this is add code to make all paths (that this patch touched) be ready
> > for either coroutine or thread. That may cause confusions in another way.
> > So reviewers, please take my sincere apology on the hardness of reviewing
> > this patch: it covers a few modules at the same time, and with some risky
> > changes.
> >
> > BQL Analysis
> >
> >
> > Firstly, when moving it over to the thread, it means the thread cannot take
> > BQL during the whole process of loading anymore, because otherwise it can
> > block main thread from using the BQL for all kinds of other concurrent
> > tasks (for example, processing QMP / HMP commands).
> >
> > Here the first question to ask is: what needs BQL during precopy load, and
> > what doesn't?
> >
>
> I just noticed that the BQL held at process_incoming_migration_co is
> also responsible for stopping qmp_migrate_set_capabilities from being
> dispatched.
I don't know if it is by design, or even if it will be guaranteed to work..
Consider the migration incoming rocoutine runs into qemu_get_byte(), and
then proactively yield the migration coroutine (qemu_coroutine_yield())
when the incoming port is blocked on read..
AFAIU, a proper fix for that (note, this will currently break tests) is:
bool migration_is_running(void)
{
-MigrationState *s = current_migration;
+MigrationStatus state;
-if (!s) {
-return false;
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+MigrationIncomingState *mis = migration_incoming_get_current();
+
+if (!mis) {
+return false;
+}
+
+state = mis->state;
+} else {
+MigrationState *s = migrate_get_current();
+
+if (!s) {
+return false;
+}
+
+state = s->state;
}
-switch (s->state) {
+switch (state) {
case MIGRATION_STATUS_ACTIVE:
case MIGRATION_STATUS_POSTCOPY_DEVICE:
case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>
> Any point during incoming migration when BQL is unlocked we have a
> window where a capability could be changed. Same for parameters, for
> that matter.
>
> To make matters worse, the -incoming cmdline will trigger
> qmp_migrate_incoming->...->migration_transport_compatible early on, but
> until the channels finally connect and process_incoming_migration_co
> starts it's possible to just change a capability in an incompatible way
> and the transport will never be validated again.
Right. Above should fix it, but I believe it also means after "-incoming
tcp:xxx" (or anything not "defer") we should forbid changing migration caps
or params on destination.
As discussed above, that'll at least break our qtests. But frankly
speaking I think that's the right thing to do.. I hope libvirt always
works with "defer" and never update any caps/params after QMP
migrate_incoming.
So I wonder if I should continue with above patch, and then fix our qtests.
Your work from the other "merge caps+params" might also work here,
actually, if we make sure everything will be set alone with the QMP
migrate_incoming single command.
Let me know your initial thoughts, then I'll see what I can do..
Thanks,
>
> One example:
>
> -- >8 --
> From 99bd88aa0a8b6d4e7c52196f25d344a2800b3d89 Mon Sep 17 00:00:00 2001
> From: Fabiano Rosas
> Date: Thu, 8 Jan 2026 17:21:20 -0300
> Subject: [PATCH] tmp
>
> ---
> tests/qtest/migration/precopy-tests.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/tests/qtest/migration/precopy-tests.c
> b/tests/qtest/migration/precopy-tests.c
> index aca7ed51ef..3f1a2870ee 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -158,6 +158,13 @@ static int new_rdma_link(char *buffer, bool ipv6)
> retur
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
Peter Xu writes:
> Migration module was there for 10+ years. Initially, it was in most cases
> based on coroutines. As more features were added into the framework, like
> postcopy, multifd, etc.. it became a mixture of threads and coroutines.
>
> I'm guessing coroutines just can't fix all issues that migration want to
> resolve.
>
> After all these years, migration is now heavily based on a threaded model.
>
> Now there's still a major part of migration framework that is still not
> thread-based, which is precopy load. We do load in a separate thread in
> postcopy since the 1st day postcopy was introduced, however that requires a
> separate state transition from precopy loading all devices first, which
> still happens in the main thread of a coroutine.
>
> This patch tries to move the migration incoming side to be run inside a
> separate thread (mig/dst/main) just like the src (mig/src/main). The
> entrance to be migration_incoming_thread().
>
> Quite a few things are needed to make it fly.. One note here is we need to
> change all these things in one patch to not break anything. The other way
> to do this is add code to make all paths (that this patch touched) be ready
> for either coroutine or thread. That may cause confusions in another way.
> So reviewers, please take my sincere apology on the hardness of reviewing
> this patch: it covers a few modules at the same time, and with some risky
> changes.
>
> BQL Analysis
>
>
> Firstly, when moving it over to the thread, it means the thread cannot take
> BQL during the whole process of loading anymore, because otherwise it can
> block main thread from using the BQL for all kinds of other concurrent
> tasks (for example, processing QMP / HMP commands).
>
> Here the first question to ask is: what needs BQL during precopy load, and
> what doesn't?
>
I just noticed that the BQL held at process_incoming_migration_co is
also responsible for stopping qmp_migrate_set_capabilities from being
dispatched.
Any point during incoming migration when BQL is unlocked we have a
window where a capability could be changed. Same for parameters, for
that matter.
To make matters worse, the -incoming cmdline will trigger
qmp_migrate_incoming->...->migration_transport_compatible early on, but
until the channels finally connect and process_incoming_migration_co
starts it's possible to just change a capability in an incompatible way
and the transport will never be validated again.
One example:
-- >8 --
>From 99bd88aa0a8b6d4e7c52196f25d344a2800b3d89 Mon Sep 17 00:00:00 2001
From: Fabiano Rosas
Date: Thu, 8 Jan 2026 17:21:20 -0300
Subject: [PATCH] tmp
---
tests/qtest/migration/precopy-tests.c | 8
1 file changed, 8 insertions(+)
diff --git a/tests/qtest/migration/precopy-tests.c
b/tests/qtest/migration/precopy-tests.c
index aca7ed51ef..3f1a2870ee 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -158,6 +158,13 @@ static int new_rdma_link(char *buffer, bool ipv6)
return -1;
}
+static void *migrate_rdma_set_caps(QTestState *from, QTestState *to)
+{
+migrate_set_capability(to, "mapped-ram", true);
+
+return NULL;
+}
+
static void __test_precopy_rdma_plain(MigrateCommon *args, bool ipv6)
{
char buffer[128] = {};
@@ -185,6 +192,7 @@ static void __test_precopy_rdma_plain(MigrateCommon *args,
bool ipv6)
args->listen_uri = uri;
args->connect_uri = uri;
+args->start_hook = migrate_rdma_set_caps;
test_precopy_common(args);
}
--
2.51.0
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On Tue, Nov 04, 2025 at 02:40:53AM +, Zhijian Li (Fujitsu) wrote: > Both COLO and RDMA changes look good to me > > Reviewed-by: Li Zhijian # COLO and RDMA > > And with an addition fixes[1] for COLO, the whole patchset: > > Tested-by: Li Zhijian # COLO and RDMA > > [1] > https://lore.kernel.org/qemu-devel/[email protected]/ This is helpful, thanks a lot Zhijian. -- Peter Xu
Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process
On 23/10/2025 03:26, Peter Xu wrote: > Migration module was there for 10+ years. Initially, it was in most cases > based on coroutines. As more features were added into the framework, like > postcopy, multifd, etc.. it became a mixture of threads and coroutines. > > I'm guessing coroutines just can't fix all issues that migration want to > resolve. > > After all these years, migration is now heavily based on a threaded model. > > Now there's still a major part of migration framework that is still not > thread-based, which is precopy load. We do load in a separate thread in > postcopy since the 1st day postcopy was introduced, however that requires a > separate state transition from precopy loading all devices first, which > still happens in the main thread of a coroutine. > > This patch tries to move the migration incoming side to be run inside a > separate thread (mig/dst/main) just like the src (mig/src/main). The > entrance to be migration_incoming_thread(). > > Quite a few things are needed to make it fly.. One note here is we need to > change all these things in one patch to not break anything. The other way > to do this is add code to make all paths (that this patch touched) be ready > for either coroutine or thread. That may cause confusions in another way. > So reviewers, please take my sincere apology on the hardness of reviewing > this patch: it covers a few modules at the same time, and with some risky > changes. > > BQL Analysis > > > Firstly, when moving it over to the thread, it means the thread cannot take > BQL during the whole process of loading anymore, because otherwise it can > block main thread from using the BQL for all kinds of other concurrent > tasks (for example, processing QMP / HMP commands). > > Here the first question to ask is: what needs BQL during precopy load, and > what doesn't? > > Most of the load process shouldn't need BQL, especially when it's about > RAM. After all, RAM is still the major chunk of data to move for a live > migration process. VFIO started to change that, though, but still, VFIO is > per-device so that shouldn't need BQL either in most cases. > > Generic device loads will need BQL, likely not when receiving VMSDs, but > when applying them. One example is any post_load() could potentially > inject memory regions causing memory transactions to happen. That'll need > to update the global address spaces, hence requires BQL. The other one is > CPU sync operations, even if the sync alone may not need BQL (which is > still to be further justified), run_on_cpu() will need it. > > For that, qemu_loadvm_state() and qemu_loadvm_state_main() functions need > to now take a "bql_held" parameter saying whether bql is held. We could > use things like BQL_LOCK_GUARD(), but this patch goes with explicit > lockings rather than relying on bql_locked TLS variable. In case of > migration, we always know whether BQL is held in different context as long > as we can still pass that information downwards. > > COLO > > > COLO assumed the dest VM load happens in a coroutine. After this patch, > it's not anymore. Change that by invoking colo_incoming_co() directly from > the migration_incoming_thread(). > > The name (colo_incoming_co()) isn't proper anymore. Change it to > colo_incoming_wait(), removing the coroutine annotation alongside. > > Remove all the bql_lock() implications in COLO, e.g., colo_incoming_co() > used to release the lock for a short period while join(). Now it's not > needed. Instead, taking BQL but only when needed (colo_release_ram_cache). > > At the meantime, there's colo_incoming_co variable that used to store the > COLO incoming coroutine, only to be kicked off when a secondary failover > happens. > > To recap, what should happen for such failover should be (taking example of > a QMP command x-colo-lost-heartbeat triggering on dest QEMU): > >- The QMP command will kick off both the coroutine and the COLO > thread (colo_process_incoming_thread()), with something like: > > /* Notify COLO incoming thread that failover work is finished */ > qemu_event_set(&mis->colo_incoming_event); > > qemu_coroutine_enter(mis->colo_incoming_co); > >- The coroutine, which yielded itself before, now resumes after enter(), > then it'll wait for the join(): > > mis->colo_incoming_co = qemu_coroutine_self(); > qemu_coroutine_yield(); > mis->colo_incoming_co = NULL; > > /* Wait checkpoint incoming thread exit before free resource */ > qemu_thread_join(&th); > > Here, when switching to a thread model, it should be fine removing > colo_incoming_co variable completely, because if so, the incoming thread > will (instead of yielding the coroutine) wait at qemu_thread_join() until > the colo thread completes execution (after receiving colo_incoming_event). > > RDMA > > > With the prior patch making sure io_watch won't block for RDMA iochannels, > RDMA threads s
