Repository navigation
feat(libkernel): wait for pending guest signals in the signal waits - #4138
Conversation
|
Which title stops at |
|
The title IDs, with the NIDs they stop on:
Where each of those comes from, all in the catalog sweep on #1578: To be exact about what is not verified: no run has shown the process dying inside the wait itself. What the sweep records is the NID at the import/next-stop level, which is why #3552 says "not yet observed in a run", and #4561 now tracks the same four calls throwing on I will rebase on #3986 as soon as it lands and keep the pending set consistent with the per-thread mask that replaces the process-wide one. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the title list. Three things before this can land:
Signals.cpp:93-97:MarkPendingdoesfetch_orandnotify_allwithout holdingpending.lock, whileWaitForPendingInSet(:209-223) andpause(:476-484) check the set under that lock and then wait. A signal raised between the check and thewaitis lost and an infinite wait (sigwait,sigwaitinfo, null-timeoutsigtimedwait,pause) hangs forever. Take the lock (or at least lock/unlock it) between thefetch_orand thenotify_all. The 50 ms sleep inGuestSigtimedwait.cpphides this; a test that raises with no delay, repeated many times, would show it.- This builds on the process-wide mask and pending set that #3986 replaces, as @oneandonlydean said. Please rebase on #3986 once it lands and keep pending signals per thread, so
pthread_killto another thread (:396) marks the target's set, not a process-wide one. - It also conflicts with
mainindocs/dev/TechnicalDebt.md.
Before: raise, kill and pthread_kill delivered a signal at once or failed with EINVAL, nothing stayed pending, and pause, sigwait, sigwaitinfo and sigtimedwait called NotImplemented_nid_no_patch. Now: a process-wide pending set holds signals 1 to 128. A signal raised while it is blocked stays pending and is delivered when _sigprocmask unblocks it, taken by sigwait, sigwaitinfo or sigtimedwait, or delivered by pause, which runs its handler on the calling thread and returns -1 with EINTR. pthread_kill to another thread leaves the signal pending for whichever thread waits for it and answers ESRCH for a handle that is not a live guest thread. sigtimedwait follows FreeBSD's sigtimedwait(2): EAGAIN on timeout, EINVAL for a tv_nsec outside 0 to 1000000000 while nothing is pending, and an indefinite wait with a null timeout.
Before: MarkPending set the bit and notified while the waits checked the set under pending.lock, so a signal raised between the check and the wait was lost and sigwait, sigwaitinfo, a null-timeout sigtimedwait and pause hung forever. Now: the bit is set while holding that lock, so a wait either sees it or is notified after it has blocked. GuestSigtimedwait raises with no delay 500 times without the first sleep the other senders use; on the previous code that hung in 2 of 10 runs, and it passes on the new one.
404fe40 to
01874b4
Compare
|
Thanks — all three: 1. The lost wakeup. You were right. 2. The conflict. Rebased onto 3. Per-thread pending. Agreed, and I would rather not guess it here: this still keeps one process-wide pending set, as you said it builds on the mask #3986 replaces. As soon as #3986 lands I will rebase on it and keep the set per thread, so |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
adriadam10
left a comment
There was a problem hiding this comment.
Checked the lost-wakeup fix (bit set under pending.lock, notify after release): guest_sigtimedwait with 500 undelayed raises passes 30 runs in a row, and both guest_signals and guest_sigtimedwait fail without the change. Scoped build and ctest (141 tests) pass on the PR merged onto main. The process-wide pending set stays until #3986 lands, as recorded in docs/dev/debt/libkernel.md; whichever of the two merges second rebases. Merging.
What
pause,sigwait,sigwaitinfoandsigtimedwaitno longer callNotImplemented_nid_no_patch, andpthread_killto another thread no longer throws. They wait on a process-wide set of pending guest signals that covers every number from 1 to 128:_sigprocmaskunblocks it, taken bysigwait,sigwaitinfoorsigtimedwait, or delivered bypause, which runs its handler on the calling thread and returns -1 withEINTR;pthread_killto another thread leaves the signal pending for whichever thread waits for or unblocks it, and answersESRCHfor a handle that is not a live guest thread; it threw for every thread other than the caller before;raiseandkillaccept every number from 1 to 128: an unblocked one whose handler is registered runs its handler on the calling thread, which is what the signals with no host equivalent (SIGIO,SIGALRM,SIGBUS,SIGPIPE) need to be usable at all. An unblocked signal with no host equivalent and no handler still fails withEINVALas before, so no path that used to continue now runs a default action.sigtimedwaitfollows FreeBSD's sigtimedwait(2): it returns the selected signal number,EAGAINwhen the timeout expires with nothing pending in the set,EINVALfor atv_nsecoutside 0 to 1000000000 while nothing is pending, and a null timeout blocks indefinitely.sigwaitreturns 0 with the number through its pointer, as POSIX specifies. The guestsiginfo_tthatsigwaitinfoandsigtimedwaitwrite follows FreeBSD 11'ssys/sys/siginfo.h(80 bytes,si_signoat 0,si_codeat 8,si_pidat 12,si_uidat 16,si_addrat 24,si_valueat 32,si_bandat 40, asserted in the source); onlysi_signo,si_code(SI_USER) andsi_pidare written, andsi_uidstays 0 because no uid is modelled.The limits that remain are recorded in technical debt: the set is process-wide like the mask, the waits are not cancellation points, an unblocked caught signal does not interrupt them with
EINTR, and a pending signal whose disposition is the default action and that has no host equivalent stays pending instead of running that action, which on FreeBSD terminates the process for most signals.guest_sigtimedwaitreplaces itsNotImplementedchecks with the behaviour above, including a signal sent from another thread andpausewoken by one. One line ofguest_signalschanged:pthread_killwith a handle that is not a live guest thread threw and now returnsESRCH, as FreeBSD's does.Overlap: #3986 changes the same file with a per-thread mask and pending set. No open pull request implements these four waits; if #3986 lands first this rebases onto it, keeps its per-thread masks and drops the duplicate part of the pending handling here.
Tested
Linux (Fedora 44 container, GCC 16.2.1, Release,
BUILD_TESTING=ON, bundled dependencies), on this branch rebased ontomainbca7a29d2:guest_signals,guest_sigtimedwait,guest_signal_sets,guest_kill,guest_ioctl_getsid,guest_getitimer,guest_thread_cpu_clock,guest_memoryand everyguest_pthread_*test pass;guest_sigtimedwaitwas run five times in a row.ctest -E "agc_|video_out|lds_|msaa" --timeout 90 -j6: 482 of 482 passed in 7.7 s. The excluded tests need a GPU, and this machine only has lavapipe.python3 tools/check_conventions.py --base origin/main,git diff --checkandpython3 tools/tests/test_progress.pypass.std::mutex,std::condition_variable,std::atomicandstd::chrono, but the Windows build is unchecked by me.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository