Skip to content

Commit bf056db

Browse files
Remove check_preempt() from impl_facade!: Nio/Iocp layers handle cooperative yielding
Per maintainer feedback: cooperative preemption should NOT be in the impl_facade! macro layer. The Nio/Iocp syscall layers already call Suspender::suspend_with themselves (e.g., NioSleepSyscall yields via EventLoops::wait_event). Changes: - Remove check_preempt() call from impl_facade! in syscall/windows/mod.rs - Remove pub(crate) fn check_preempt() from monitor.rs - Keep PREEMPT_PENDING two-level safety in do_preempt() to prevent crashing when SuspendThread fires during critical sections (heap allocation, IO). First SuspendThread sets flag and returns; second SuspendThread forces suspension for truly CPU-bound loops. - Update comments to reflect correct architecture Agent-Logs-Url: https://github.com/acl-dev/open-coroutine/sessions/838614fd-23df-4980-ae2a-837b1fe07788 Co-authored-by: loongs-zhang <38336731+loongs-zhang@users.noreply.github.com>
1 parent d1cccd5 commit bf056db

2 files changed

Lines changed: 13 additions & 31 deletions

File tree

core/src/monitor.rs

Lines changed: 13 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -177,10 +177,10 @@ impl Monitor {
177177
}
178178
} else if #[cfg(windows)] {
179179
// Two-level preemption: the first preempt_thread call
180-
// sets a cooperative flag; the second call (next iteration,
181-
// ~1ms later) forces immediate suspension. We do NOT remove
182-
// the node here — MonitorListener::on_state_changed removes
183-
// it when the coroutine actually transitions to Suspend.
180+
// sets a flag via do_preempt; the second call (~1ms
181+
// later) forces suspension for CPU-bound coroutines.
182+
// MonitorListener::on_state_changed removes the node
183+
// when the coroutine transitions to Suspend.
184184
if !Self::preempt_thread(node.thread_id) {
185185
error!(
186186
"Attempt to preempt scheduling for thread:{} failed !",
@@ -503,38 +503,24 @@ std::arch::global_asm!(
503503

504504
// Thread-local flag for two-level preemption on Windows.
505505
// Level 1: SuspendThread fires, do_preempt sets this flag and returns
506-
// without switching coroutines. The hooked syscall (Sleep, etc.)
507-
// calls check_preempt() which yields cooperatively at a safe point.
508-
// Level 2: If the flag is still set on the next SuspendThread (1ms later),
506+
// without switching coroutines — the thread continues executing
507+
// and exits any critical section (heap allocation, IO, etc.).
508+
// If it reaches a hooked syscall, the Nio/Iocp layer will call
509+
// Suspender::suspend_with cooperatively.
510+
// Level 2: If the flag is still set on the next SuspendThread (~1ms later),
509511
// the coroutine is truly CPU-bound with no syscalls — do_preempt
510512
// forces an immediate context switch.
511513
#[cfg(windows)]
512514
std::thread_local! {
513515
static PREEMPT_PENDING: Cell<bool> = const { Cell::new(false) };
514516
}
515517

516-
/// Check if a preemption request is pending and yield cooperatively.
517-
/// Called from the Windows syscall hooks (impl_facade!) after each
518-
/// hooked syscall returns, providing a safe yield point where no
519-
/// thread-local borrows (e.g. test harness stdout capture) are held.
520-
#[cfg(windows)]
521-
pub(crate) fn check_preempt() {
522-
PREEMPT_PENDING.with(|flag| {
523-
if flag.get() {
524-
flag.set(false);
525-
if let Some(suspender) = SchedulableSuspender::current() {
526-
suspender.suspend();
527-
}
528-
}
529-
});
530-
}
531-
532518
#[cfg(windows)]
533519
extern "C" fn do_preempt() {
534520
PREEMPT_PENDING.with(|flag| {
535521
if flag.get() {
536522
// Flag was already set from a previous SuspendThread attempt but the
537-
// coroutine never made a hooked syscall — it is truly CPU-bound.
523+
// coroutine never yielded (no hooked syscalls) — it is truly CPU-bound.
538524
// Force immediate suspension.
539525
flag.set(false);
540526
if let Some(suspender) = SchedulableSuspender::current() {
@@ -543,8 +529,9 @@ extern "C" fn do_preempt() {
543529
} else {
544530
// First attempt: set the flag and return without suspending.
545531
// preempt_asm will restore all registers and return to the original
546-
// code. If the coroutine reaches a hooked syscall, check_preempt()
547-
// will yield cooperatively at a safe point.
532+
// code. This gives the thread time to exit any critical section.
533+
// If the coroutine reaches a hooked syscall, the Nio/Iocp layer
534+
// will yield cooperatively via Suspender::suspend_with.
548535
flag.set(true);
549536
}
550537
});

core/src/syscall/windows/mod.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,11 +95,6 @@ macro_rules! impl_facade {
9595
}
9696
}
9797
$crate::info!("exit syscall {} {:?} {}", syscall, r, saved_errno);
98-
// Cooperative preemption: if the monitor thread requested
99-
// preemption via SuspendThread, yield here at a safe point
100-
// where no thread-local borrows are held.
101-
#[cfg(feature = "preemptive")]
102-
$crate::monitor::check_preempt();
10398
// Restore errno so callers see the correct error.
10499
if let Some(e) = saved_errno.raw_os_error() {
105100
$crate::syscall::set_errno(

0 commit comments

Comments
 (0)