From dbc220982520b0888f90e1fda342f405af16c81a Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 15 Dec 2024 09:03:58 -0800 Subject: [PATCH 1/3] kern: stack watermark support Each task has grown a new field, `low_stack_pointer`, that tracks the lowest stack pointer value seen so far (because stacks grow down). We update this value on any kernel entry (interrupt, timer, fault, syscall), so while we will miss very brief excursions of stack _between_ syscalls or interrupts, we should still get a useful value. In addition, the field `past_low_stack_pointer` tracks the lowest stack pointer value across _all previous incarnations_ of the task. This way if the task is periodically restarting, we can still get useful stack stats, including at stack overflow. --- sys/kern/Cargo.toml | 1 + sys/kern/src/arch/arm_m.rs | 34 ++++++++++++++++++++++++++++++++-- sys/kern/src/syscalls.rs | 5 ++++- sys/kern/src/task.rs | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 3 deletions(-) diff --git a/sys/kern/Cargo.toml b/sys/kern/Cargo.toml index 0e50c8754..00460d667 100644 --- a/sys/kern/Cargo.toml +++ b/sys/kern/Cargo.toml @@ -36,6 +36,7 @@ phash-gen = { path = "../../build/phash-gen" } [features] dump = [] nano = [] +stack-watermark = [] [lib] test = false diff --git a/sys/kern/src/arch/arm_m.rs b/sys/kern/src/arch/arm_m.rs index 1168d4df0..dac9a5027 100644 --- a/sys/kern/src/arch/arm_m.rs +++ b/sys/kern/src/arch/arm_m.rs @@ -1039,6 +1039,17 @@ static TICKS: [AtomicU32; 2] = { #[no_mangle] pub unsafe extern "C" fn SysTick() { crate::profiling::event_timer_isr_enter(); + + // We don't need the current task pointer in this routine, but we'll access + // it briefly to update the stack watermark: + { + let current = CURRENT_TASK_PTR.load(Ordering::Relaxed); + // Safety: `CURRENT_TASK_PTR` is valid once the kernel is started, and + // this interrupt is only enabled once the kernel is started. + let t = unsafe { &mut *current }; + t.update_stack_watermark(); + } + with_task_table(|tasks| { // Load the time before this tick event. let t0 = TICKS[0].load(Ordering::Relaxed); @@ -1210,6 +1221,15 @@ pub unsafe extern "C" fn DefaultHandler() { ipsr & 0x1FF }; + let current = CURRENT_TASK_PTR.load(Ordering::Relaxed); + uassert!(!current.is_null()); // irq before kernel started? + + let current_prio = { + let t = unsafe { &mut *current }; + t.update_stack_watermark(); + t.priority() + }; + // The first 16 exceptions are architecturally defined; vendor hardware // interrupts start at 16. match exception_num { @@ -1508,7 +1528,12 @@ unsafe extern "C" fn handle_fault(task: *mut task::Task) { // assembly fault handler to pass us a legitimate one. We use it // immediately and discard it because otherwise it would alias the task // table below. - let t = unsafe { &(*task) }; + let t = unsafe { &mut (*task) }; + // Take the opportunity to update the stack watermark. This is + // technically wasted effort if the fault is in the kernel, but it's + // still nice to keep it updated -- and it's critical if the fault is in + // the task! + t.update_stack_watermark(); ( t.save().exc_return & 0b1000 != 0, usize::from(t.descriptor().index), @@ -1635,7 +1660,12 @@ unsafe extern "C" fn handle_fault( // of dereferencing it, as it would otherwise alias the task table obtained // later. let (exc_return, psp, idx) = unsafe { - let t = &(*task); + let t = &mut (*task); + // Take the opportunity to update the stack watermark. This is + // technically wasted effort if the fault is in the kernel, but it's + // still nice to keep it updated -- and it's critical if the fault is in + // the task! + t.update_stack_watermark(); ( t.save().exc_return, t.save().psp, diff --git a/sys/kern/src/syscalls.rs b/sys/kern/src/syscalls.rs index 10e4646ab..194b35bce 100644 --- a/sys/kern/src/syscalls.rs +++ b/sys/kern/src/syscalls.rs @@ -70,7 +70,10 @@ pub unsafe extern "C" fn syscall_entry(nr: u32, task: *mut Task) { let idx = { // Safety: we're trusting the interrupt entry routine to pass us a valid // task pointer. - let t = unsafe { &*task }; + let t = unsafe { &mut *task }; + + t.update_stack_watermark(); + usize::from(t.descriptor().index) }; diff --git a/sys/kern/src/task.rs b/sys/kern/src/task.rs index 31095b4b7..b151adbe9 100644 --- a/sys/kern/src/task.rs +++ b/sys/kern/src/task.rs @@ -49,6 +49,20 @@ pub struct Task { /// Pointer to the ROM descriptor used to create this task, so it can be /// restarted. descriptor: &'static TaskDesc, + + /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on + /// any kernel entry for this instance of this task. + /// + /// Initialized to `u32::MAX` if the task has not yet run. + #[cfg(feature = "stack-watermark")] + stack_pointer_low: u32, + + /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on + /// any kernel entry across *any* instance of this task. + /// + /// Initialized to `u32::MAX` if the task has not yet run. + #[cfg(feature = "stack-watermark")] + past_stack_pointer_low: u32, } impl Task { @@ -69,6 +83,10 @@ impl Task { notifications: 0, save: crate::arch::SavedState::default(), timer: crate::task::TimerState::default(), + #[cfg(feature = "stack-watermark")] + stack_pointer_low: u32::MAX, + #[cfg(feature = "stack-watermark")] + past_stack_pointer_low: u32::MAX, } } @@ -321,9 +339,28 @@ impl Task { self.notifications = 0; self.state = TaskState::default(); + #[cfg(feature = "stack-watermark")] + { + self.past_stack_pointer_low = + u32::min(self.past_stack_pointer_low, self.stack_pointer_low); + self.stack_pointer_low = u32::MAX; + } + crate::arch::reinitialize(self); } + /// Updates the task's stack watermark stats, if enabled. + /// + /// If not enabled, this does nothing, so it should be safe to call freely + /// without checking for the feature. + pub fn update_stack_watermark(&mut self) { + #[cfg(feature = "stack-watermark")] + { + self.stack_pointer_low = + u32::min(self.stack_pointer_low, self.save().stack_pointer()); + } + } + /// Returns a reference to the `TaskDesc` that was used to initially create /// this task. pub fn descriptor(&self) -> &'static TaskDesc { From 28d4d40e7769d3dc665c2271fe6d4ac5b500bfa9 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 17 Dec 2024 11:10:56 -0800 Subject: [PATCH 2/3] SQUASH fix clippy --- sys/kern/src/arch/arm_m.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sys/kern/src/arch/arm_m.rs b/sys/kern/src/arch/arm_m.rs index dac9a5027..21443f1c3 100644 --- a/sys/kern/src/arch/arm_m.rs +++ b/sys/kern/src/arch/arm_m.rs @@ -1224,11 +1224,10 @@ pub unsafe extern "C" fn DefaultHandler() { let current = CURRENT_TASK_PTR.load(Ordering::Relaxed); uassert!(!current.is_null()); // irq before kernel started? - let current_prio = { + { let t = unsafe { &mut *current }; t.update_stack_watermark(); - t.priority() - }; + } // The first 16 exceptions are architecturally defined; vendor hardware // interrupts start at 16. From 5b7b031a91af56f8fec3fb0ecefbcc1e341aeff6 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 16 Jan 2025 12:20:47 -0800 Subject: [PATCH 3/3] SQUASH: changes requested in code review --- sys/kern/src/arch/arm_m.rs | 23 ++++++++++++--- sys/kern/src/task.rs | 59 ++++++++++++++++++++++++++------------ 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/sys/kern/src/arch/arm_m.rs b/sys/kern/src/arch/arm_m.rs index 21443f1c3..8019d1a43 100644 --- a/sys/kern/src/arch/arm_m.rs +++ b/sys/kern/src/arch/arm_m.rs @@ -1185,10 +1185,14 @@ unsafe extern "C" fn pendsv_entry() { crate::profiling::event_secondary_syscall_enter(); let current = CURRENT_TASK_PTR.load(Ordering::Relaxed); + // We're being slightly pedantic about this, since it's straightforward (if + // weird) for pre-kernel main to trigger a PendSV, and it's not possible to + // disable this interrupt source. uassert!(!current.is_null()); // irq before kernel started? // Safety: we're dereferencing the current task pointer, which we're - // trusting the rest of this module to maintain correctly. + // trusting the rest of this module to maintain correctly (and we've just + // checked that the kernel has started). let current = usize::from(unsafe { (*current).descriptor().index }); with_task_table(|tasks| { @@ -1222,10 +1226,19 @@ pub unsafe extern "C" fn DefaultHandler() { }; let current = CURRENT_TASK_PTR.load(Ordering::Relaxed); - uassert!(!current.is_null()); // irq before kernel started? - { - let t = unsafe { &mut *current }; + // Because this handler is reachable in response to an NMI, it's possible to + // get here before the kernel has started. So before dereferencing the task + // pointer, check for NULL (the state at reset). If we manage to get here + // before the kernel has started, we'll skip the stack watermark update and + // hit a panic below. + + // Safety: this dereferences the pointer only if it isn't NULL. If it + // isn't NULL, that means we've initialized it elsewhere in this module + // and it's a valid (but aliased) pointer to a task. We can safely + // dereference it as long as we throw it away before hitting + // `with_task_table` below. + if let Some(t) = unsafe { current.as_mut() } { t.update_stack_watermark(); } @@ -1251,6 +1264,8 @@ pub unsafe extern "C" fn DefaultHandler() { .get(abi::InterruptNum(irq_num)) .unwrap_or_else(|| panic!("unhandled IRQ {irq_num}")); + // with_task_table will panic if the kernel has not yet been + // started. This is good. let switch = with_task_table(|tasks| { disable_irq(irq_num, false); diff --git a/sys/kern/src/task.rs b/sys/kern/src/task.rs index b151adbe9..77b45ef5a 100644 --- a/sys/kern/src/task.rs +++ b/sys/kern/src/task.rs @@ -50,19 +50,12 @@ pub struct Task { /// restarted. descriptor: &'static TaskDesc, - /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on - /// any kernel entry for this instance of this task. + /// Stack watermark tracking support. /// - /// Initialized to `u32::MAX` if the task has not yet run. + /// This field is completely missing if the feature is disabled to make that + /// clear to debug tools. #[cfg(feature = "stack-watermark")] - stack_pointer_low: u32, - - /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on - /// any kernel entry across *any* instance of this task. - /// - /// Initialized to `u32::MAX` if the task has not yet run. - #[cfg(feature = "stack-watermark")] - past_stack_pointer_low: u32, + stack_watermark: StackWatermark, } impl Task { @@ -84,9 +77,7 @@ impl Task { save: crate::arch::SavedState::default(), timer: crate::task::TimerState::default(), #[cfg(feature = "stack-watermark")] - stack_pointer_low: u32::MAX, - #[cfg(feature = "stack-watermark")] - past_stack_pointer_low: u32::MAX, + stack_watermark: StackWatermark::default(), } } @@ -341,9 +332,11 @@ impl Task { #[cfg(feature = "stack-watermark")] { - self.past_stack_pointer_low = - u32::min(self.past_stack_pointer_low, self.stack_pointer_low); - self.stack_pointer_low = u32::MAX; + self.stack_watermark.past_low = u32::min( + self.stack_watermark.past_low, + self.stack_watermark.current_low, + ); + self.stack_watermark.current_low = u32::MAX; } crate::arch::reinitialize(self); @@ -356,8 +349,10 @@ impl Task { pub fn update_stack_watermark(&mut self) { #[cfg(feature = "stack-watermark")] { - self.stack_pointer_low = - u32::min(self.stack_pointer_low, self.save().stack_pointer()); + self.stack_watermark.current_low = u32::min( + self.stack_watermark.current_low, + self.save().stack_pointer(), + ); } } @@ -418,6 +413,32 @@ impl Task { } } +#[cfg(feature = "stack-watermark")] +#[derive(Copy, Clone, Debug)] +struct StackWatermark { + /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on + /// any kernel entry for this instance of this task. + /// + /// Initialized to `u32::MAX` if the task has not yet run. + current_low: u32, + + /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on + /// any kernel entry across *any* instance of this task. + /// + /// Initialized to `u32::MAX` if the task has not yet run. + past_low: u32, +} + +#[cfg(feature = "stack-watermark")] +impl Default for StackWatermark { + fn default() -> Self { + Self { + current_low: u32::MAX, + past_low: u32::MAX, + } + } +} + /// Interface that must be implemented by the `arch::SavedState` type. This /// gives architecture-independent access to task state for the rest of the /// kernel.