Skip to content

Commit f531a8d

Browse files
zyuiopzyuiop
committed
move sleep state
1 parent c88e434 commit f531a8d

4 files changed

Lines changed: 135 additions & 130 deletions

File tree

‎src/arch/x86_64/kernel/apic.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -886,7 +886,7 @@ pub fn ipi_tlb_flush() {
886886
#[allow(unused_variables)]
887887
pub fn wakeup_core(core_id_to_wakeup: CoreId) {
888888
#[cfg(all(feature = "smp", not(feature = "idle-poll")))]
889-
if core_id_to_wakeup != core_id() && !processor::supports_mwait() && scheduler::core_wake_up(core_id_to_wakeup) {
889+
if core_id_to_wakeup != core_id() && !processor::supports_mwait() && scheduler::sleep_state::core_wake_up(core_id_to_wakeup) {
890890
without_interrupts(|| {
891891
let apic_ids = CPU_LOCAL_APIC_IDS.lock();
892892
let local_apic_id = apic_ids[core_id_to_wakeup as usize];

‎src/arch/x86_64/kernel/interrupts.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ pub(crate) fn enable_and_wait() {
8383
}
8484
} else {
8585
#[cfg(feature = "smp")]
86-
if !scheduler::core_sleep() {
86+
if !scheduler::sleep_state::core_sleep() {
8787
return;
8888
}
8989

‎src/scheduler/mod.rs‎

Lines changed: 3 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ use alloc::sync::Arc;
88
use alloc::vec::Vec;
99
use core::cell::RefCell;
1010
use core::ptr;
11-
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
12-
use core::sync::atomic::AtomicU8;
1311
use core::sync::atomic::{AtomicI32, AtomicU32, Ordering};
1412

1513
use ahash::RandomState;
@@ -32,6 +30,8 @@ use crate::kernel::scheduler::TaskStacks;
3230
use crate::scheduler::task::*;
3331
use crate::{arch, io};
3432

33+
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
34+
pub mod sleep_state;
3535
pub mod task;
3636
pub mod timer_interrupts;
3737

@@ -46,129 +46,6 @@ static WAITING_TASKS: InterruptTicketMutex<BTreeMap<TaskId, VecDeque<TaskHandle>
4646
/// Map between Task ID and TaskHandle
4747
static TASKS: InterruptTicketMutex<BTreeMap<TaskId, TaskHandle>> =
4848
InterruptTicketMutex::new(BTreeMap::new());
49-
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
50-
static CORE_HLT_STATE: RwSpinLock<Vec<CoreState>> = RwSpinLock::new(Vec::new());
51-
52-
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
53-
struct CoreState(AtomicU8);
54-
55-
/// # Race condition prevention
56-
///
57-
/// Two methods matter here: [arch::kernel::interrupts::enable_and_wait] and [arch::kernel::apic::wakeup_core].
58-
/// `enable_and_wait` checks the status, ensuring it is set to 0+setting it to 1, then sleeps.
59-
/// `wakeup_core` checks the status, ensuring it is set to 1+setting it to 0, then issues interrupt.
60-
///
61-
/// A race would happen if `go_to_sleep` returns true, and `wake_up` returns false.
62-
/// The result of these two methods must therefore be atomically determined in a single operation.
63-
/// No ordering should exist in which that condition can happen.
64-
/// Any other variant is okay:
65-
/// - if `go_to_sleep` is false and `wake_up` is true, we have a un-necessary core wakeup (which is okay)
66-
/// - if both are false, the core is not entering sleep and is therefore not woken up
67-
/// - if both are true, the core sleeps and is woken up
68-
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
69-
impl CoreState {
70-
/// The core is currently busy and should not be interrupted for new tasks
71-
const STATUS_ACTIVE: u8 = 0;
72-
73-
/// The core is currently halted and can be interrupted for new tasks
74-
const STATUS_IDLE: u8 = 1;
75-
76-
/// Another core tried to wake up this core but it was already active.
77-
const STATUS_DONT_SLEEP: u8 = 2;
78-
79-
pub fn new() -> Self {
80-
Self(AtomicU8::new(CoreState::STATUS_ACTIVE))
81-
}
82-
83-
#[inline(always)]
84-
pub fn set_active(&self) {
85-
self.0.store(CoreState::STATUS_ACTIVE, Ordering::SeqCst);
86-
}
87-
88-
/// Indicates that this core will go to HLT.
89-
/// This must be called *before* entering a HLT loop, and *with interrupts disabled*.
90-
/// Returns a boolean indicating if the core should enter the HLT loop or not
91-
#[inline(always)]
92-
pub fn go_to_sleep(&self) -> bool {
93-
if self
94-
.0
95-
.compare_exchange(
96-
CoreState::STATUS_ACTIVE,
97-
CoreState::STATUS_IDLE,
98-
Ordering::SeqCst,
99-
Ordering::Relaxed,
100-
)
101-
.is_err()
102-
{
103-
// There is a possible race condition here, because the two atomic operations can be
104-
// interleaved, but this has no effect on the final result: we're not going to sleep
105-
// anyway.
106-
// If the state was already set to ACTIVE, this has no effect.
107-
// Normally, nothing can set the state to IDLE except this method.
108-
// So the race should have no effect.
109-
// IN ANY CASE: as per top-level comments, returning false is the safe default here.
110-
self.set_active();
111-
false
112-
} else {
113-
// We have correctly read status ACTIVE and set STATUS_IDLE. We go to sleep. This is
114-
// safe because:
115-
// - Another `wake_up` could be processing, either BEFORE or AFTER the atomic `swap`.
116-
// * BEFORE:
117-
// We have set the state to STATUS_IDLE, so `swap` will read that value and
118-
// wake_up will return true ==> SAFE.
119-
// * AFTER: **We cannot be here!** Indeed, `swap` has set the status to
120-
// `STATUS_DONT_SLEEP`, so `compare_exchange` is in the `Err` case.
121-
true
122-
}
123-
}
124-
125-
/// Request to wake up this core.
126-
/// Returns a boolean indicating if an interrupt should be sent, or if the core is already active
127-
#[inline(always)]
128-
pub fn wake_up(&self) -> bool {
129-
// Ask the core not to sleep.
130-
// This makes sure that if the two atomic operations become interleaved, the core will
131-
// not go to sleep with us assuming it was running.
132-
let previous_state = self.0.swap(CoreState::STATUS_DONT_SLEEP, Ordering::SeqCst);
133-
134-
// If the core was idle, we can actually wake it up
135-
if previous_state == CoreState::STATUS_IDLE {
136-
// Again, this operation is not necessarily ordered.
137-
// - Another `go_to_sleep` could be processing, either BEFORE or AFTER the atomic op.
138-
// * BEFORE:
139-
// We have set the state to STATUS_DONT_SLEEP, so the atomic op will read that value
140-
// and go_to_sleep will return false. We will send an interrupt which could have
141-
// been avoided, but that's okay.
142-
// * AFTER:
143-
// The value read at the atomic OP does not matter here, in any case we WILL send
144-
// the interrupt and wake the core up.
145-
// IN ANY CASE: as per top-level comments, returning true is the safe default here.
146-
self.set_active();
147-
true
148-
} else {
149-
// We don't wake up the core. This is safe, because:
150-
// - Another `go_to_sleep` could be processing, either BEFORE or AFTER the atomic op.
151-
// * BEFORE:
152-
// We have set the state to STATUS_DONT_SLEEP, so the atomic op will read that value
153-
// and go_to_sleep will return false ==> SAFE.
154-
// * AFTER: **We cannot be here!** Indeed, the atomic operation in `go_to_sleep`
155-
// also sets the state to STATUS_IDLE, so `previous_state` MUST BE STATUS_IDLE.
156-
false
157-
}
158-
}
159-
}
160-
161-
#[inline]
162-
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
163-
pub fn core_sleep() -> bool {
164-
CORE_HLT_STATE.read()[usize::try_from(core_id()).unwrap()].go_to_sleep()
165-
}
166-
167-
#[inline]
168-
#[cfg(all(target_arch = "x86_64", feature = "smp", not(feature = "idle-poll")))]
169-
pub fn core_wake_up(core_id: CoreId) -> bool {
170-
CORE_HLT_STATE.read()[usize::try_from(core_id).unwrap()].wake_up()
171-
}
17249

17350
/// Unique identifier for a core.
17451
pub type CoreId = u32;
@@ -1013,9 +890,7 @@ pub(crate) fn add_current_core() {
1013890
&CoreLocal::get().scheduler_input,
1014891
);
1015892
#[cfg(all(target_arch = "x86_64", not(feature = "idle-poll")))]
1016-
CORE_HLT_STATE
1017-
.write()
1018-
.insert(core_id.try_into().unwrap(), CoreState::new());
893+
sleep_state::install_for_core(core_id);
1019894
}
1020895
}
1021896

‎src/scheduler/sleep_state.rs‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
use alloc::vec::Vec;
2+
use core::sync::atomic::{AtomicU8, Ordering};
3+
4+
use hermit_sync::RwSpinLock;
5+
6+
use crate::core_id;
7+
use crate::scheduler::CoreId;
8+
9+
static CORE_HLT_STATE: RwSpinLock<Vec<SleepState>> = RwSpinLock::new(Vec::new());
10+
11+
struct SleepState(AtomicU8);
12+
13+
/// # Race condition prevention
14+
///
15+
/// Two methods matter here: [crate::arch::interrupts::enable_and_wait] and [crate::arch::wakeup_core].
16+
/// `enable_and_wait` checks the status, ensuring it is set to 0+setting it to 1, then sleeps.
17+
/// `wakeup_core` checks the status, ensuring it is set to 1+setting it to 0, then issues interrupt.
18+
///
19+
/// A race would happen if `go_to_sleep` returns true, and `wake_up` returns false.
20+
/// The result of these two methods must therefore be atomically determined in a single operation.
21+
/// No ordering should exist in which that condition can happen.
22+
/// Any other variant is okay:
23+
/// - if `go_to_sleep` is false and `wake_up` is true, we have a un-necessary core wakeup (which is okay)
24+
/// - if both are false, the core is not entering sleep and is therefore not woken up
25+
/// - if both are true, the core sleeps and is woken up
26+
impl SleepState {
27+
/// The core is currently busy and should not be interrupted for new tasks
28+
const STATUS_ACTIVE: u8 = 0;
29+
30+
/// The core is currently halted and can be interrupted for new tasks
31+
const STATUS_IDLE: u8 = 1;
32+
33+
/// Another core tried to wake up this core but it was already active.
34+
const STATUS_DONT_SLEEP: u8 = 2;
35+
36+
fn new() -> Self {
37+
Self(AtomicU8::new(SleepState::STATUS_ACTIVE))
38+
}
39+
40+
#[inline]
41+
fn set_active(&self) {
42+
self.0.store(SleepState::STATUS_ACTIVE, Ordering::SeqCst);
43+
}
44+
45+
/// Indicates that this core will go to HLT.
46+
/// This must be called *before* entering a HLT loop, and *with interrupts disabled*.
47+
/// Returns a boolean indicating if the core should enter the HLT loop or not
48+
fn go_to_sleep(&self) -> bool {
49+
if self
50+
.0
51+
.compare_exchange(
52+
SleepState::STATUS_ACTIVE,
53+
SleepState::STATUS_IDLE,
54+
Ordering::SeqCst,
55+
Ordering::Relaxed,
56+
)
57+
.is_err()
58+
{
59+
// There is a possible race condition here, because the two atomic operations can be
60+
// interleaved, but this has no effect on the final result: we're not going to sleep
61+
// anyway.
62+
// If the state was already set to ACTIVE, this has no effect.
63+
// Normally, nothing can set the state to IDLE except this method.
64+
// So the race should have no effect.
65+
// IN ANY CASE: as per top-level comments, returning false is the safe default here.
66+
self.set_active();
67+
false
68+
} else {
69+
// We have correctly read status ACTIVE and set STATUS_IDLE. We go to sleep. This is
70+
// safe because:
71+
// - Another `wake_up` could be processing, either BEFORE or AFTER the atomic `swap`.
72+
// * BEFORE:
73+
// We have set the state to STATUS_IDLE, so `swap` will read that value and
74+
// wake_up will return true ==> SAFE.
75+
// * AFTER: **We cannot be here!** Indeed, `swap` has set the status to
76+
// `STATUS_DONT_SLEEP`, so `compare_exchange` is in the `Err` case.
77+
true
78+
}
79+
}
80+
81+
/// Request to wake up this core.
82+
/// Returns a boolean indicating if an interrupt should be sent, or if the core is already active
83+
fn wake_up(&self) -> bool {
84+
// Ask the core not to sleep.
85+
// This makes sure that if the two atomic operations become interleaved, the core will
86+
// not go to sleep with us assuming it was running.
87+
let previous_state = self.0.swap(SleepState::STATUS_DONT_SLEEP, Ordering::SeqCst);
88+
89+
// If the core was idle, we can actually wake it up
90+
if previous_state == SleepState::STATUS_IDLE {
91+
// Again, this operation is not necessarily ordered.
92+
// - Another `go_to_sleep` could be processing, either BEFORE or AFTER the atomic op.
93+
// * BEFORE:
94+
// We have set the state to STATUS_DONT_SLEEP, so the atomic op will read that value
95+
// and go_to_sleep will return false. We will send an interrupt which could have
96+
// been avoided, but that's okay.
97+
// * AFTER:
98+
// The value read at the atomic OP does not matter here, in any case we WILL send
99+
// the interrupt and wake the core up.
100+
// IN ANY CASE: as per top-level comments, returning true is the safe default here.
101+
self.set_active();
102+
true
103+
} else {
104+
// We don't wake up the core. This is safe, because:
105+
// - Another `go_to_sleep` could be processing, either BEFORE or AFTER the atomic op.
106+
// * BEFORE:
107+
// We have set the state to STATUS_DONT_SLEEP, so the atomic op will read that value
108+
// and go_to_sleep will return false ==> SAFE.
109+
// * AFTER: **We cannot be here!** Indeed, the atomic operation in `go_to_sleep`
110+
// also sets the state to STATUS_IDLE, so `previous_state` MUST BE STATUS_IDLE.
111+
false
112+
}
113+
}
114+
}
115+
116+
pub(super) fn install_for_core(core_id: CoreId) {
117+
CORE_HLT_STATE
118+
.write()
119+
.insert(core_id.try_into().unwrap(), SleepState::new());
120+
}
121+
122+
#[inline]
123+
pub fn core_sleep() -> bool {
124+
CORE_HLT_STATE.read()[usize::try_from(core_id()).unwrap()].go_to_sleep()
125+
}
126+
127+
#[inline]
128+
pub fn core_wake_up(core_id: CoreId) -> bool {
129+
CORE_HLT_STATE.read()[usize::try_from(core_id).unwrap()].wake_up()
130+
}

0 commit comments

Comments
 (0)