Repository navigation
[kernel][thread] wake the pinned cpu when a sleeping thread wakes up - #547
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
travisg
left a comment
There was a problem hiding this comment.
Thanks, this looks right to me. On #544 I'd wanted re-pin-while-sleeping left for the per-cpu scheduler rework, but this is reachable under the contract that already landed, not only that deferred case: a thread running on A gets pinned to B by someone else (which only records the pin), then calls thread_sleep() before A reschedules. The timer is armed on A, the handler requeues it there, A can't run it and nothing pokes B. So it's a real hang today.
It also isn't the eager-wakeup pattern I pushed back on: mp_reschedule() drops the local cpu, so a thread pinned where it slept costs a call and no IPI, and unpinned sleepers are untouched. It brings the sleep path in line with resume/unblock/wait-queue wakes, and the scheduler rework on the preempt branch does the same thing in its sleep handler, so this converges with where that's going. When that branch rebases this hunk will fold into its version, but the test should come along, like the #544 one.
A few small things:
docs/threading_and_scheduler.md, CPU Pinning: "kernel/test/thread_tests.ccovers the self-migration and repinning a queued thread" should mention the sleeping case now.kernel/include/kernel/thread.h, the comment abovethread_set_pinned_cpu(): "Blocked or suspended threads only record the new affinity" leaves out sleeping, which the docs bullet includes.
The test is nicely built: without the fix it fails via the join timeout rather than hanging, and the retry covers the sleeper waking before it can be repinned. On a platform with a periodic tick it would pass either way, which is fine.
travisg
left a comment
There was a problem hiding this comment.
LGTM. Ran the boot tests locally (gcc): arm64, x86-64 and riscv64 pass with the change. With just the thread.c hunk reverted, arm64 and x86-64 each fail only test_thread_pin_sleeping (join returns ERR_TIMED_OUT) and the run still completes, so the test does catch it. The nits from the earlier review are fine as a follow-up or folded in here, your call.
A thread can be pinned to another cpu while it sleeps. Its sleep timer still fires on the cpu it fell asleep on, and thread_sleep_handler() only requeued it there, so if the new cpu was idle nothing ever ran the thread. When the thread is pinned to another cpu, send that cpu the reschedule IPI that thread_unblock() and thread_resume() send, and skip the local reschedule, which cannot pick the thread up. Add a test that repins a sleeping thread to an idle cpu, and mention the sleeping case in the scheduler docs and the thread_set_pinned_cpu() comment.
fd3cb2d to
ef70e15
Compare
|
Thanks for the review! Updated:
Re-ran |
A thread can be pinned to another CPU while it sleeps; for a sleeping thread
thread_set_pinned_cpu()only records the CPU. Its sleep timer still fires on the CPU it fell asleep on, andthread_sleep_handler()only requeues it there. Unlikethread_unblock()andthread_resume(), it sends no reschedule IPI, so if the new CPU is idle nothing ever runs the thread.thread_sleep_handler()now callswakeup_cpu_for_thread()and returnsINT_NO_RESCHEDULE, since a local reschedule can't pick the thread up. A thread pinned to this CPU, or unpinned, is handled as before.test_thread_pin_sleepingstarts a thread on another CPU, waits for it to fall asleep, repins it to the test's own CPU and waits inthread_join(), which leaves that CPU idle. Without the fix the join times out; with it the thread runs on its new CPU. If the sleeper wakes before it can be repinned, the test tries again (up to 3 times).docs/threading_and_scheduler.mdand thethread_set_pinned_cpu()comment inthread.hnow cover the sleeping case.Tested with
scripts/run-qemu-boot-tests.py, and by runningut thread_testsfive times per boot on arm64 and x86-64:thread_join()returnedERR_TIMED_OUT)