signal: implement signalfd(2)#1439
Conversation
|
Please squash such PRs, which only have a single commit plus a few small correction patches, into one commit. It makes it easier to review, easier to read "git log" later, and we don't get incorrect statements in commit messages that are corrected in the next patch. |
There was a problem hiding this comment.
Pull request overview
Implements signalfd(2) support in OSv so signal-driven event loops can consume signals via a pollable file descriptor (similar to Linux usage with epoll/poll), and wires delivery into kill().
Changes:
- Add
libc/signalfd.ccimplementing a pollablespecial_file-backed signalfd with a global registry and queuedsignalfd_siginfodelivery. - Route
kill()through the signalfd registry first and remove the priorsignalfd()ENOSYS stub. - Add
tst-signalfdand integrate it into the test module/build.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tst-signalfd.cc | Adds coverage for signalfd delivery/read/poll semantics and error cases |
| modules/tests/Makefile | Adds tst-signalfd.so to the test suite build |
| Makefile | Builds and hides signalfd.o in libc |
| libc/signalfd.cc | Implements signalfd fd object, queueing, polling, and delivery hook |
| libc/signal.cc | Hooks kill() to deliver to signalfd and removes the old stub |
| include/osv/signal.hh | Declares the osv_signalfd_deliver() hook used by kill() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <signal.h> | ||
| #include <errno.h> |
| // Copy out as many queued siginfos as fit in the caller's buffer. | ||
| while (!_queue.empty() && | ||
| uio->uio_resid >= (ssize_t)sizeof(signalfd_siginfo)) { | ||
| signalfd_siginfo si = _queue.front(); | ||
| _queue.pop_front(); | ||
| int err = uiomove(&si, sizeof(si), uio); | ||
| if (err) { | ||
| return err; | ||
| } | ||
| } |
| // Create a signalfd watching SIGUSR1 and SIGUSR2. | ||
| sigset_t mask; | ||
| sigemptyset(&mask); | ||
| sigaddset(&mask, SIGUSR1); | ||
| sigaddset(&mask, SIGUSR2); | ||
| int sfd = signalfd(-1, &mask, SFD_NONBLOCK); | ||
| assert(sfd >= 0); |
| // OSv is a single process, so a signalfd watches a set of signals for the whole | ||
| // image. When kill() raises a signal that some signalfd is watching, the | ||
| // signal is queued to those fds (and their pollers woken) instead of running | ||
| // the default action or a handler -- matching Linux, where a signalfd-consumed | ||
| // signal must be blocked and is dequeued through the fd. |
|
Addressed the review and rebuilt the branch cleanly on current master:
Squashed into one coherent commit. Rebuilt: The change was independently reviewed for the read() reordering correctness and, importantly, for lock ordering: |
| if (!mask || (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK))) { | ||
| return libc_error(EINVAL); | ||
| } |
| if (!sfd) { | ||
| return libc_error(EINVAL); | ||
| } | ||
| sfd->set_mask(*mask); | ||
| return fd; |
| WITH_LOCK(registry_lock) { | ||
| for (auto* sfd : registry) { | ||
| if (sfd->watches(signo)) { | ||
| sfd->deliver(si); | ||
| delivered = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting. I was also wonder about that while reading this - whether you need to "return true" after the first match instead of "delivered = true". I don't know. Please investigate what is the correct behavior on Linux.
nyh
left a comment
nyh
left a comment
There was a problem hiding this comment.
Looks good. Copilot found a couple of mistakes (I think copilot is smarter than I am ;-)).
| // signalfd delivery hook (libc/signalfd.cc). Called by kill() when a signal is | ||
| // raised: returns true if a signalfd consumed the signal, in which case the | ||
| // caller must not run the default action or a handler. | ||
| extern "C" bool osv_signalfd_deliver(int signo); |
There was a problem hiding this comment.
Why did this function need to be extern "C"? Do you plan to use it from C code?
| WITH_LOCK(registry_lock) { | ||
| for (auto* sfd : registry) { | ||
| if (sfd->watches(signo)) { | ||
| sfd->deliver(si); | ||
| delivered = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting. I was also wonder about that while reading this - whether you need to "return true" after the first match instead of "delivered = true". I don't know. Please investigate what is the correct behavior on Linux.
Replace the ENOSYS signalfd() stub with a real implementation: create (or update) a file descriptor that a program reads to receive signals as signalfd_siginfo records instead of via a handler. - signal_fd is a special_file holding the watched sigset and a queue of siginfos; read() copies out as many queued records as fit (blocking or EAGAIN under SFD_NONBLOCK), poll() reports POLLIN when the queue is non-empty. read() peeks-copies-then-dequeues so a failed copy-out cannot drop a queued signal. - kill()/the signal-raise path (libc/signal.cc) calls osv_signalfd_deliver(): if a signalfd watches the signal it is queued to a single (arbitrary) watching fd and consumed (no default action / handler), approximating Linux where a blocked signal is consumed by one reader rather than broadcast. The old WARN_STUBBED signalfd() in libc/signal.cc is removed. - SIGKILL and SIGSTOP are filtered from the watched mask (sigdelset) so they keep their uncatchable default behavior, as Linux silently does. - signalfd() updating an existing fd's mask takes registry_lock, the same lock the dispatch path holds while reading the mask, so a mask update cannot race a delivery. The flags argument is only applied on creation (per the man page). Note on Linux fidelity: OSv's signal model is a simplification and does not track a precise per-thread blocked mask here, so a watched signal is consumed unconditionally, and with two signalfds watching the same signal the choice of fd is arbitrary. Sufficient for the common use (block the signals, read them through one fd); documented in the source. Covered by tst-signalfd (blocks the signals first, as a real signalfd user does, so it is meaningful on Linux too).
|
Addressed the round-2 comments:
Squashed into one commit. Rebuilt, tst-signalfd passes. Independently reviewed for the single-consumer semantics and the extern"C"/linkage change. |
What
signalfd(2)was stubbed to returnENOSYS, so event loops that consumesignals through a file descriptor (a common pattern with epoll) could not run.
How
Implemented as a pollable
special_file(modeled oneventfd) backed by aglobal registry of live signalfd objects. OSv is a single process, so a
signalfd watches a set of signals for the whole image.
kill()now checks theregistry first: if any signalfd is watching the raised signal, the signal is
queued to those fds (as a
signalfd_siginfo) and their pollers are woken, andthe signal is consumed -- the default action and any handler are skipped. This
matches Linux, where a signalfd-consumed signal must be blocked and is dequeued
through the fd.
signalfd(-1, mask, flags)creates a new fd;signalfd(fd, mask, flags)updates an existing one's mask.
SFD_CLOEXECandSFD_NONBLOCKare honored.read()returns as many queuedsignalfd_siginfostructures as fit in thebuffer, blocking (or
EAGAINwhen non-blocking) when the queue is empty, andrejecting a buffer smaller than one siginfo with
EINVAL.poll()reportsPOLLINwhen signals are queued.The delivery hook (
osv_signalfd_deliver) is declared in<osv/signal.hh>andcalled from
kill()inlibc/signal.cc; the stub there is removed.Testing
tests/tst-signalfd.cccovers delivery of a watched signal to the fd (insteadof the default poweroff action), the
EAGAIN/poll/queue-order paths, and theEINVALcases. Passes on OSv under KVM (1 and 2 vCPUs).tst-sigwaitandtst-sigactioncontinue to pass, so normal signal delivery is unaffected forsignals no signalfd is watching.
Note
OSv's signal model is thread-directed and minimal; this integrates at the
process-wide
kill()delivery point, which is the right granularity for asingle-address-space unikernel. Signals raised synchronously in-thread (e.g.
faults) are a separate path and not routed to signalfd here.
(Recreated from #1421, which GitHub auto-closed when its branch was rebased onto current master. Same change, rebased and verified on master 3aba46c.)