bhyve: terminate waiting loops if thread suspension is requested

[kib]

Summary

The thread_susp_check() is a hack. I will redo it properly, and something similar was already needed for D12773.

PR: 242724

[markj]

sys/amd64/vmm/vmm.c

1283 ↗

(On Diff #65821)

Why can we not suspend here? If we bail at this point the rendezvous state may be inconsistent.

[kib]

sys/amd64/vmm/vmm.c

1283 ↗

(On Diff #65821)

You mean to replace the sleeping arg to true, just at this spot ?

[markj]

sys/amd64/vmm/vmm.c

1283 ↗

(On Diff #65821)

Right. Otherwise I don't really see how this can work following resume, e.g., after the debugger attaches. I am not sure about the other calls yet.

[kib]

sys/amd64/vmm/vmm.c

1283 ↗

(On Diff #65821)

I believe for other places it is fine and even required to make the condition to propagate to kernel/user boundary.

[kib]

Enable suspend during rendezvous handling.

[pmooney_pfmooney.com]

We had similar issues regarding vm_smp_rendezvous() and thread interruption in SmartOS. The easiest path forward for us was to totally eliminate the need for vm_smp_rendezvous(), which has been floated for upstreaming in D20389.

[afedorov]

sys/amd64/vmm/vmm.c

1277 ↗

(On Diff #65865)

It seems that mtx_sleep() cannot be interrupted by signals. Can we wake up it periodically to check pending signals?

[afedorov]

I catch this KASSERT (https://svnweb.freebsd.org/base/head/sys/amd64/vmm/vmm_dev.c?view=markup#l799) when trying to attach debugger to bhyve process.

[kib]

Handle ERESTART from thread_check_susp().
Make sleep in the rendezvous time-bound.

[afedorov]

sys/amd64/vmm/vmm.c

1371 ↗

(On Diff #65954)

Sending SIGSTOP/SIGCONT or gdb attach/detach to bhyve process leads to it's termination. In these cases thread_check_susp(td, false) return an ERESTART error to user space which doesn't handled. I think, we must replace the sleeping arg to true as it's done in vm_handle_randezvous(). I tested SIGSTOP/SIGCONT and GDB with sleeping arg = true and it's work fine.

1543 ↗

(On Diff #65954)

I think the same as above.

[kib]

sys/amd64/vmm/vmm.c

1371 ↗

(On Diff #65954)

Well, I do not allow ERESTART to reach userspace, instead EINTR is returned and this probably scares the userspace bhyve. Point is that the state of vCPU is not quite defined after we broke out of the loop due to check_susp().

Also I do not want to allow sleep inside these handlers.

Can you try to remove lines 779-804 instead and see if it still works for you ?

sys/amd64/vmm/vmm_dev.c

804 ↗

(On Diff #65954)

Remove these lines.

[afedorov]

sys/amd64/vmm/vmm.c

1371 ↗

(On Diff #65954)

Oops, of course you are right, EINTR is returned to user space.

sys/amd64/vmm/vmm_dev.c

804 ↗

(On Diff #65954)

Ok, I remove these lines (KASSERT too) and it's works. SIGSTOP/SIGCONT, gdb -p <pid> attach/detach

[kib]

Do not translate ERESTART to EINTR.