On 22.10.25 22:26, Peter Xu wrote:
Teach qemu_loadvm_state() and some of the internal functions to know
whether we're holding BQL or not.
Actually, this commit does more: not only pass the bql_held information,
but also by introduce some WITH_BQL_HELD() sections.
IMHO it could be split:
1. only add bql_held parameters, which are used only to passthorough to
called functions. That's just to simplify further commits. Make the
information available. At this commit, we only need to check, that
passed information is correct (is it really held, may be add some
comments/assertions to make it obvious)
2. one or more commits, which prepares different functions to be called
in thread: support bql_held=false by introducing WITH_BQL_HELD() sections.
In such commit, we should lookthorgh the whole function and check that it
actually prepared to be called from thread.
Hmm, or without [1.], there may be several commits to prepare different
functions. Or maybe, even one commit as it is, but change commit subject
and message somehow, to reflect all the changes..
So far, all the callers still always pass in TRUE, hence no functional
change expected. But it may change in the near future.
To reviewers: even if this is not functional change yet, it'll be the major
core functional change after we switch to threadified loadvm soon. Please
Treat it as one to add explicit code to mark out which part of incoming
live migration would need to be executed always with the BQL, or would need
to be run always without BQL.
Signed-off-by: Peter Xu <[email protected]>
---
[..]
diff --git a/migration/colo.c b/migration/colo.c
index db783f6fa7..4fd586951a 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -686,7 +686,7 @@ static void
colo_incoming_process_checkpoint(MigrationIncomingState *mis,
bql_lock();
cpu_synchronize_all_states();
- ret = qemu_loadvm_state_main(mis->from_src_file, mis, errp);
+ ret = qemu_loadvm_state_main(mis->from_src_file, mis, true, errp);
That one is obvious..
bql_unlock();
if (ret < 0) {
diff --git a/migration/migration.c b/migration/migration.c
index 4ed2a2e881..38a584afae 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -878,7 +878,7 @@ process_incoming_migration_co(void *opaque)
MIGRATION_STATUS_ACTIVE);
mis->loadvm_co = qemu_coroutine_self();
- ret = qemu_loadvm_state(mis->from_src_file, &local_err);
+ ret = qemu_loadvm_state(mis->from_src_file, true, &local_err);
Here, why are we sure? Are coroutines triggered by QMP command always run under
BQL?
Maybe, worth an assertion.
mis->loadvm_co = NULL;
trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
diff --git a/migration/savevm.c b/migration/savevm.c
index 232cae090b..44aadc2f51 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -154,11 +154,12 @@ static void
qemu_loadvm_thread_pool_destroy(MigrationIncomingState *mis)
}
static bool qemu_loadvm_thread_pool_wait(MigrationState *s,
- MigrationIncomingState *mis)
+ MigrationIncomingState *mis,
+ bool bql_held)
{
- bql_unlock(); /* Let load threads do work requiring BQL */
- thread_pool_wait(mis->load_threads);
- bql_lock();
+ WITH_BQL_RELEASED(bql_held) {
+ thread_pool_wait(mis->load_threads);
+ }
return !migrate_has_error(s);
The function is now prepared to be called from thread, as migrate_has_error()
has own mutex.
}
@@ -2117,7 +2118,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
qemu_file_set_blocking(f, true, &error_fatal);
/* TODO: sanity check that only postcopiable data will be loaded here */
- load_res = qemu_loadvm_state_main(f, mis, &local_err);
+ load_res = qemu_loadvm_state_main(f, mis, true, &local_err);
Is it correct? I see, a bit later, postcopy_ram_listen_thread() does
bql_lock();
migration_incoming_state_destroy();
bql_unlock();
so, I assume, that before this, when we call qemu_loadvm_state_main(), BQL is
not actually locked?
/*
* This is tricky, but, mis->from_src_file can change after it
@@ -2420,7 +2421,8 @@ static void
loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
* Returns: Negative values on error
*
*/
-static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error
**errp)
+static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis,
+ bool bql_held, Error **errp)
{
int ret;
[..]
--
Best regards,
Vladimir
