Tracking: Remove static global variables in kernel (process.rs)

[bradjc]

Right now there are two key variables in process.rs that the kernel uses extensively:

pub static mut PROCS: &'static mut [Option<&mut Process<'static>>] = &mut [];
static mut HAVE_WORK: VolatileCell<usize> = VolatileCell::new(0);

Having these global variables is probably not the best design choice, and doesn't match how the rest of Tock is structured. Plus removing them would remove a bunch of unsafe uses. This tracking issue is a place to work on what it would take to "tockify" those variables.

• Refactor debug.rs (Refactor debug.rs to not be implemented as a kernel process #1046)
• Create a Kernel struct that would hold this kernel-crate global state (kernel: make HAVE_WORK a member variable #1044)
• Move the processes array into the Kernel struct somehow (Kernel: Move PROCS array to Kernel struct #1111)
• Move CONTAINER_COUNTER to Kernel so we can ensure grants are created before process memory is setup.
• Move APP_FAULT and SCB_REGISTERS to /arch crates. (Move architecture-dependent syscall code to arch/cortex-m #1113, Move architecture-dependent debugging print code to arch/cortex-m #1115)

debug.rs makes it hard to refactor code in the kernel because it is so different from how the rest of the kernel is structured. It is predicated on the idea of "kernel processes", or code in the kernel that looks like any other app to capsules. These feel a bit bolted on and have lead to some difficult-to-maintain code. A possible replacement would be to make debug.rs not look like apps, and instead use a dedicated kernel interface from a capsule instead. Since we haven't had any other uses for kernel processes this seems reasonable.

Making process.rs not cortex-m specific will remove the global static variables that are used to move information from interrupt handlers to the kernel (like APP_FAULT and SCB_REGISTERS.

[bradjc]

For the updates to debug.rs: something like: https://github.com/tock/tock/compare/debug-no-process?expand=1

In this version, the DebugWriter is created with static_init!() like any other capsule struct, and can be passed anything that implements the UART trait to actually write the output. Right now this is just replacing the console on hail, but more realistically there would be a virtualization layer on top of the uart that console and kernel debug share. This also opens up the ability for boards to easily use the segger RTT as their kernel debug rather than the normal uart.

It seems like most of the DebugWriter code could be a capsule. I tried this, but then got stuck on how to create the static reference in debug.rs that the macros use to write strings. It needs a type, but I had moved the struct to a capsule, so that didn't work.

@ppannuto thoughts?

[ppannuto]

On the one hand, I think that someday, some ability to write "kernel app(let)s" will be useful, under the arguments of performance/timing/etc. However, I agree that the first pass at that is pretty cumbersome and a little too hacked together, I won't be so sad to see it go. The changes to Debug look pretty good to me, especially if we virtualize the console

[alevy]

A note that having PROCS as a static variable seems to be a problem for making Process generic in any way (e.g. across architectures). So yet another reason try to avoid it.

[bradjc]

I tried to take a stab at removing the static process array, and found very quickly that because we handle Process as &mut Process quite often this is not a straightforward conversion (like the HAVE_WORK conversion is).

[bradjc]

On attempt number 3 of moving the PROCS array I tried to put the array in a TakeCell:

/// Main object for the kernel. Each board will need to create one.
pub struct Kernel {
    /// How many "to-do" items exist at any given time. These include
    /// outstanding callbacks and processes in the Running state.
    work: Cell<usize>,
    /// This holds a pointer to the static array of Process pointers.
    processes: TakeCell<'static, [Option<&'static mut Process<'static>>]>,
}

Then many structs defined in the kernel crate need a pointer to the kernel something like this:

pub struct Grant<T: Default> {
    kernel: &'static Kernel,
    grant_num: usize,
    ptr: PhantomData<T>,
}

Then all uses of the PROCS array now access processes something like this:

impl Allocator {
    pub fn alloc<T>(&mut self, data: T) -> Result<Owned<T>, Error> {
        unsafe {
            let app_id = self.app_id;
            self.kernel.process_map_or(Err(Error::NoSuchApp), app_id, |process| {
                process.alloc(size_of::<T>())
                .map_or(Err(Error::OutOfMemory), |arr| {
                    ...
                })
            })
        }
    }
}

where the process_map_or() function looks like:

impl Kernel {
    ...

    crate fn process_map_or<F, R>(&self, default: R, process_index: usize, closure: F) -> R
    where
        F: FnOnce(&mut Process) -> R,
    {
        let processes_len = self.processes.map_or(0, |processes| processes.len());
        if process_index > processes_len {
            return default;
        }
        self.processes.map_or_else(|| panic!("boo"), |processes| {
            processes[process_index].as_mut().map_or(default, |mut val| {
                closure(val)
            })
        })
    }

    ...
}

The issue is that the kernel immediately takes the processes array inside of the takecell, so all future calls that try to use that array have nothing there (and the code shown panics). So...this approach seems like it won't work.

The only other idea I have is to make the Process struct like any capsule struct and have it use Cells instead of mutably modifying itself.

Am I missing any other approaches?

[alevy]

@bradjc, I don't think you're missing anything. Make Process like other capsule code is probably the right approach. It an every so slightly bigger bummer that there is another word attached to each grant type, but I think it's worth it.

Another approach could be to explicitly pass around kernel everywhere. This would result in a pretty massive change to interfaces, though, and i think is probably not worthwhile. But maybe good to keep in the back of our minds as an option, at least to compare against.

[bradjc]

Do we care about moving the CONTAINER_COUNTER variable?

tock/kernel/src/grant.rs

Line 11 in af55345

crate static mut CONTAINER_COUNTER: usize = 0;

It could also move to the Kernel struct.

[alevy]

Do we care about moving the CONTAINER_COUNTER variable?

I think this one belongs in the grant module.

[bradjc]

Should we think about some mechanism to detect calling grant::create() after process::create()? I was thinking that by keeping CONTAINER_COUNTER in the kernel struct we could check that you aren't incrementing CONTAINER_COUNTER after using it in a process.

[bradjc]

Should we think about some mechanism to detect calling grant::create() after process::create()? I was thinking that by keeping CONTAINER_COUNTER in the kernel struct we could check that you aren't incrementing CONTAINER_COUNTER after using it in a process.

Support for this is in master.