Page MenuHomeFreeBSD

bhyve: terminate waiting loops if thread suspension is requested
ClosedPublic

Authored by kib on Dec 19 2019, 8:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 5:12 AM
Unknown Object (File)
Jan 29 2024, 2:58 PM
Unknown Object (File)
Jan 29 2024, 2:57 PM
Unknown Object (File)
Jan 29 2024, 2:57 PM
Unknown Object (File)
Jan 11 2024, 4:46 PM
Unknown Object (File)
Dec 20 2023, 1:43 AM
Unknown Object (File)
Dec 12 2023, 7:23 AM
Unknown Object (File)
Oct 30 2023, 8:28 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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

sys/amd64/vmm/vmm.c
1283 ↗(On Diff #65821)

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

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.

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.

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.

afedorov 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 marked an inline comment as done.

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

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.

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

Do not translate ERESTART to EINTR.

This revision is now accepted and ready to land.Jan 2 2020, 3:22 PM