Mutex: Only track a single locked flag

[bugadani]

No description provided.

[Dirbaio]

I don't think this is sound. register() can drop or wake wakers, which will call waker methods through the vtable which is basically running arbitrary code. This code could call back into the same Mutex, causing two &muts to the same WakerRegistration to exist at the same time, which is UB.

[bugadani]

Can we add a !Sync version of WakerRegistration, that would only need &self to register/wake?

[Dirbaio]

it might be possible if it uses Cell<Option<Waker>>, but it'd mean you'd have to constantly take out and put back the waker so it might be less efficient, not sure.

[bugadani]

We can look into it through a pointer unsafely I think.

[Dirbaio]

I don't think so. for example to wake the waker you'd need a &Waker pointing to the contents. The wake can run arbitrary code which can reentrantly call into the same WakerRegistration and replace the waker (with either &mut or ptr::write), which would be UB because you'd be mutating the data while a &Waker pointing to that location exists.

The only way is to really ensure you never get any & or &mut pointing to the contents, which is what Cell does basically.

[bugadani]

Uh, I might be dense here to not understand, but given the following impl, how can a reference exist while we're mutating the cell?

fn register(&self, w: &Waker) {
        match self.inner_as_ref() { // may need to be unsafe but that's beside the question
            Some(ref w2) if (w2.will_wake(w)) => return,
            _ => {}
        }

        // clone the new waker and store it
        if let Some(old_waker) = self.waker.replace(Some(w.clone())) {
            old_waker.wake() // this may call reentrantly into `register`
        }
    }

[bugadani]

Running the following example on an ESP32-S2 (just to reduce the instructions from the critical section) with the latest commit, I see a slight speedup, so I'd like to hammer on this until it becomes correct :)

#![no_std]
#![no_main]

use embassy_executor::Spawner;
use esp_backtrace as _;
use esp_hal::{
    prelude::*,
    time::Duration,
    timer::{systimer::SystemTimer, OneShotTimer},
};
use esp_println::println;

static mut COUNTER: u32 = 0;

const CLOCK: CpuClock = CpuClock::max();
const TEST_MILLIS: u64 = 500;

#[handler]
fn timer_handler() {
    let c = unsafe { COUNTER } as u64;
    let cpu_clock = CLOCK.hz() as u64;
    let timer_ticks_per_second = SystemTimer::ticks_per_second();
    let cpu_cycles_per_timer_ticks = cpu_clock / timer_ticks_per_second;
    println!(
        "Test OK, count={}, cycles={}/100",
        c,
        (100 * timer_ticks_per_second * cpu_cycles_per_timer_ticks * TEST_MILLIS / 1000) / c
    );
    loop {}
}

#[embassy_executor::task]
async fn task() {
    use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
    let mutex = embassy_sync::mutex::Mutex::<CriticalSectionRawMutex, ()>::new(());
    loop {
        unsafe { COUNTER += 1 };
        let _g = mutex.lock().await;
        embassy_futures::yield_now().await;
    }
}

#[esp_hal_embassy::main]
async fn main(spawner: Spawner) {
    let config = esp_hal::Config::default().with_cpu_clock(CLOCK);
    let peripherals = esp_hal::init(config);
    let systimer = SystemTimer::new(peripherals.SYSTIMER);
    esp_hal_embassy::init(systimer.alarm0);
    println!("Embassy initialized!");

    spawner.spawn(task()).unwrap();

    println!("Starting test");

    let mut timer = OneShotTimer::new(systimer.alarm1);
    timer.set_interrupt_handler(timer_handler);
    timer.enable_interrupt(true);
    timer.schedule(Duration::millis(TEST_MILLIS)).unwrap();
}

Before (1adee53): Test OK, count=296207, cycles=40512/100
After (6a18ca2): Test OK, count=317360, cycles=37811/100

Test's binary size shrinks from 92,720 to 92,576 using the same profile (release+fat LTO) and MCU.