Repository navigation
signal: don't disable preemption before handling SIGSTOP - #72
Open
greatbridf wants to merge 1 commit into
Open
greatbridf wants to merge 1 commit into
greatbridf wants to merge 1 commit into
Conversation
For some reason, we disabled preemptions before we set the waker and yield. This is prohibited since we can't go to sleep with preemption disabled. After examining the previous commit, I think we used to call Scheduler::schedule() to do context switches, which needs preemptions disabled. When we move on to the new method, we just forgot to change it. This should fix the issue. Fixes: e23c9eb ("runtime: new task sleep-wakeup method and some adaption") Fixes #52 Signed-off-by: greatbridf <greatbridf@icloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stopping a task —
SIGSTOP, orSIGTSTP/SIGTTIN/SIGTTOUtaking their defaultaction — panicked the kernel instead of stopping the task:
This removes the stray
eonix_preempt::disable()that caused it.Fixes #52.
Root cause
SignalList::handleimplemented the default stop action by disabling preemptionand then awaiting a blocking future:
Runtime::block_till_wokenalways returnsPendingon its first poll, so thestackful-future runner reached
assert_preempt_enabled!("Blocking in stackful futures is not allowed.")insrc/kernel/task.rsand panicked. The panic wastherefore deterministic, not a race: it fired on the first stop signal delivered
to any task, regardless of
SMP.This
disable()is a leftover from when context switches went throughScheduler::schedule(), which did require preemption to be off. It was simplymissed when the task sleep/wakeup mechanism was reworked in
e23c9eb1f24c("runtime: new task sleep-wakeup method and some adaption"), andit is actively wrong under the new scheme: a task that blocks must have
preemption enabled.
Fix
Delete the
eonix_preempt::disable()call, and correct the adjacent comment,which claimed
SAFETY: Preempt disabled above.and so no longer described thecode.
Verification
Reproduced with a local headless
expectreproducer (not included in this PR):boot the kernel, start a CPU-bound child, then
kill -STOPit followed bykill -CONT. A CPU-bound target is required, because a task that is alreadyblocked does not take a pending stop signal.
7baa97689d5f--expect bug, SMP=4REPRO bug-observed7baa97689d5f--expect fixed, SMP=4--expect fixed, SMP=4REPRO fixed-observed, 0 panics--expect bug, SMP=4The base panic matches the issue report exactly:
Additional checks:
than a multi-CPU race.
make build MODE=release ARCH=x86_64— pass;ARCH=x86_64 script/test.shsmoke test — pass.
(
echostill responds), so the target is not merely "unpanicked but wedged".Risks / notes
Removing this call makes the stop path reachable for the first time, so the
remaining defects in that path become observable. They are pre-existing and not
introduced here, but they are worth a follow-up:
stop_waker.BlockTillWoken::pollreturnsReadyon any second poll, while
stop_wakeris only cleared by aSIGCONTraise. If another wake completes the "stop" first,
stop_wakerstaysSome, and the next stop signal tripsassert!(old_waker.is_none(), "We should not have a waker here")atsignal.rs:238. The stop can also silently become a no-op.SIGKILLdoes not wake a stopped task.raisehandsSIGCONTtostop_waker, but every other signal —SIGKILLincluded — goes tosignal_waker, which the stop path never installs. A stopped task istherefore not killable until a
SIGCONTarrives, contrary to the"
SIGSTOPcan only be waken up bySIGCONTorSIGKILL" comment retainedabove.
stop_waker: aSIGCONTraised in between is pushed topendingand wakesnobody, leaving the task parked. The window is not instruction-sized, because
the preceding
ProcessList::get().read().awaitcan genuinely suspend.These are not regressions — before this change every stop signal panicked
earlier, at
task.rs:147— but this PR should not be read as claiming thatSIGSTOP/SIGCONTis fully correct. A follow-up should fix the wakerprotocol (re-check
pendingfor a stop-ending signal inside theset_wakerclosure, and treat a spurious
Readyas a re-park rather than a continue).