On Wed, Dec 11, 2024 at 12:05:23AM +0100, Maciej S. Szmigiero wrote:
Maybe move it over to migration_object_init()? Then we keep
qemu_loadvm_state_setup() only invoke the load_setup()s.
AFAIK migration_object_init() is called unconditionally
at QEMU startup even if there won't me any migration done?
Creating a load thread pool there seems wasteful if no
incoming migration will ever take place (or will but only
much later).
I was expecting an empty pool to not be a major resource, but if that's a
concern, yes we can do that until later.
[...]
@@ -3007,6 +3071,19 @@ int qemu_loadvm_state(QEMUFile *f)
return ret;
}
+ if (ret == 0) {
+ bql_unlock(); /* Let load threads do work requiring BQL */
+ thread_pool_wait(load_threads);
+ bql_lock();
+
+ ret = load_threads_ret;
+ }
+ /*
+ * Set this flag unconditionally so we'll catch further attempts to
+ * start additional threads via an appropriate assert()
+ */
+ qatomic_set(&load_threads_abort, true);
I assume this is only for debugging purpose and not required.
Setting "abort all threads" to make sure "nobody will add more thread
tasks" is pretty awkward, IMHO. If we really want to protect against it
and fail hard, it might be easier after the thread_pool_wait() we free the
pool directly (destroy() will see NULL so it'll skip; still need to free
there in case migration failed before this). Then any enqueue will access
null pointer on the pool.
We don't want to destroy the thread pool in the path where the downtime
is still counting.
Yeah this makes sense.
That's why we only do cleanup after the migration is complete.
The above setting of load_threads_abort flag also makes sure that we abort
load threads if the migration is going to fail for other reasons (non-load
threads related) - in other words, when the above block with thread_pool_wait()
isn't even entered due to ret already containing an earlier error.
In that case IIUC we should cleanup the load threads in destroy(), not
here? Especially with the comment that's even more confusing.