WIP: Kernel accounting of task CPU usage#2592
Conversation
|
Oh, and there are a bunch of semi-unrelated changes/refactoring to kernel unsafe code that I should probably factor out into a separate PR, or revert. I was mostly just putzing around while looking at things understanding how scheduling and startup worked in process. @cbiffle you might be interested in these, outside of the context of time accounting. |
|
Misc note, this PR (and #2571 before it) uses the hardware system timer at 1MHz resolution. However, there is practically little downside IMO for this use case to not use the maximum possible speed, 200MHz (the full speed of the APB1 timer clock) for ticking. With a 32-bit timer, this would roll-over every 21.47 seconds, meaning that as long as we perform "timekeeping" every 10.73 seconds, this frequency is suitable. One potential risk is if a debugger causes the core to be halted longer than that, we may observe "weird" time (potentially breaking monotonic guarantees). At 1MHz, this is 200x longer, meaning our max acceptable interruption time is roughly 35.79 minutes. With a 64-bit extended timing range at 200MHz, we would have a full range of 2922 years before encountering a roll-over. It seems acceptable to qualify this as "forever", as it is likely we will not experience that duration of uptime. If we are concerned about this, we could operationally require a reboot every two millennia or so for good measure. This increased precision would allow for fewer rounding errors when calculating time spent in each task. At 1MHz, I'd say our practical resolution is compromised when there is less than 2us between scheduling operations, which is approximately 800 CPU cycles. It is probably not THAT often we are scheduling a task for less than that time, but it does seem at least potentially possible. There is probably some floor to this: whatever our total time spent entering and exiting a task, e.g. one that immediately yields or is interrupted, probably measured in the dozens-to-low-hundreds of cycles. For this, even bumping up to 10(s) of MHz is likely "good enough", even if we don't go all the way up to 200MHz. One downside of not picking 1MHz as "the official timebase" is potential variance in precision between targets. The LPC55 likely can't run a timer all the way up to 200MHz. That being said, the current impl can handle this reasonably. |
labbott
left a comment
labbott
left a comment
There was a problem hiding this comment.
Some of this is good cleanup which might be good to review/merge separately.
| cortex-m-rt = { workspace = true } | ||
| stm32h7 = { workspace = true } | ||
| measurement-handoff = { path = "../../lib/measurement-handoff", optional = true } | ||
| kern = { path = "../../sys/kern" } |
There was a problem hiding this comment.
It's unfortunate to introduce a dep on kern for something in drv since we've been able to have that dep be limited to the core app so far.
| let vtable: *mut PTimeVTable = PTIME_VTABLE.load(Ordering::Acquire); | ||
| let vtable: *const PTimeVTable = vtable.cast_const(); | ||
| unsafe { vtable.as_ref() } |
There was a problem hiding this comment.
Had to go re-read the Rust doc to refresh myself on this but it looks great :)
Also could use // SAFETY comment
| // With init done, set up initial register state etc. | ||
| crate::arch::reinitialize(task); |
There was a problem hiding this comment.
The was originally written to be very very explicit about when were were dealing with init vs uninit code. I think that deserves a bigger comment/explanation
| /// To avoid causing problems, ensure that `task` is a member of the task | ||
| /// table, with memory protection generated by the build system, and that | ||
| /// your access to `task` goes out of scope before next kernel entry. | ||
| pub(crate) unsafe fn switch_to(&self) { |
There was a problem hiding this comment.
semantically, this is the same as the old switch_to but do we need to be concerned about optimizations doing something unexpected here? I may be overly worried here.
2 participants
This is currently "minimum viable experiment" state, it did work while testing, but I was distracted by discovering #2588 while implementing this.