Repository navigation
[kernel][thread] migrate running thread immediately in thread_set_pinned_cpu - #544
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
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.
|
The issue I have is that it's much too aggressive about waking up cores in the case where everything is fine. Seems like the right strategy is to hard migrate the thread the moment the pinning happens right then, so that the constraint that when it leaves the thread_set_pinned() or whatevcer the function is that the thread has been moved. Then it avoids having to sprinkle this code around. |
|
Good call! Updated |
travisg
left a comment
There was a problem hiding this comment.
Reviewed the two commits in aggregate. This is the right shape: pinning the calling thread migrates it before the call returns, which is the contract I want, it adds nothing to the normal scheduling path, and it fixes a real hang. I reproduced the x86_feature_tests / pin_to_cpu(0) stall on master with do-qemux86 -6 -s 16 (TCG): 2 of 8 runs hang at test_cpuid_cache_matches_live [RUNNING], 0 of 8 with this branch, and test_thread_pin_cpu passes every run.
For reference, Zircon's Scheduler::SetCpuAffinity never moves the thread itself: it stores the mask and reschedules whichever cpu holds the thread, and the reschedule path (one chokepoint) does the move and IPIs the target. That is also how the per-cpu run queue scheduler on wip/preempt-disable handles it, and that work will replace this mechanism fairly soon, so I'm keeping the ask here small: the contract, the call sites and the test are what carry over.
Known gaps I'm deliberately not asking you to close here, since the per-cpu scheduler already handles them: pinning a thread that is running on another cpu does nothing until that cpu next reschedules and doesn't wake the pinned cpu (master's status quo), and a thread re-pinned while sleeping wakes onto the old cpu's queue. Please just state the first one in the header comment so the contract is explicit.
A few asserts and the comment inline, then I'm happy to take it.
c28332b to
3be6d5f
Compare
Pinning the current thread to another CPU must wake that CPU and migrate before returning. Otherwise the thread can be requeued for an idle target that never receives a reschedule IPI, hanging callers such as the x86 feature tests. Move this work into thread_set_pinned_cpu() under thread_lock, rather than adding wakeups to the normal scheduling paths. Drop the now-redundant yields at its call sites and document the locking and thread-state contract in thread.h. Require interrupts enabled and an active target for self-migration, with debug assertions instead of silently skipping the migration. Once the thread resumes, check its current affinity, including the unpinned case: another caller may have changed the affinity while it was queued. Keep the existing limitations for threads running on another CPU or blocked unchanged. Record the initial unpinned state directly when initializing a thread, before the locking setter can be used. Test synchronous pinning across active CPUs and both repinning and unpinning during a pending migration. A higher-priority worker occupies the active destination so the affinity-update test is deterministic and does not rely on pinning to an inactive CPU. Use unsigned shifts for the run-queue bitmap. The regression uses priority 31 and exposes the existing signed 1 << 31 undefined behavior under UBSAN; update both insertion paths and both queue-selection masks.
3be6d5f to
1605420
Compare
travisg
left a comment
There was a problem hiding this comment.
LGTM, thanks. That covers everything from the last round: the asserts in the self-migrate path, the post-resched check that tolerates a concurrent unpin, the contract comment in thread.h, and the single commit. The extra queued-update test and the 1U << bitmap fix are welcome too.
Re-tested the new tip locally under TCG (default gcc toolchain, local.mk ignored):
- x86-64
-smp 16,ut x86_feature_tests; ut thread_tests: 8/8 pass. master hung 2/8 intest_cpuid_cache_matches_liveon the same setup. - arm64 and riscv64 (supervisor)
-smp 4,ut thread_tests: 4/4 each, including both new pin tests.

When
thread_set_pinned_cpu()pins the currently running thread to a different CPU, migrate it synchronously underthread_lock: enqueue it, wake the target, and reschedule. This fixes the x86 feature-test stall without adding wakeups to the normal scheduling paths, and removes the now-redundantthread_yield()calls at the pinning call sites.Self-migration requires interrupts enabled and an active target CPU, enforced by debug assertions rather than silently skipping migration. After rescheduling, check the current affinity under the lock, allowing another caller to have repinned or unpinned the queued thread.
The header documents the locking and thread-state contract. The existing limitations for a thread running on another CPU (no forced reschedule or destination wakeup) and blocked/suspended threads are intentionally unchanged.
Tests retain synchronous pinning across active CPUs and cover both repinning and unpinning during a pending migration. The latter uses a higher-priority worker to occupy an active destination CPU; it does not pin to an inactive CPU and only skips when fewer than two CPUs are active. Restoring the original stale-CPU assertion makes this regression fail deterministically on ARM64.
Earlier review validation, before the four-line bitmap correction, used Clang/LLD,
DEBUG=2,WERROR=1,LK_HEAP_IMPLEMENTATION=dlmalloc, andIGNORE_LOCAL_MK=1:ut all57/57 cases passed, including both affinity-update paths.ut all53/53 cases passed, including both affinity-update paths.WITH_SMP=0, 1 CPU:ut all57/57 cases passed.The highest-priority blocker exposed existing signed
1 << 31undefined behavior in the scheduler. Both run-queue insertion paths and both queue-selection masks now use1Ushifts; the regression retains priority 31.Final-tree validation using the same GCC 15.2.0 toolchains as CI,
UBSAN=1,IGNORE_LOCAL_MK=1, and 4 CPUs:ut all57/57, zero UBSAN reports.ut all53/53 each, zero UBSAN reports.Squashed into one commit describing the aggregate change; its source tree matches the tested tree exactly.