Skip to content

Work Queue support with Async #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 29 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
da280a6
cmake: Consistent expansion of include path
d3zd3z Dec 27, 2024
6003752
zephyr: Add initial implementation of work queue and async support
d3zd3z Jan 7, 2025
5001b6e
zephyr: work: Support showing work in Segger SystemView
d3zd3z Jan 7, 2025
af336be
samples: work-philosophers
d3zd3z Jan 7, 2025
cf6f5df
zephyr: Add numerous missing conditionals around alloc
d3zd3z Jan 7, 2025
d78fd67
zephyr: work: Create ContextExt
d3zd3z Jan 7, 2025
82ee349
zephyr: work: Fix broken doc links
d3zd3z Jan 8, 2025
4d8ab92
zephyr: work: futures: Ensure JoinHandle is Send when it can be
d3zd3z Jan 10, 2025
0dba021
samples: work-philosphers: Async Semaphore based solution
d3zd3z Jan 10, 2025
0dbfd14
zephyr: work/kio: Timeout support for work
d3zd3z Jan 11, 2025
1fa53ab
zephyr: Apply `cargo fmt` to new work-queue code
d3zd3z Jan 17, 2025
abd6194
samples: bench: Create benchmark sample
d3zd3z Jan 28, 2025
72308cb
zephyr: sys: sync: semaphore: Remove a few 'mut' declarations
d3zd3z Jan 28, 2025
3d9360d
samples: bench: Add ping-pong semaphore benchmarks
d3zd3z Jan 28, 2025
97a12b1
samples: bench: Update samples.yaml
d3zd3z Jan 29, 2025
7ffa9c2
zephyr: work: futures: `size_of` utility function
d3zd3z Jan 29, 2025
788fe82
samples: bench: Additional tests of varying numbers of workers
d3zd3z Jan 29, 2025
d20529d
samples: work-philosophers: Add target restriction
d3zd3z Jan 29, 2025
82e7e87
samples: bench: Run `cargo fmt`
d3zd3z Jan 30, 2025
5a6fee5
zephyr: error: Always inline error wrappers
d3zd3z Feb 7, 2025
5f50dc5
zephyr: work: Simple Workqueue `Work`
d3zd3z Feb 7, 2025
e2191c8
samples: bench: Add Simple Work benchmark
d3zd3z Feb 7, 2025
5b883ed
samples: bench: Increase optimization of benchmark
d3zd3z Feb 7, 2025
4f6032b
samples: bench: Benchmark a "base semaphore"
d3zd3z Feb 11, 2025
a1c5f61
samples: bench: rustfmt update
d3zd3z Feb 11, 2025
69b8c74
zephyr: rustfmt update
d3zd3z Feb 11, 2025
369561f
zephyr: sys: sync: semaphore: Add missing conditional on alloc
d3zd3z Feb 11, 2025
9f89a3a
zephyr: work: Make Signal::new() just return Self
d3zd3z Feb 12, 2025
bedd897
zephyr: various comment cleanups from review suggestions
d3zd3z Feb 12, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ endfunction()
function(get_include_dirs target dirs)
get_target_property(include_dirs ${target} INTERFACE_INCLUDE_DIRECTORIES)
if(include_dirs)
set(${dirs} ${include_dirs} PARENT_SCOPE)
set(${dirs} "${include_dirs}" PARENT_SCOPE)
else()
set(${dirs} "" PARENT_SCOPE)
endif()
Expand Down
8 changes: 8 additions & 0 deletions samples/bench/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.20.0)

find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(bench)

rust_cargo_application()
33 changes: 33 additions & 0 deletions samples/bench/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Copyright (c) 2024 Linaro LTD
# SPDX-License-Identifier: Apache-2.0

[package]
# This must be rustapp for now.
name = "rustapp"
version = "0.1.0"
edition = "2021"
description = "A sample hello world application in Rust"
license = "Apache-2.0 or MIT"

[lib]
crate-type = ["staticlib"]

[dependencies]
zephyr = "0.1.0"
critical-section = "1.1.2"

# Dependencies that are used by build.rs.
[build-dependencies]
zephyr-build = "0.1.0"

[profile.release]
debug-assertions = true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could add codegen-units = 1 here to speed up the build?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give it a try.

overflow-checks = true
debug = true
opt-level = "z"
lto = true

[profile.dev]
debug = 2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here?

lto = true
opt-level = "z"
51 changes: 51 additions & 0 deletions samples/bench/Kconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Copyright (c) 2024 Linaro LTD
# SPDX-License-Identifier: Apache-2.0

mainmenu "Rust Dining Philosphers"

source "Kconfig.zephyr"

choice
prompt "Select Synchronization implementation"
default SYNC_CHANNEL

config SYNC_SYS_SEMAPHORE
bool "Use sys::Semaphore to synchronize forks"
help
Use to have the dining philosophers sample use sys::Semaphore, with one per fork, to
synchronize.

config SYNC_SYS_DYNAMIC_SEMAPHORE
bool "Use a dynamic sys::Semaphore to synchronize forks"
help
Use to have the dining philosophers sample use sys::Semaphore, with one per fork, to
synchronize. The Semaphores will be dynamically allocated.

config SYNC_SYS_MUTEX
bool "Use sys::Semaphore to synchronize forks"
help
Use to have the dining philosophers sample use sys::Mutex, with one per fork, to
synchronize.

config SYNC_CONDVAR
bool "Use sync::Condvar and sync::Mutex to synchronize forks"
help
Use to have the dining philosophers sample use a single data structure, protected
by a sync::Mutex and coordinated with a sync::Condvar, to synchronize.

config SYNC_CHANNEL
bool "Use sync::channel to synchronize forks"
help
Use to have the dining philosophers sample use a worker thread, communicating via
channels to synchronize.

endchoice

if SYNC_CHANNEL
config USE_BOUNDED_CHANNELS
bool "Should channel sync use bounded channels?"
default y
help
If set, the channel-based communication will use bounded channels with bounds calculated
to not ever block.
endif
7 changes: 7 additions & 0 deletions samples/bench/boards/pimoroni_tiny_2040.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright (c) 2024 Linaro LTD
# SPDX-License-Identifier: Apache-2.0

# This board doesn't have a serial console, so use RTT.
CONFIG_UART_CONSOLE=n
CONFIG_RTT_CONSOLE=y
CONFIG_USE_SEGGER_RTT=y
7 changes: 7 additions & 0 deletions samples/bench/boards/rpi_pico.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright (c) 2024 Linaro LTD
# SPDX-License-Identifier: Apache-2.0

# This board doesn't have a serial console, so use RTT.
CONFIG_UART_CONSOLE=n
CONFIG_RTT_CONSOLE=y
CONFIG_USE_SEGGER_RTT=y
9 changes: 9 additions & 0 deletions samples/bench/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright (c) 2023 Linaro LTD
// SPDX-License-Identifier: Apache-2.0

// This crate needs access to kconfig variables. This is an example of how to do that. The
// zephyr-build must be a build dependency.

fn main() {
zephyr_build::export_bool_kconfig();
}
17 changes: 17 additions & 0 deletions samples/bench/prj.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Copyright (c) 2024 Linaro LTD
# SPDX-License-Identifier: Apache-2.0

CONFIG_RUST=y
CONFIG_RUST_ALLOC=y
CONFIG_MAIN_STACK_SIZE=8192

CONFIG_POLL=y

# CONFIG_USERSPACE=y

# Some debugging
CONFIG_THREAD_MONITOR=y
CONFIG_THREAD_ANALYZER=y
CONFIG_THREAD_ANALYZER_USE_PRINTK=y
CONFIG_THREAD_ANALYZER_AUTO=n
# CONFIG_THREAD_ANALYZER_AUTO_INTERVAL=15
24 changes: 24 additions & 0 deletions samples/bench/sample.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
sample:
description: Philosphers, in Rust
name: philosophers rust
common:
harness: console
harness_config:
type: one_line
regex:
# Match message printed at the end.
- "Done with all tests"
tags: rust
filter: CONFIG_RUST_SUPPORTED
platform_allow:
- qemu_cortex_m0
- qemu_cortex_m3
- qemu_riscv32
- qemu_riscv32/qemu_virt_riscv32/smp
- qemu_riscv64
- qemu_riscv64/qemu_virt_riscv64/smp
- nrf52840dk/nrf52840
tests:
sample.rust/bench.plain:
tags: benchmark
min_ram: 256
117 changes: 117 additions & 0 deletions samples/bench/src/basesem.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
//! Base Semaphore
//!
//! This is an experiment into a different approach to Zephyr kernel objects.
//!
//! Currently, these kernel objects are directed through "Fixed", which is an enum referencing with
//! a pointer to something static declared, or to a `Pin<Box<UnsafeCell<T>>>`. This was done in an
//! attempt to keep things performant, but we actually always still end up with both an enum
//! discriminant, as well as an extra indirection for the static one.
//!
//! The deep issue here is that Zephyr objects inherently cannot be moved. Zephyr uses a `dlist`
//! structure in most objects that has a pointer back to itself to indicate the empty list.
//!
//! To work around this, we will implement objects as a pairing of an `AtomicUsize` and a
//! `UnsafeCell<k_sem>` (for whatever underlying type). The atomic will go through a small number
//! of states:
//!
//! - 0: indicates that this object is uninitialized.
//! - ptr: where ptr is the address of Self for an initialized object.
//!
//! On each use, the atomic value can be read (Relaxed is fine here), and if a 0 is seen, perform an
//! initialization. The initialization will lock a simple critical section, checking the atomic
//! again, to make sure it didn't get initialized by something intercepting it. If the check sees a
//! 'ptr' value that is not the same as Self, it indicates the object has been moved after
//! initialization, and will simply panic.

// To measure performance, this module implements this for `k_sem` without abstractions around it.
// The idea is to compare performance with the above `Fixed` implementation.

use core::{cell::UnsafeCell, ffi::c_uint, mem, sync::atomic::Ordering};

use zephyr::Result;
use zephyr::{
error::to_result_void,
raw::{k_sem, k_sem_give, k_sem_init, k_sem_take},
sync::atomic::AtomicUsize,
time::Timeout,
};

pub struct Semaphore {
state: AtomicUsize,
item: UnsafeCell<k_sem>,
}

// SAFETY: These are both Send and Sync. The semaphore itself is safe, and the atomic+critical
// section protects the state.
unsafe impl Send for Semaphore {}
unsafe impl Sync for Semaphore {}

impl Semaphore {
/// Construct a new semaphore, with the given initial_count and limit. There is a bit of
/// trickery to pass the initial values through to the initializer, but otherwise this is just a
/// basic initialization.
pub fn new(initial_count: c_uint, limit: c_uint) -> Semaphore {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using the C types here instead of Rust type (e.g. u32) for any other reason than convenience with the k_sem API? Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, this was intended to just be a quick test to see if the "base sem" model will be workable. Given that this has similar performance to using Fixed, I will likely make a larger change across the codebase to have the zephyr objects directly, with an init atomic. I won't be changing the APIs to the existing types.

let this = Self {
state: AtomicUsize::new(0),
item: unsafe { UnsafeCell::new(mem::zeroed()) },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be more reliable to use UnsafeCell<MaybeUninit<k_kem>> here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior is different, though. I'm not certain if the Zephyr init actually requires the items to be zero initialized, but I wanted to keep the semantics.

};

// Set the initial count and limit in the semaphore to use for later initialization.
unsafe {
let ptr = this.item.get();
(*ptr).count = initial_count;
(*ptr).limit = limit;
}

this
}

/// Get the raw pointer, initializing the `k_sem` if needed.
fn get(&self) -> *mut k_sem {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be better to return a Fixed<k_sem> here instead of a raw pointer, so the type is more explicit on what's behind?

Talking about safety, I would say that types must give insights to the developer about actual confidence one could have in the data behind the raw pointer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this little exercise (basesem, which will go away at some point) is to see about the feasibility of eliminating the Fixed type entirely. In this case, the items are always just used directly, and the atomic protects against things being moved (which only happens when there is a single reference anyway).

// First load can be relaxed, for performance reasons. If it is seen as uninitialized, the
// below Acquire load will see the correct value.
let state = self.state.load(Ordering::Relaxed);
if state == self as *const Self as usize {
return self.item.get();
} else if state != 0 {
panic!("Semaphore was moved after first use");
}

critical_section::with(|_| {
// Reload, with Acquire ordering to see a determined value.
let state = self.state.load(Ordering::Acquire);
if state == self as *const Self as usize {
return self.item.get();
} else if state != 0 {
panic!("Semaphore was moved after first use");
}

// Perform the initialization. We're within the critical section, and know that nobody
// could be using this.
unsafe {
let ptr = self.item.get();
let initial_count = (*ptr).count;
let limit = (*ptr).limit;

k_sem_init(ptr, initial_count, limit);
}

self.state
.store(self as *const Self as usize, Ordering::Release);
self.item.get()
})
}

/// Synchronous take.
pub fn take(&self, timeout: impl Into<Timeout>) -> Result<()> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick remark about naming here. I know that take is an usual operation name for a semaphore. However, in Rust, take is widely used for replacing variable (i.e. https://doc.rust-lang.org/core/mem/fn.take.html). So, I would say that perphaps we need something here to remove ambiguity.

But I do not have a better naming to propose sorry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for semaphores, I was trying to keep the same names as Zephyr. Embassy, for example, uses acquire and release. The very traditional names would be "try" and "increment", or even better, the Dutch "probeer" and "verhoog".

Names are always a little weird with semaphores because they tend to come from a specific way that semaphores are being used, whereas semaphores are actually pretty generic.

let timeout: Timeout = timeout.into();
let ptr = self.get();
let ret = unsafe { k_sem_take(ptr, timeout.0) };
to_result_void(ret)
}

pub fn give(&self) {
let ptr = self.get();
unsafe { k_sem_give(ptr) };
}
}
Loading