Re: vmm: remove vm teardown from vcpu run path (testers needed)
On Thu, Jun 02, 2022 at 03:05:16PM -0400, Dave Voutila wrote:
>
> Dave Voutila writes:
>
> > tech@ et al.:
> >
> > Looking for testers of the following diff for vmm(4). In my efforts to
> > fix some stability issues, I'm taking baby steps tweaking parts of the
> > code to make my upcoming proposal (adding refcnts) easier to swallow.
> >
> > This change removes the calling of vm_teardown from the code path in
> > vm_run after vmm has exited the vm/vcpu and is on its way back to
> > userland/vmd(8).
> >
> > vm_teardown is currently called in 3 areas to destroy/free a vm:
> >
> > - vm_create: as cleanup in an error path
> > - vm_terminate: on a vm the ioctl is killing
> > - vm_run: the run ioctl handler
> >
> > This diff removes that last bullet. It's not needed as vmd will cleanup
> > the vm on child exit, calling vm_terminate. Any non-vmd user of vmm(4)
> > can stop being lazy and use the VMM_IOC_TERM ioctl.
> >
> > Not included in the snippet is the existing final else block that still
> > toggles the vcpu state:
> >
> > } else {
> > vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
> > vcpu->vc_state = VCPU_STATE_TERMINATED;
> > }
> >
> > If testing, please describe *any* difference in shutdown/reboot of vm
> > guests. (n.b. there's a known issue for Linux guests running very recent
> > Linux kernels not being able to reboot. That needs to be addressed in
> > vmd.)
> >
>
> Bumping as the diff has been out for testing and looking for ok's.
>
> -dv
>
ok mlarkin if this helps your subsequent cleanup
> >
> >
> > Index: sys/arch/amd64/amd64/vmm.c
> > ===
> > RCS file: /opt/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> > retrieving revision 1.311
> > diff -u -p -r1.311 vmm.c
> > --- sys/arch/amd64/amd64/vmm.c 20 May 2022 22:42:09 - 1.311
> > +++ sys/arch/amd64/amd64/vmm.c 23 May 2022 11:57:49 -
> > @@ -4495,22 +4495,8 @@ vm_run(struct vm_run_params *vrp)
> > ret = vcpu_run_svm(vcpu, vrp);
> > }
> >
> > - /*
> > -* We can set the VCPU states here without CAS because once
> > -* a VCPU is in state RUNNING or REQTERM, only the VCPU itself
> > -* can switch the state.
> > -*/
> > atomic_dec_int(>vm_vcpus_running);
> > - if (vcpu->vc_state == VCPU_STATE_REQTERM) {
> > - vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
> > - vcpu->vc_state = VCPU_STATE_TERMINATED;
> > - if (vm->vm_vcpus_running == 0) {
> > - rw_enter_write(_softc->vm_lock);
> > - vm_teardown(vm);
> > - rw_exit_write(_softc->vm_lock);
> > - }
> > - ret = 0;
> > - } else if (ret == 0 || ret == EAGAIN) {
> > + if (ret == 0 || ret == EAGAIN) {
> > /* If we are exiting, populate exit data so vmd can help. */
> > vrp->vrp_exit_reason = (ret == 0) ? VM_EXIT_NONE
> > : vcpu->vc_gueststate.vg_exit_reason;
>
>
> --
> -Dave Voutila
Re: vmm: remove vm teardown from vcpu run path (testers needed)
Dave Voutila writes:
> tech@ et al.:
>
> Looking for testers of the following diff for vmm(4). In my efforts to
> fix some stability issues, I'm taking baby steps tweaking parts of the
> code to make my upcoming proposal (adding refcnts) easier to swallow.
>
> This change removes the calling of vm_teardown from the code path in
> vm_run after vmm has exited the vm/vcpu and is on its way back to
> userland/vmd(8).
>
> vm_teardown is currently called in 3 areas to destroy/free a vm:
>
> - vm_create: as cleanup in an error path
> - vm_terminate: on a vm the ioctl is killing
> - vm_run: the run ioctl handler
>
> This diff removes that last bullet. It's not needed as vmd will cleanup
> the vm on child exit, calling vm_terminate. Any non-vmd user of vmm(4)
> can stop being lazy and use the VMM_IOC_TERM ioctl.
>
> Not included in the snippet is the existing final else block that still
> toggles the vcpu state:
>
> } else {
> vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
> vcpu->vc_state = VCPU_STATE_TERMINATED;
> }
>
> If testing, please describe *any* difference in shutdown/reboot of vm
> guests. (n.b. there's a known issue for Linux guests running very recent
> Linux kernels not being able to reboot. That needs to be addressed in
> vmd.)
>
Bumping as the diff has been out for testing and looking for ok's.
-dv
>
>
> Index: sys/arch/amd64/amd64/vmm.c
> ===
> RCS file: /opt/cvs/src/sys/arch/amd64/amd64/vmm.c,v
> retrieving revision 1.311
> diff -u -p -r1.311 vmm.c
> --- sys/arch/amd64/amd64/vmm.c20 May 2022 22:42:09 - 1.311
> +++ sys/arch/amd64/amd64/vmm.c23 May 2022 11:57:49 -
> @@ -4495,22 +4495,8 @@ vm_run(struct vm_run_params *vrp)
> ret = vcpu_run_svm(vcpu, vrp);
> }
>
> - /*
> - * We can set the VCPU states here without CAS because once
> - * a VCPU is in state RUNNING or REQTERM, only the VCPU itself
> - * can switch the state.
> - */
> atomic_dec_int(>vm_vcpus_running);
> - if (vcpu->vc_state == VCPU_STATE_REQTERM) {
> - vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
> - vcpu->vc_state = VCPU_STATE_TERMINATED;
> - if (vm->vm_vcpus_running == 0) {
> - rw_enter_write(_softc->vm_lock);
> - vm_teardown(vm);
> - rw_exit_write(_softc->vm_lock);
> - }
> - ret = 0;
> - } else if (ret == 0 || ret == EAGAIN) {
> + if (ret == 0 || ret == EAGAIN) {
> /* If we are exiting, populate exit data so vmd can help. */
> vrp->vrp_exit_reason = (ret == 0) ? VM_EXIT_NONE
> : vcpu->vc_gueststate.vg_exit_reason;
--
-Dave Voutila
vmm: remove vm teardown from vcpu run path (testers needed)
tech@ et al.:
Looking for testers of the following diff for vmm(4). In my efforts to
fix some stability issues, I'm taking baby steps tweaking parts of the
code to make my upcoming proposal (adding refcnts) easier to swallow.
This change removes the calling of vm_teardown from the code path in
vm_run after vmm has exited the vm/vcpu and is on its way back to
userland/vmd(8).
vm_teardown is currently called in 3 areas to destroy/free a vm:
- vm_create: as cleanup in an error path
- vm_terminate: on a vm the ioctl is killing
- vm_run: the run ioctl handler
This diff removes that last bullet. It's not needed as vmd will cleanup
the vm on child exit, calling vm_terminate. Any non-vmd user of vmm(4)
can stop being lazy and use the VMM_IOC_TERM ioctl.
Not included in the snippet is the existing final else block that still
toggles the vcpu state:
} else {
vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
vcpu->vc_state = VCPU_STATE_TERMINATED;
}
If testing, please describe *any* difference in shutdown/reboot of vm
guests. (n.b. there's a known issue for Linux guests running very recent
Linux kernels not being able to reboot. That needs to be addressed in
vmd.)
thx,
dv
Index: sys/arch/amd64/amd64/vmm.c
===
RCS file: /opt/cvs/src/sys/arch/amd64/amd64/vmm.c,v
retrieving revision 1.311
diff -u -p -r1.311 vmm.c
--- sys/arch/amd64/amd64/vmm.c 20 May 2022 22:42:09 - 1.311
+++ sys/arch/amd64/amd64/vmm.c 23 May 2022 11:57:49 -
@@ -4495,22 +4495,8 @@ vm_run(struct vm_run_params *vrp)
ret = vcpu_run_svm(vcpu, vrp);
}
- /*
-* We can set the VCPU states here without CAS because once
-* a VCPU is in state RUNNING or REQTERM, only the VCPU itself
-* can switch the state.
-*/
atomic_dec_int(>vm_vcpus_running);
- if (vcpu->vc_state == VCPU_STATE_REQTERM) {
- vrp->vrp_exit_reason = VM_EXIT_TERMINATED;
- vcpu->vc_state = VCPU_STATE_TERMINATED;
- if (vm->vm_vcpus_running == 0) {
- rw_enter_write(_softc->vm_lock);
- vm_teardown(vm);
- rw_exit_write(_softc->vm_lock);
- }
- ret = 0;
- } else if (ret == 0 || ret == EAGAIN) {
+ if (ret == 0 || ret == EAGAIN) {
/* If we are exiting, populate exit data so vmd can help. */
vrp->vrp_exit_reason = (ret == 0) ? VM_EXIT_NONE
: vcpu->vc_gueststate.vg_exit_reason;
