On 2.02.2025 13:45, Dr. David Alan Gilbert wrote:
* Maciej S. Szmigiero (m...@maciej.szmigiero.name) wrote:
On 2.02.2025 03:06, Dr. David Alan Gilbert wrote:
* Maciej S. Szmigiero (m...@maciej.szmigiero.name) wrote:
From: "Maciej S. Szmigiero" <maciej.szmigi...@oracle.com>
postcopy_ram_listen_thread() is a free running thread, so it needs to
take BQL around function calls to migration methods requiring BQL.
qemu_loadvm_state_main() needs BQL held since it ultimately calls
"load_state" SaveVMHandlers.
migration_incoming_state_destroy() needs BQL held since it ultimately calls
"load_cleanup" SaveVMHandlers.
Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
migration/savevm.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/migration/savevm.c b/migration/savevm.c
index b0b74140daea..0ceea9638cc1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2013,7 +2013,9 @@ static void *postcopy_ram_listen_thread(void *opaque)
* in qemu_file, and thus we must be blocking now.
*/
qemu_file_set_blocking(f, true);
+ bql_lock();
load_res = qemu_loadvm_state_main(f, mis);
+ bql_unlock();
Doesn't that leave that held for a heck of a long time?
Yes, and it effectively broke "postcopy recover" test but I
think the reason for that is qemu_loadvm_state_main() and
its children don't drop BQL while waiting for I/O.
I've described this case in more detail in my reply to Fabiano here:
https://lore.kernel.org/qemu-devel/0a09e627-955e-4f26-8d08-0192ecd25...@maciej.szmigiero.name/
While it might be the cause in this case, my feeling is it's more fundamental
here - it's the whole reason that postcopy has a separate ram listen
thread. As the destination is running, after it loads it's devices
and as it starts up the destination will be still loading RAM
(and other postcopiable devices) potentially for quite a while.
Holding the bql around the ram listen thread means that the
execution of the destination won't be able to take that lock
until the postcopy load has finished; so while that might apparently
complete, it'll lead to the destination stalling until that's finished
which defeats the whole point of postcopy.
That last one probably won't fail a test but it will lead to a long stall
if you give it a nice big guest with lots of RAM that it's rapidly
changing.
Okay, I understand the postcopy case/flow now.
Thanks for explaining it clearly.
I still think that "load_state" SaveVMHandlers need to be called
with BQL held since implementations apparently expect it that way:
for example, I think PCI device configuration restore calls
address space manipulation methods which abort() if called
without BQL held.
However, the only devices that *should* be arriving on the channel
that the postcopy_ram_listen_thread is reading from are those
that are postcopiable (i.e. RAM and hmm block's dirty_bitmap).
Those load handlers are safe to be run while the other devices
are being changed. Note the *should* - you could add a check
to fail if any other device arrives on that channel.
I think ultimately there should be either an explicit check, or,
as you suggest in the paragraph below, a separate SaveVMHandler
that runs without BQL held.
Since the current state of just running these SaveVMHandlers
without BQL in this case and hoping that nothing breaks is
clearly sub-optimal.
I have previously even submitted a patch to explicitly document
"load_state" SaveVMHandler as requiring BQL (which was also
included in the previous version of this patch set) and it
received a "Reviewed-by:" tag:
https://lore.kernel.org/qemu-devel/6976f129df610c8207da4e531c8c0475ec204fa4.1730203967.git.maciej.szmigi...@oracle.com/
https://lore.kernel.org/qemu-devel/e1949839932efaa531e2fe63ac13324e5787439c.1731773021.git.maciej.szmigi...@oracle.com/
https://lore.kernel.org/qemu-devel/87o732bti7....@suse.de/
It happens!
You could make this safer by having a load_state and a load_state_postcopy
member, and only mark the load_state as requiring the lock.
To not digress too much from the subject of this patch set
(multifd VFIO device state transfer) for now I've just updated the
TODO comment around that qemu_loadvm_state_main(), so hopefully this
discussion won't get forgotten:
https://gitlab.com/maciejsszmigiero/qemu/-/commit/046e3deac5b1dbc406b3e9571f62468bd6743e79
(..)
That RAM loading has to happen in parallel with the loading of
devices doesn't it - especially if one of the devices
being loaded touches RAM.
(I wish this series had a description in the cover letter!)
I guess you mean "more detailed description" since there's
a paragraph about this patch in this series cover letter change log:
* postcopy_ram_listen_thread() now takes BQL around function calls that
ultimately call migration methods requiring BQL.
This fixes one of QEMU tests failing when explicitly BQL-sensitive code
is added later to these methods.
I meant a higher level description of what the series is doing.
There was a general overview what this series is trying to achieve
on its RFC (version 0) cover letter:
https://lore.kernel.org/qemu-devel/cover.1713269378.git.maciej.szmigi...@oracle.com/
I also did a presentation about this patch set during last year's KVM Forum,
its slide deck is here:
https://pretalx.com/media/kvm-forum-2024/submissions/ZSYR9Z/resources/kvm-forum-2024-multifd-device-state-transfer_3K5EQIG.pdf
Dave
Thanks,
Maciej
