Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kern: stack watermark support #1956

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sys/kern/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ phash-gen = { path = "../../build/phash-gen" }
[features]
dump = []
nano = []
stack-watermark = []

[lib]
test = false
Expand Down
50 changes: 47 additions & 3 deletions sys/kern/src/arch/arm_m.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if this whole block should be conditional on the feature flag? I see that update_stack_watermark is a nop if the feature isn't enabled, and the compiler is probably smart enough to eliminate the CURRENT_TASK_PTR access when the function that's called on it doesn't do anything, but... I just kinda don't trust it...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to make as little code conditional as possible, so that it always gets compiled; this is why it's only the body of update_watermark that's cfg'd. The compiler does appear to optimize this load out, fwiw.

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);
Expand Down Expand Up @@ -1174,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| {
Expand Down Expand Up @@ -1210,6 +1225,23 @@ pub unsafe extern "C" fn DefaultHandler() {
ipsr & 0x1FF
};

let current = CURRENT_TASK_PTR.load(Ordering::Relaxed);

// 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();
}

// The first 16 exceptions are architecturally defined; vendor hardware
// interrupts start at 16.
match exception_num {
Expand All @@ -1232,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);

Expand Down Expand Up @@ -1508,7 +1542,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),
Expand Down Expand Up @@ -1635,7 +1674,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,
Expand Down
5 changes: 4 additions & 1 deletion sys/kern/src/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
};

Expand Down
58 changes: 58 additions & 0 deletions sys/kern/src/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ pub struct Task {
/// Pointer to the ROM descriptor used to create this task, so it can be
/// restarted.
descriptor: &'static TaskDesc,

/// Stack watermark tracking support.
///
/// This field is completely missing if the feature is disabled to make that
/// clear to debug tools.
#[cfg(feature = "stack-watermark")]
stack_watermark: StackWatermark,
}

impl Task {
Expand All @@ -69,6 +76,8 @@ impl Task {
notifications: 0,
save: crate::arch::SavedState::default(),
timer: crate::task::TimerState::default(),
#[cfg(feature = "stack-watermark")]
stack_watermark: StackWatermark::default(),
}
}

Expand Down Expand Up @@ -321,9 +330,32 @@ impl Task {
self.notifications = 0;
self.state = TaskState::default();

#[cfg(feature = "stack-watermark")]
{
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);
}

/// 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_watermark.current_low = u32::min(
self.stack_watermark.current_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 {
Expand Down Expand Up @@ -381,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.
Expand Down
Loading