Tock: Missed optimization opportunities with `static mut` data?

[gendx]

Given that the topic of optimizing binary size came up in #1660, I want to mention that I recently observed what seems to be another missed optimization opportunity.

Looking at the nRF52840-DK's panic handler code, a const LED is chosen as a blinking signal, and therefore the corresponding GPIO is obtained from nrf52840::gpio::PORT. The LED index is compile-time, so I was expecting the array indexing to be optimized away.

https://github.com/tock/tock/blob/fbc863faf0c9615537ee52dcdccdfcb9204d2467/boards/nordic/nrf52840dk/src/io.rs#L89-L102

However, the disassembled code is the following:

00007d4e <core::panicking::panic_fmt>:
    7d4e:   b580        push    {r7, lr}
    7d50:   466f        mov r7, sp
    7d52:   b084        sub sp, #16
    7d54:   e9cd 0102   strd    r0, r1, [sp, #8]
    7d58:   f646 1074   movw    r0, #26996  ; 0x6974
    7d5c:   f2c0 0001   movt    r0, #1
    7d60:   9001        str r0, [sp, #4]
    7d62:   f24c 2060   movw    r0, #49760  ; 0xc260
    7d66:   f2c0 0001   movt    r0, #1
    7d6a:   9000        str r0, [sp, #0]
    7d6c:   4668        mov r0, sp
    7d6e:   f006 fe35   bl  e9dc <rust_begin_unwind>
    7d72:   defe        udf #254    ; 0xfe

0000e9dc <rust_begin_unwind>:
    e9dc:   b5f8        push    {r3, r4, r5, r6, r7, lr}
    e9de:   af04        add r7, sp, #16
    e9e0:   4604        mov r4, r0
    e9e2:   f006 fd3b   bl  1545c <<nrf5x::gpio::Port as core::ops::index::IndexMut<nrf5x::gpio::Pin>>::index_mut>
    e9e6:   4905        ldr r1, [pc, #20]   ; (e9fc <rust_begin_unwind+0x20>)
    e9e8:   e9cd 0101   strd    r0, r1, [sp, #4]
    e9ec:   a801        add r0, sp, #4
    e9ee:   9003        str r0, [sp, #12]
    e9f0:   a803        add r0, sp, #12
    e9f2:   4621        mov r1, r4
    e9f4:   f003 ff44   bl  12880 <kernel::debug::panic>
    e9f8:   defe        udf #254    ; 0xfe
    e9fa:   bf00        nop
    e9fc:   0001b5b0    .word   0x0001b5b0

In particular, I wasn't expecting to see the following instruction in rust_begin_unwind: <<nrf5x::gpio::Port as core::ops::index::IndexMut<nrf5x::gpio::Pin>>::index_mut>.

It turns out that the definition of PORT is static mut.

https://github.com/tock/tock/blob/fbc863faf0c9615537ee52dcdccdfcb9204d2467/chips/nrf52840/src/gpio.rs#L54-L56

Same goes for the PIN array.

https://github.com/tock/tock/blob/fbc863faf0c9615537ee52dcdccdfcb9204d2467/chips/nrf52840/src/gpio.rs#L3-L52

My guess is that even though the PORT.pins field is never mutated after initialization, the Rust compiler assumes that it could be mutated by something else (because PORT is static mutable), so the compiler cannot optimize PORT[13] into PINS[13] (because PORT.pins may not point to PINS anymore).

The call to &mut PORT[13] is syntactic sugar for IndexMut, which takes a &mut self reference as input, and therefore PORT is required to be mut, even though in fact it is immutable (it's the PIN array that is mutable).

Given the widespread use of static mut throughout the code base, I'm wondering what's the impact on code size, but there seem to be missed optimization opportunities here.

[alevy]

This is an excellent observation.

The reason both of these static symbols are currently marked mut is actually primarily because non-mut static variables need to be Sync (since multiple threads _could_ access them safely), while mut statics do not.

Our data structures are not (and should not) be Sync. If there was a way for us to mark something as unsafe to access from a global (the way mut statics are), but not actually "mutable", that would be better.

Any thoughts on better ways to structure this?

[jrvanwhy]

Our data structures are not (and should not) be Sync. If there was a way for us to mark something as unsafe to access from a global (the way mut statics are), but not actually "mutable", that would be better.

How about the following?

struct UnsafeSync<T> {
  inner: T,
}

unsafe impl<T> Sync for UnsafeSync<T> {}

impl<T> UnsafeSync<T> {
  pub const fn new(value: T) -> Self {
    Self { inner: value }
  }

  // Safety: Similar to `static mut` but you get non-`mut` access.
  pub unsafe fn get(&self) -> &T {
    &self.inner
  }
}

Note: requires the unstable const_fn feature.

Then instead of writing static mut x: usize = 4 you would write static x: UnsafeSync<usize> = UnsafeSync::new(4);. You would access it using a get() call wrapped in unsafe.

Alternatively, given the kernel's threading model, do we even need get() to be unsafe? I implemented something similar (but for userspace) here and I think the API is sound. It would be extremely unsafe to touch my TockStatic from an interrupt, but it should be safe to manipulate from the main thread.

[alevy]

Interesting, we'd need to be certain about the semantic implications, but it doesn't _seem_ reasonable at first glance at least.

Worth testing to see if this indeed fixes the code size problem that @gendx points out in this issue!

[alevy]

(Sorry, not exactly just a code size issue, also a performance and potentially memory issue)

[gendx]

Thanks @jrvanwhy for the proposal. This sounds interesting!

Another question would be how to prototype with this to see the performance, code size and memory implications. Maybe my example of GPIO pins is self-contained enough that we can do it. But getting numbers on the whole of Tock (or at least a given board) may take a non-trivial refactoring time.

In general, I also wonder whether things like detangling lifetimes from 'static everywhere (https://github.com/tock/tock/issues/1074, https://github.com/tock/tock/pull/1628) would make it easier to then prototype new strategies. I see these refactorings as useful, but they also have some cost (development time to do the refactoring). Generic lifetimes also bring a bit more verbosity, which I see as useful in the sense that it clarifies things, but which could also be seen as a "visual" cost.

[hudson-ayers]

My series of peripheral instantiation PRs (starting with #2069) are now all submitted, though 3 are yet to be merged. They have removed almost all uses of static mut to define peripherals:

Before these PRs, there were 231 uses of static mut, not counting use in comments, static mut MaybeUninit, byte buffers, and symbol definitions in "extern C" blocks:

hudson: ~/tock ((HEAD detached at 23b0737ee)) $ rg '^\s*(static mut|pub static mut) .*:' | rg -v '^\s*[/;]' | rg -v '(MaybeUninit|UninitializedBuffer)' | rg -v '\[(u8|u16)' | rg -v 'static mut _.*' | wc -l
231

After these PRs, there are 82 uses:

hudson: ~/tock (the-future) $ rg '^\s*(static mut|pub static mut) .*:' --glob '!doc/' | rg -v '^\s*[/;]' | rg -v '(MaybeUninit|UninitializedBuffer)' | rg -v '\[(u8|u16)' | rg -v 'static mut _.*' | wc -l
82

Of the remaining uses:

boards/: 62

chips/: 16 (was 161 before these PRs!)

arch/: 3

kernel/: 5

The vast majority (all but 13) of the uses in boards/ are to define the CHIP, PROCESSES and WRITER globals used by all boards. Of these CHIP and WRITER are necessary to allow access from the panic handler, while PROCESSES could probably be removed with some refactoring.

Most of the remaining uses in chips could probably be addressed as well, half are just peripherals I missed while working on my PRs, and the other half are peripherals that will be much easier to not use globals for once const generics is available.

The uses in kernel/ and arch are probably necessary.

Notably, this analysis ignores the 72 instances where byte buffers, such as those used by SPI or DMA, are defined as static mut variables. Almost all of these could trivially be moved to use static_init() at the cost of some added verbosity.

I consider all uses in boards/ much less important, as buffers defined in boards/ cannot be accessed from any other crates anyway.