Page MenuHomeFreeBSD

bhyve: terminate waiting loops if thread suspension is requested
ClosedPublic

Authored by kib on Dec 19 2019, 8:57 PM.

Details

Summary

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

PR: 242724

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Dec 19 2019, 8:57 PM
kib edited the summary of this revision. (Show Details)Dec 19 2019, 8:58 PM
markj added a subscriber: markj.Dec 20 2019, 4:28 PM
markj added inline comments.
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 added inline comments.Dec 20 2019, 5:23 PM
sys/amd64/vmm/vmm.c
1283 ↗(On Diff #65821)

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

markj added inline comments.Dec 20 2019, 5:26 PM
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 added inline comments.Dec 20 2019, 5:29 PM
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 updated this revision to Diff 65865.Dec 20 2019, 5:38 PM

Enable suspend during rendezvous handling.

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.

aleksandr.fedorov_itglobal.com added inline comments.
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?

kib updated this revision to Diff 65954.Dec 24 2019, 2:08 PM
kib marked an inline comment as done.

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

kib added a reviewer: markj.Dec 24 2019, 2:09 PM
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 added inline comments.Dec 25 2019, 12:55 PM
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.

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 updated this revision to Diff 65977.Dec 25 2019, 2:29 PM

Do not translate ERESTART to EINTR.

markj accepted this revision as: markj.Jan 2 2020, 3:22 PM
This revision is now accepted and ready to land.Jan 2 2020, 3:22 PM
This revision was automatically updated to reflect the committed changes.