Replace `static mut`s with new `CoreLocal` construct #3945

alevy · 2024-03-31T20:44:39Z

Pull Request Overview

This pull request fixes #3841 by replacing most global static mut throughout the kernel, chips, capsules, and board crates with a new construct called CoreLocal.

CoreLocal is similar in principle to Rust std's LocalKey (obtained from the thread_local! macro). It allows access to its internally stored data within a closure passed to with, which obtains a temporary shared reference. It is marked Sync, under the assumption that it is accessed in a thread-safe way---e.g. one copy per CPU core, as the name implies---allowing it to be stored in a global (non-mut) static.

This implementation only includes support for single-core, single-threaded, platforms (as all of Tock for the moment) via the unsafe new_single_core constructor, though extending to multiple cores should be relatively simple.

In order to support mutation (which is ostensibly the purpose of static mut in many cases), users should use some form of single-threaded mutual exclusion, such as a MapCell, TakeCell, Cell, etc.

This PR converts most uses of static mut to use this new construct, thus avoiding various annoying warnings and probably some real unsoundness. This comes at virtually no cost to code size, but some minor cost to memory (around 100 bytes on the platforms I looked at).

Testing Strategy

Still need to test. This changes some subtle parts of the kernel (deferred calls, the schedulers, process implementation) in ways that, if I didn't take enough care to be precise about the logic, could likely result in bugs (though probably not safety bugs).

TODO or Help Wanted

Finish porting other boards. I think this is just menial and straight forward, but tbd
Port some chip-specific drivers that have more subtle uses of static mut

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

alevy · 2024-04-01T01:44:11Z

@lschuermann and I discussed this, so summarizing the feedback and take aways here:

One potential issue is that we technically have additional threads in the ISRs.
Those really really shouldn't touch the same static muts (now CoreLocals), but we don't have a great way of enforcing that
This probably comes down to how we articulate safety for the constructor for CoreLocal or by adding unsafe to one or more methods.

Ideally, we need to do at least one of:

make it impossible or very hard to accidentally call the offending method from an ISR context, e.g., by limiting the state that ISRs can access (for instance, putting them in a separate crate, although that's not bulletproof)
make the constructor itself unsafe.
make the method that accesses it (with) unsafe.

The current implementation in this PR makes the constructor unsafe, but does not yet explain why it is unsafe, and what need to be ensured (e.g. that it is private to the module, and that it is only accessible by an ISR).

alevy · 2024-04-01T01:46:48Z

Porting a few instances of static mut to use CoreLocal may require changes to the logic in chip-specific and maybe capsule drivers (so far this has been the case for NRF52's BLE implementation). So we will need some scrutiny of these changes that are probably beyond what I will reasonably do while I'm porting.

hudson-ayers · 2024-04-01T06:03:40Z

boards/nordic/nrf52840dk/src/main.rs

+    io::DEBUG_INFO.with(|debug_info| debug_info.put(io::DebugInfo {
+	chip,
+	processes,
+	process_printer,
+    }));


This passes references to these to a potential future panic "thread" while other references to them live on and continue to be used.

Do we have a good sense for whether this is safe? I agree that the old approach is probably unsafe, so this is not necessarily making things worse. But it is not obvious to me that this is safe, and if it is not I sort of think we shouldn't make it look like it may be.

The fundamental danger with this infrastructure is that we're providing a reference of T: ?Sync in a Sync wrapper, and I hardly think that chip or processes are Sync. So strictly speaking this is probably not sound as would be required by Rust.

This is a hard problem to solve, and while the introduction of CoreLocal does not change our premise and actual safety guarantees here, it's properties around Sync are what's problematic... In fact, I never thought of panics as being virtually a different thread (which they definitely are!), but only ISRs.

I think that this here is symptomatic of a larger body of unsoundness in Tock when it comes to reasoning about which state can be assumed to be valid in these non-single-thread-synchronous circumstances. A panic may occur precisely because an invariant in some struct reachable through those reference is violated, and Rust doesn't give us particularly many guarantees around their validity beyond that point. So, it's really not only that chip and processes are !Sync, but that they may not be valid at all in a panic context. An unergonomic, expensive and ugly fix is to move all state required in the panic handler into some "atomic" statics (perhaps wrapped in a CoreLocal), that are Sync, have a Cell-like interface, can only wrap datatypes that are guaranteed to be updated atomically w.r.t. a single hardware thread, and are unconditionally valid.

In short, I don't think we can or should fix this in this PR.

Panic is not in a different thread

I hardly think that chip or processes are Sync

Why? Multiple points of entry does not mean they are multiple threads of execution. They are not multiple threads of execution and that is pretty important to the Tock kernel functioning correctly generally.

There are exactly two kinds of software things that run concurrently with the kernel:

Actual Processes, which do not have access to kernel memory

ISRs which are specifically designed to do almost nothing except switch back to the kernel thread and retain the interrupt flag. They really should not touch any memory shared with the broader kernel.

Everything else is single threaded. If it is not single threaded, it is a bug.

I hardly think that chip or processes are Sync

@alevy Aside from your actual comment text which I agree with, the aforementioned variables chip and process won't be sync because at least, as far as I know, they all contain some form of core::cell::UnsafeCell that is !Sync.

And, as you pointed out below, currently panic handlers are exactly invoked as part of ISRs, will access precisely this !Sync state.

Oh... I retract! You're of course right. I totally misunderstood that you were referring to the variables rather than the crates/modules. Sorry!

lschuermann · 2024-04-01T14:13:25Z

Agree with @alevy's summary, just to illustrate the point of why we reason about ISRs / panic contexts at all:

The fundamental assumption in this infrastructure is that, due to Tock's single-threaded nature, deriving Sync on a wrapper around a T: ?Sync is sound. However, we can violate this assumption through use of this infrastructure in ISR or (as pointed out by @hudson-ayers) panic contexts, which behave as virtually separate threads, bringing concerns about reentrancy with them. Even if we find this acceptable as it's not strictly worse than what we're doing right now anyways, it's still important that we strive to remove these soundness issues in the future. This is exactly where the proposals around either strictly separating out state that is accessible in these "threaded" contexts and/or making parts of this infrastructure unsafe, as summarized by @alevy above, come in.

bradjc

Need documentation.

bradjc · 2024-04-01T14:34:23Z

kernel/src/core_local.rs

This should go in kernel/utilities.

bradjc · 2024-04-01T14:38:57Z

kernel/src/core_local.rs

+}
+
+impl<T> CoreLocal<T> {
+    pub fn with<F, R>(&self, f: F) -> R where F: FnOnce(&T) -> R {


Is with standard in some context? I would expect it to be flipped ie closure.with(static_mut) (although I know that doesn't make sense). .access()?

Yes, it's how thread_local works in Rust's std

bradjc · 2024-04-01T14:40:12Z

kernel/src/core_local.rs

+impl<T> CoreLocal<T> {
+    pub const unsafe fn new_single_core(val: T) -> Self {
+	CoreLocal(UnsafeCell::new(val))
+    }
+}


Suggested change

impl<T> CoreLocal<T> {

pub const unsafe fn new_single_core(val: T) -> Self {

CoreLocal(UnsafeCell::new(val))

}

}

impl<T> CoreLocalSingleCore<T> {

pub const unsafe fn new(val: T) -> Self {

CoreLocal(UnsafeCell::new(val))

}

}

?

Otherwise this reads like it is creating a CPU core.

That's not how I read it. A new_<something>() in Rust generally describes creating a particular variant of a thing (in this case, creating a single-core variant of a CoreLocal).

I agree that new_X is a variant, but it's almost always an adjective (eg unchecked) not a noun (core).

https://doc.rust-lang.org/std/?search=new_

There seem to be exceptions for creating a slice.

Maybe this should be new_single? Although I don't see the need for the modifier if there is only one new() method.

bradjc · 2024-04-01T14:41:22Z

kernel/src/kernel.rs

@@ -41,10 +46,43 @@ use crate::utilities::cells::NumericCellExt;
 /// is less than this threshold.
 pub(crate) const MIN_QUANTA_THRESHOLD_US: u32 = 500;

+#[derive(Copy, Clone)]
+pub struct StaticSlice<T> {


A [u8] is not Sized, so can't be stored in a bunch of places. In Rust std statics (like thread_local) end up using Box or Vec. StaticSlice is basically the a fixed-sized Vec (it has a fixed length, but that length is stored dynamically), so it is Sized and can be stored in statics directly, like a fixed-sized array.

I'm having trouble seeing the semantic difference between a StaticSlice<T> and a &'static mut [T].

There should be no semantic difference, only that [T] is not Sized so cannot be declared in a context that needs Sized (such as a a static variable), and we can't create a &'static mut [T] statically.

Ah, I see. The issue is not Sized but that mut references are not allowed in a static item's type. Maybe a different name and/or a link to rust-lang/rust#57349 would make this more clear?

Some other improvement ideas:

StaticSlice doesn't actually need to be slice-specific. You can make it generic for any type and let the user choose whether to store a slice in it.

You can store a &'static mut T instead of a raw pointer to avoid unsafe in Deref and DerefMut.

Demonstration: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8de22b70edcbedaffccc382fd35ddd11

bradjc · 2024-04-01T14:42:14Z

kernel/src/process_checker/basic.rs

@@ -83,6 +83,7 @@ impl<'a> AppCredentialsPolicy<'a> for AppCheckerSimulated<'a> {
    }
 }

+#[allow(dead_code)]


It's a warning we get on newer compilers that for some reason maybe we weren't getting before? Either way, the code is never used and should probably actually be removed, but I didn't want to mess with that stuff in this PR. (Document in the changing commit, but I know that's somewhat hidden)

I think it should be pub.

bradjc · 2024-04-01T14:43:02Z

kernel/src/grant.rs

+    processes: &'a CoreLocal<MapCell<StaticSlice<Option<&'static dyn Process>>>>,
+    iterator: usize,


We lose the comment. Why this change?

bradjc · 2024-04-01T14:53:56Z

This implementation only includes support for single-core, single-threaded, platforms (as all of Tock for the moment) via the unsafe new_single_core constructor, though extending to multiple cores should be relatively simple.

At the moment seems to imply we need to guard against something else, but I don't see the need to make the scope larger than our current issue. Since this is pressing (we can't upgrade nightly), I think we need to keep the scope narrow.

In order to support mutation (which is ostensibly the purpose of static mut in many cases), users should use some form of single-threaded mutual exclusion, such as a MapCell, TakeCell, Cell, etc.

This seems like something that should be wrapped in our type. Why put the complexity on to users? It seems like the is the purpose of static mut, so why not a one-to-one replacement?

Those really really shouldn't touch the same static muts (now CoreLocals), but we don't have a great way of enforcing that

This probably comes down to how we articulate safety for the constructor for CoreLocal or by adding unsafe to one or more methods.

I view our job as kernel developers to manage the complexity of using unsafe correctly. Exposing more APIs where the burden is on other developers to reason about unsafe correctness abdicates our responsibility. We should put safeguards in place to make writing correct code easier. If that means a CI script that checks ISR functions, then that is what is requires.

alevy · 2024-04-01T15:10:47Z

This seems like something that should be wrapped in our type. Why put the complexity on to users? It seems like the is the purpose of static mut, so why not a one-to-one replacement?

Three reasons:

It separates reasoning about the safety of CoreLocal from reasoning about the safety of interior mutability. Both are a bit subtle, and to the extent they can be kept correct separately, the better.
Different users want/need different Cells for interior mutability. Already in the ports I have there are a couple cases where Cell makes more sense.
Mutability isn't the only reason we use static mut. The other reason is that you're not allowed to put even read-only things in non-mut globals that don't implement Sync, which is most things. So we have a few places where we use static mut to store something that is never used as mut, but isn't Sync. CoreLocal without an internal cell is better (more efficient, easier to use, more obviously correct, etc).

alevy · 2024-04-01T15:29:26Z

I view our job as kernel developers to manage the complexity of using unsafe correctly.

CI scripts would be nice, if we can figure out what and how to check for that.

I think this ultimately boils down to fairly simple rules, though I've not finished thinking through these and partially needs to be validated by implementing the rest of the port:

CoreLocals should have module-private scope
ISRs code should be in completely separate modules (and probably crates) than the rest of the kernel. I believe this is the case except apparently not in RISC-V right now :)

I'll note that this is very very similar to how thread_local! in Rust's std interacts with signal handlers---if you're in a signal handler, you may be able to access a thread-local global concurrent with the thread you were dispatched to, which may have been mid-critical-section unprotected by a lock (so, e.g., you could in principle get two mutable references). Signal handlers are mega unsafe, just like ISRs.

The high bit, ISRs are really not a point of extensibility for Tock and should be handled with the utmost care.

This, of course, doesn't change that having automatic ways of validating our the safety requirements would be really good.

alevy · 2024-04-01T19:12:18Z

Let me try to articulate the safety invariants for CoreLocal (that are not currently in the documentation, but obviously should be) that I think might make the feedback/discussion better directed.

A CoreLocal stores data (statically allocated) that is has a unique value per-core. Because Tock's execution model is such that each core has exactly one kernel-thread of execution, this struccture is Sync and, thus can wrap non-Sync types or references to non-Send types (most things in Tock). Meaning, it can be stored in non-mut statics.

An aside for justification

This kind of co-reliance between the system's execution model and the language-enforced APIs is fundamental. The notion of a thread, what constitutes a thread, and how to distinguish different threads is entirely an OS-level construct. For example, the safety of thread_local in Rust std entirely relies on the particular implementation of pthreads (e.g. using the gs register to store a location to thread-local variables and the kernel swaps that register when switching threads).

The hidden kernel threads

Tock does actually have one other kind of software that runs truly-concurrently (i.e. it may run at any point, including during "critical sections") with the kernel---interrupt service routines (ISRs).

There's an important invariant for ISRs (which may not always be maintained at the moment): ISRs must not share any data with the kernel except the variables that signal a syscall, hardfault, etc just fired (even those are generally controlled within the arch crate, but they are accessed in both ISR and kernel threads). Those do not use CoreLocal.

There are at least two notable exceptions to this rule right now. As @hudson-ayers points out, panic might be called from both the kernel thread and ISRs (for example, when an interrupt is not recognized, some archs panic!). This is a problem if the panic handler accesses core-local variables, which it does/will on most boards in order to print a debug message and blink.

Supporting more than one thread-of-execution

Supporting more than one thread (e.g. ISRs) safely is a tricky and potentially heavy-weight proposition. There needs to be some way of determining whether we are currently in---e.g. are we in ISR or the kernel main-thread, and which ISR. We could do this in architecture specific ways---e.g. use the armv7m IPSR register (which denotes the exception level), and either deny access if it's not the kernel main-thread, or have a separate copy for each ISR (that's a lot of memory....).

This implementation argues that a better strategy is to eliminate access to CoreLocals from ISRs, use an unsafe constructor for CoreLocal to denote that the user must ensure that such a static is not accessible from another core or from an ISR, and to have a simple set of "rules" to follow that ensure this by convention---and rely on ISRs themselves being highly subtle from a safety perspective (and requiring unsafe to implement in the first place) to avoid violating these rules in ISR context.

bradjc · 2024-04-02T13:11:24Z

This is a problem if the panic handler accesses core-local variables, which it does/will on most boards in order to print a debug message and blink.

I don't think this is problem worth thinking about; if your ship's sinking, poking a hole in the hull isn't going to matter.

This implementation argues that a better strategy is to eliminate access to CoreLocals from ISRs,

Agreed.

use an unsafe constructor for CoreLocal to denote that the user must ensure that such a static is not accessible from another core

This seems like it needs more explanation as to what the expectation is.

or from an ISR,

Agreed.

and to have a simple set of "rules" to follow that ensure this by convention

Sounds good in theory, but what are they?

---and rely on ISRs themselves being highly subtle from a safety perspective (and requiring unsafe to implement in the first place) to avoid violating these rules in ISR context.

Very much agreed.

bradjc

There's a lot in this PR (two new kernel types, changes to HILs, wrapping static muts in cells, and of course the headline change). How should we make forward progress?

bradjc · 2024-04-02T13:32:27Z

kernel/src/kernel.rs

-        }
-        None
+	self.processes.with(|ps| {
+	    ps.map_or(None, |ps| {


Does if let Some(ps) = ps.get() not work here?

bradjc · 2024-04-02T13:33:47Z

kernel/src/kernel.rs

 /// Main object for the kernel. Each board will need to create one.
 pub struct Kernel {
    /// This holds a pointer to the static array of Process pointers.
-    processes: &'static [Option<&'static dyn process::Process>],
+    pub(crate) processes: &'static CoreLocal<MapCell<StaticSlice<Option<&'static dyn process::Process>>>>,


bradjc · 2024-04-02T13:35:53Z

kernel/src/kernel.rs

 /// Main object for the kernel. Each board will need to create one.
 pub struct Kernel {
    /// This holds a pointer to the static array of Process pointers.
-    processes: &'static [Option<&'static dyn process::Process>],
+    pub(crate) processes: &'static CoreLocal<MapCell<StaticSlice<Option<&'static dyn process::Process>>>>,


I'm concerned about adding MapCell here. Nothing should ever call .take(), but that is an option now. What we need is a cell that can't lose its contents.

jrvanwhy · 2024-04-02T22:34:58Z

Need documentation.

In particular, I'd like to see safety requirements of CoreLocal::new_single_core -- it's almost impossible to review the rest of this PR for soundness without that documentation.

kernel/src/kernel.rs

jrvanwhy · 2024-04-02T22:15:52Z

kernel/src/kernel.rs

@@ -41,10 +46,43 @@ use crate::utilities::cells::NumericCellExt;
 /// is less than this threshold.
 pub(crate) const MIN_QUANTA_THRESHOLD_US: u32 = 500;

+#[derive(Copy, Clone)]
+pub struct StaticSlice<T> {


I'm having trouble seeing the semantic difference between a StaticSlice<T> and a &'static mut [T].

jrvanwhy · 2024-04-02T22:27:15Z

chips/nrf52/src/ble_radio.rs

+	    };
+            self.registers.packetptr.set(buf.as_ptr() as u32);
+	    self.registers.pcnf1.modify(PacketConfiguration1::MAXLEN.val(buf.len() as u32));
+	    slice::from_raw_parts_mut(old_base, old_len)
        }


It looks like old_base can be null at the slice::from_raw_parts_mut invocation, which is UB.

Please add a safety comment for slice::from_raw_parts_mut, and minimize the size of this unsafe block (is slice::from_raw_parts_mut the only unsafe operation here?).

jrvanwhy · 2024-04-02T22:33:16Z

kernel/src/core_local.rs

@@ -0,0 +1,17 @@
+use core::cell::UnsafeCell;
+
+pub struct CoreLocal<T>(UnsafeCell<T>);


I don't see why we need UnsafeCell here, though maybe the documentation will clarify that when it is added.

We don't, I believe! I'm confident this is an artifact from our earlier design. We may want to re-introduce it in the future, if we ever add an unsafe fn with_mut, where developers themselves must ensure non-reentrance.

I believe that having an UnsafeCell here will place all static CoreLocals into RAM. Without UnsafeCell, if T does not have interior mutability, then CoreLocal<T> can live in .rodata. I don't know how common it is to have immutable data in a CoreLocal -- presumably rare -- but that cost is probably worth thinking about if we decide to reintroduce (or keep) UnsafeCell.

jrvanwhy · 2024-04-02T22:36:17Z

kernel/src/core_local.rs

+impl<T> CoreLocal<T> {
+    pub const unsafe fn new_single_core(val: T) -> Self {
+	CoreLocal(UnsafeCell::new(val))
+    }
+}


That's not how I read it. A new_<something>() in Rust generally describes creating a particular variant of a thing (in this case, creating a single-core variant of a CoreLocal).

ppannuto · 2024-04-02T23:58:00Z

Catching up on phone on the train ride home, so haven't read or thought through all the subtleties here, but I feel it's worth pointing out that ISRs need not necessarily risk running concurrently with accesses to CoreLocals; i.e., it is a single assembly instruction to turn all interrupts off and similarly one to reenable them. We have the fundamental primitive available if needed to meet Rust's safety requirements IIUC.

(There are further complexities to such an approach, and overhead that isn't strictly necessary if we can otherwise prove ISRs won't access the memory, but a "Rust-legal Sync" is somewhat trivially available from this design IIUC)

lschuermann · 2024-04-03T11:49:01Z

it is a single assembly instruction to turn all interrupts off and similarly one to reenable them. We have the fundamental primitive available if needed to meet Rust's safety requirements IIUC.

One of the main issues with this is that this requires architecture-specific code (even to call the fn atomic helper we have in the arch crates). This would effectively either make the kernel crate specific to the architectures we currently support, or preclude us from including this in the kernel crate at all.

In our more "advanced"/multi-core design we have a trait-based approach that is actually able to resolve the core-ID at runtime (and thus inject architecture-specific code), but that's not entirely free (uses trait objects) and seems overengineered for this particular use-case.

Probably should just remove the dead code? But, it's not relevant to what I'm working on right now.

This required changing the ble_advertising HIL as well as the ble_advertising driver in ways that I'm not certain will work. (likely looses buffers). Need to test

jrvanwhy · 2024-04-03T23:29:43Z

kernel/src/core_local.rs

@@ -0,0 +1,17 @@
+use core::cell::UnsafeCell;
+
+pub struct CoreLocal<T>(UnsafeCell<T>);


I believe that having an UnsafeCell here will place all static CoreLocals into RAM. Without UnsafeCell, if T does not have interior mutability, then CoreLocal<T> can live in .rodata. I don't know how common it is to have immutable data in a CoreLocal -- presumably rare -- but that cost is probably worth thinking about if we decide to reintroduce (or keep) UnsafeCell.

jrvanwhy · 2024-04-04T00:00:28Z

kernel/src/core_local.rs

+    ///
+    /// By convetion, users should declare [`CoreLocal`] variables in
+    /// scopes that are inaccessible to ISRs or signal handler,
+    /// e.g. in module-private or function-local scopes.


This comment is somewhat self-contradictory. It first says "must only be created ... on systems ... without preemptive threads". It then explains that single-core systems can have preemptive threads in the form of ISRs and signal handlers, and implies it is okay to use a CoreLocal on such systems as long as you hide the CoreLocal from ISRs and signal handlers.

jrvanwhy · 2024-04-04T00:04:40Z

kernel/src/kernel.rs

@@ -41,10 +46,43 @@ use crate::utilities::cells::NumericCellExt;
 /// is less than this threshold.
 pub(crate) const MIN_QUANTA_THRESHOLD_US: u32 = 500;

+#[derive(Copy, Clone)]
+pub struct StaticSlice<T> {


Ah, I see. The issue is not Sized but that mut references are not allowed in a static item's type. Maybe a different name and/or a link to rust-lang/rust#57349 would make this more clear?

Some other improvement ideas:

StaticSlice doesn't actually need to be slice-specific. You can make it generic for any type and let the user choose whether to store a slice in it.

You can store a &'static mut T instead of a raw pointer to avoid unsafe in Deref and DerefMut.

Demonstration: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8de22b70edcbedaffccc382fd35ddd11

bradjc · 2024-04-05T13:48:42Z

I'm trying to understand the balance of flexibility with specificity of CoreLocal. The naming suggests flexibility: these objects are local to a core on CPUs with a single core, implying that this also supports objects local to a core on a multi-core CPU, and that another hypothetical type supports objects global to cores on multi-core CPUs. Given the scope of Tock (single core MCUs), I don't see the need for defining this broad space of shared objects. At the very least, the existence of new_single_core() implies there should be a new_multi_core(), otherwise, why be so specific? If the type only works on single core, then it would make sense for the type to be SingleCoreLocal with just new().

If there is a clear need for defining types for a more complex multi-threaded platform, what does the upgrade path look like for users who want to use the hypothetical multicore and/or multithread versions? If the kernel uses CoreLocal::new_single_core() everywhere, should they run a local patch to replace those types and constructors with the multiX version?

It seems like the current CoreLocal gets us to an unclear middleground: the type & constructor imply multiX support based on their naming, but don't indicate how you would use them to run on multiX systems.

One idea is to name the type based on how it should be used, not how it is implemented. Maybe SharedGlobal. The implementation changes based on the system you are running on to provide safety on that system. (Given there is no ambiguity about the hardware support for a given a system, this configuration is concrete and clear.) However, we want an implementation right now which does not make using this type fully safe in any context. So this may not be the approach right now.

The other direction is to double down on specificity and implementation, and not plan for a more generic approach right now. Maybe MainThreadSingleCoreLocal and then just a ::new() constructor. This, however, may also imply the existence of AnyThreadSingleCoreLocal and MultiCoreLocal etc. types. I think a more custom name (eg SimpleGlobal) would be fine too.

github-actions bot added kernel nrf Change pertains to the nRF5x family of MCUs. component labels Mar 31, 2024

alevy marked this pull request as draft March 31, 2024 20:45

github-actions bot added HIL This affects a Tock HIL interface. WG-Network In the purview of the Network working group. labels Apr 1, 2024

hudson-ayers reviewed Apr 1, 2024

View reviewed changes

bradjc requested changes Apr 1, 2024

View reviewed changes

bradjc reviewed Apr 2, 2024

View reviewed changes

jrvanwhy reviewed Apr 2, 2024

View reviewed changes

alevy force-pushed the dev/corelocal branch 2 times, most recently from 7dd7d9c to 7ede432 Compare April 3, 2024 17:27

alevy added 7 commits April 3, 2024 13:48

Initial CoreLocal implementation

d25ff06

Document CoreLocal

8381030

Port nrf52840dk to CoreLocal and avoid static mut

afd4da7

WIP: start getting rid of static mut in arch and kernel

63f8e8f

Remove static muts from deferred call

f7d8c30

Add an allow dead code for dead code

5703e88

Probably should just remove the dead code? But, it's not relevant to what I'm working on right now.

Add priority scheduler back and port

a480a6c

Port NRF52 ble_radio to CoreLocal

63fec30

This required changing the ble_advertising HIL as well as the ble_advertising driver in ways that I'm not certain will work. (likely looses buffers). Need to test

alevy force-pushed the dev/corelocal branch from 7ede432 to 573fa11 Compare April 3, 2024 17:48

Raspberry PI pico

283e662

alevy force-pushed the dev/corelocal branch from 573fa11 to 283e662 Compare April 3, 2024 17:49

jrvanwhy reviewed Apr 4, 2024

View reviewed changes

alevy mentioned this pull request Apr 16, 2024

Stopgap changes to treatment of static mut to get around compiler warnings. #3965

Merged

1 task

		processes: &'a CoreLocal<MapCell<StaticSlice<Option<&'static dyn Process>>>>,
		iterator: usize,

		@@ -0,0 +1,17 @@
		use core::cell::UnsafeCell;

		pub struct CoreLocal<T>(UnsafeCell<T>);

Replace static muts with new CoreLocal construct #3945

Are you sure you want to change the base?

Replace static muts with new CoreLocal construct #3945

Conversation

alevy commented Mar 31, 2024 • edited

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

alevy commented Apr 1, 2024

alevy commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lschuermann Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lschuermann commented Apr 1, 2024

bradjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alevy Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradjc commented Apr 1, 2024

alevy commented Apr 1, 2024

alevy commented Apr 1, 2024

alevy commented Apr 1, 2024

An aside for justification

The hidden kernel threads

Supporting more than one thread-of-execution

bradjc commented Apr 2, 2024 • edited

bradjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrvanwhy commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppannuto commented Apr 2, 2024

lschuermann commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradjc commented Apr 5, 2024

Replace `static mut`s with new `CoreLocal` construct #3945

Replace `static mut`s with new `CoreLocal` construct #3945

alevy commented Mar 31, 2024 •

edited

lschuermann Apr 1, 2024 •

edited

alevy Apr 3, 2024 •

edited

bradjc commented Apr 2, 2024 •

edited