vmm: allow coredump no need pause firstly #5604
Conversation
There was a problem hiding this comment.
What's the motivation for this? Is pausing the VM first really that much of an inconvenience - are you worried about a race?
Also I would be worried that pausing is actually an async operation - you need to wait for the virtio/CPU threads to spin round their loop and notice that they should be paused. With the separate pause HTTP action there there is less likely to be a race. Possibly we should block the pause until everything is paused but that is quite a lot of work.
| } | ||
| VmState::Paused => {} | ||
| _ => { | ||
| return Err(GuestDebuggableError::Coredump(anyhow!( |
There was a problem hiding this comment.
This error message doesn't make any sense now.
|
Thanks for you quick reply, Rob.
Yes, it may not be so convenient to do coredump now. One should do pause -> coredump -> resume every time for now, and with this patch, just call coredump one command :) Also, qemu does stop vm operation in qmp dump command, which may be used for our reference.
Yes, I didn't consider the async situation, thanks for pointing this. I looked into the pause trait method implemented in each cpu and device, maybe it's not hard to sync pause if I am right. For cpu, we can use join_thread() in VcpuState to wait vcpu thread loop exit. For virtio device, almost every of them call pause() in VirtioCommon, which calls paused_sync wait() to make sure thread in virtio device exits safely. |
Great, please create an issue (or PR) to ensure that pause is synchronised. We can then revisit this PR as I think that the coredump is only really useful if you can be sure that everything is correctly paused. |
Okay, I will have a try. |
|
I have rebased from latest upstream including the synchronous pause patch, please review again :) |
vmm/src/vm.rs
Outdated
| VmState::Paused => {} | ||
| _ => { | ||
| return Err(GuestDebuggableError::Coredump(anyhow!( | ||
| "Trying to coredump while VM is running" |
There was a problem hiding this comment.
Please update this error message accordingly.
| @@ -36,6 +36,8 @@ pub struct DumpState { | |||
| pub enum GuestDebuggableError { | |||
| Coredump(anyhow::Error), | |||
There was a problem hiding this comment.
Perhaps this is a better commit message:
vmm: Automatically pause VM for coredump
If the VMM is not already paused then pause the VM prior to executing the coredump and then resume it after. If the VM is already paused then the original state is maintained.
If the VMM is not already paused then pause the VM prior to executing the coredump and then resume it after. If the VM is already paused then the original state is maintained. Signed-off-by: Yi Wang <foxywang@tencent.com>
Add integration test of coredump with no need pause. As file of coredump has been tested in test_coredump(), so this patch only test vm state after coredump. Signed-off-by: Yi Wang <foxywang@tencent.com>
|
@up2wing Could you please share a way to verify the patches? How to enable the feature of guest_debug in the case? |
|
Coredump only supports x86_64 for now. |
It makes sense pause/resume in coredump() method, so api caller doesn't need to pause vm firstly.
Also, add integration test of this modification.