Implement Bounded channels #33

d3zd3z · 2024-11-26T23:54:49Z

Whereas the existing unbounded channels perform an allocation for each message sent, these bounded channels do a single allocation on initialization (of the channel, the zephyr queues used to implement it do an allocation). This allows send to block as well as recv.

This is the first part, coming soon will be send and receive with a timeout. This will allow using bounded channels from irq context (with no timeout).

Fixes #34

Taken from crossbeam channel, instead of directly storing the queue into the Receiver and Sender, instead wrap this in a enum, which will allow us to have different kinds of implementations. This is done via enum rather than traits as it would otherwise have to be dynamic, and a small dynamic decision is generally faster than the vtable that would result from using `dyn`. Signed-off-by: David Brown <[email protected]>

In addition to unbounded channels, also implement bounded channels. These use pre-allocated message buffers, where send as well as recv can block. They are implemented by using two `z_fifo`s to hold the items, treating one as a free list. Signed-off-by: David Brown <[email protected]>

Bounded channels only allocate on creation, and are able to block upon send until there is a message slot available. Signed-off-by: David Brown <[email protected]>

Add an argument to the `recv` method on Queue to allow a timeout to be specified. This will allow channels to have timeout variants available. Signed-off-by: David Brown <[email protected]>

Add implementations for `try_send` and `send_timeout` to channels. Currently, these all use the same error code, as there is only one kind of failure possible with Zephyr's channels (and zephyr treats "try" as just a zero timeout). Signed-off-by: David Brown <[email protected]>

Add `try_recv` and `recv_timeout` variants to the channel Receiver. Notably, this allows the special case of a `try_recv` on a bounded channel being safe to call from IRQ context. Notably, unbounded channels are _never_ safe to use from IRQ context. Signed-off-by: David Brown <[email protected]>

Add a bit of commentary as to the limited situations where it safe to use channels from IRQ context. Signed-off-by: David Brown <[email protected]>

Ablu

Only some nitpicking comments. LGTM overall. But I will repeat myself by saying that I find the safety tough to review without // SAFETY comments :).

Ablu · 2024-11-28T07:16:59Z

zephyr/src/sync/channel.rs

+            ReceiverFlavor::Unbounded { queue, .. } => {
+                unsafe {
+                    queue.release(|_| {
+                        crate::printkln!("Release");


This reads like it is just a temporary debug print?

Honestly, maybe it should just be a panic. Not really sure what to do other than just leak the underlying channel. In order to actually close them, Zephyr needs a new API to be able to wake up all blockers on a queue, not just one of them.

Ablu · 2024-11-28T07:18:40Z

zephyr/src/sync/channel.rs

+    /// An unbounded queue.  Messages are allocated with Box, and sent directly.
+    Unbounded {
+        queue: counter::Sender<Queue>,
+        _phantom: PhantomData<T>,
    }
 }


nit: I personally would have put them over the type where they were used (same below, would have kept Sender + Receiver Flavor together).

Perhaps I should move them to their own file, that's how crossbeam does it. But crossbeam also has many more flavors, and the code is a lot more complex, as they are actually implementing the channel, not just using an underlying k_queue like this does.

Actually, I'm not quite sure I understand where you would put this definition. You would group the flavors together rather than keeping the sender and receiver things together. I'm not sure I'd find that clearer.

zephyr/src/sync/channel.rs

Ablu · 2024-11-28T07:32:18Z

zephyr/src/sync/channel.rs

+        // Add each of the boxes to the free list.
+        for chan in &slots {
+            unsafe {
+                free.send(chan.get() as *mut c_void);


I find this pretty tough to review without SAFETY comments. The Box itself does not disallow moving the contents of it (through swap). Yet, this relies on pointers into it. So should the Slot or Box be wrapped in Pin?

I know that it does not change anything in practice. But I feel like it makes it a bit easier to reason about.

It could be, which might make it clearer. But, it isn't public, so doesn't leak anything. I'll add the pin.

Ablu · 2024-11-28T07:36:02Z

zephyr/src/sys/queue.rs

+    /// The timeout value can be [`Forever`] to block until there is a message, [`NoWait`] to check
+    /// and immediately return if there is no message, or a [`Duration`] to indicate a specific
+    /// timeout.
+    pub unsafe fn recv<T>(&self, timeout: T) -> *mut c_void


not relevant for this comment, but: pub unsafe? Should this be pub(crate) unsafe (probably the same for the other functions on this impl)? It reads like it is just an internal wrapper.

No, definitely 'pub'. This is part of the public interface. The k_queue is perfectly usable outside of this crate, it is just unsafe to do so. But there is no reason to stop external users from being able to do so.

Not sure, but if the intention is to create a safe wrapper for unsafe Zephyr API calls.

Would it not be better to wrap the k_queue_append call in an unsafe block, instead of declaring the send unsafe?

Otherwise the user of this function will have to wrap the code using this API in an unsafe block.

The same goes for the other unsafe APIs.

The problem is that k_queue_append is never safe. It takes a lot of stuff around it to make it safe. The idea is that the sys types are rust-language interfaces, but are still unsafe. Then in sync or other places will be the usable interfaces. The sys ones are available as there might be cases where someone needs the lower level functionality.

samples/philosophers/Kconfig

Fix a typo in the description of the Kconfig setting. Signed-off-by: David Brown <[email protected]>

Change behavior to panic if all handles are dropped. Add a comment describing this behavior, and the reason behind it. It could also be made to leak, but still should be documented. Signed-off-by: David Brown <[email protected]>

Rename this loop variable to better match what each element is, and avoid a conflict with a variable in an outer scope. Signed-off-by: David Brown <[email protected]>

Add comments describe the safey of unsafe blocks. Signed-off-by: David Brown <[email protected]>

Although this is a private field and never accessed within this code, explicitly mark it as `Pin` to make it clear that it is important that the data never be moved. Signed-off-by: David Brown <[email protected]>

d3zd3z · 2024-11-29T20:10:05Z

I have updated based on the review feedback, other than one that I'm not sure I agree with. These changes cosmetic, mostly adding comments. The Pin of the bounded message channel has some code change, but has no effect on behavior.

The `west twister` command seems to be broken right now. Fix this by directly invoking the twister script better. Signed-off-by: David Brown <[email protected]>

jilu-ot · 2024-12-04T18:37:45Z

zephyr/src/sync/channel.rs

+/// with a capacity of zero.
+pub fn bounded<T>(cap: usize) -> (Sender<T>, Receiver<T>) {
+    if cap == 0 {
+        panic!("Zero capacity queues no supported on Zephyr");


Maybe change message to: Zero capacity queues are not supported on Zephyr

I guess it would be even better with: Zero capacity queues are not supported by Zephyr

jilu-ot · 2024-12-05T09:57:29Z

zephyr/src/sync/channel.rs

+        .collect();
+        let slots = Box::into_pin(slots);
+
+        let free = Queue::new().unwrap();


Allocating memory could fail. So maybe queue::new should return an Option or Result that can be handled here.

At least in stable rust, allocation failures result in a panic. There is nightly provision for handling it, but it doesn't seem to be through returns.

Ok. If it is not the convention. I just thought it might be better to let the user of these modules decide what should happen in this case.

Ok. If it is not the convention. I just thought it might be better to let the user of these modules decide what should happen in this case.

I think we'll have to wait for Rust development itself to figure out how they're going to solve this. Nothing in 'alloc' returns an error on memory exhaustion. They call to a function that can either panic overall, or stop the present thread.

jilu-ot · 2024-12-05T10:01:45Z

zephyr/src/sync/channel.rs

    }
 }

+/// The "flavor" of a sender.  This maps to the type of channel.
+enum SenderFlavor<T> {


I think it would be more clear to a user what this type means, if it was called something like ChannelType.

Perhaps, but I'm trying to keep the naming and such as close to the crossbeam-channel implementation as possible, where it is called this.

jilu-ot · 2024-12-05T10:05:38Z

zephyr/src/sync/channel.rs

+    /// If the channel is empty and not disconnected, this call will block until the receive
+    /// operation can proceed or the operation times out.
+    /// wake up and return an error.
+    pub fn recv_timeout<D>(&self, timeout: D) -> Result<T, RecvError>


How about calling this method receive and let the timeout be an Option?

Makes the name more easily read and also covers the recv function.

Again, I'm trying to be compatible with crossbeam-channel, which has defined the API this way. The timeouts in Zephyr already support NoWait and Forever, but doing it this way allows the code to be used on the host with crossbeam-channel without changes.

jilu-ot · 2024-12-05T10:07:47Z

zephyr/src/sync/channel.rs

+    ///
+    /// This method is safe to use from IRQ context, if and only if the channel was created as a
+    /// bounded channel.
+    pub fn try_recv(&self) -> Result<T, RecvError> {


This method could be called get_message to more clearly say that it is retrieving a message from the channel, if there is one.

Likewise, just keeping compatible with an existing implementation.

Fix so that it has valid grammar. Signed-off-by: David Brown <[email protected]>

d3zd3z added 7 commits November 26, 2024 14:42

samples: philosophers: Test bounded channels as well

5bf1ac6

Bounded channels only allocate on creation, and are able to block upon send until there is a message slot available. Signed-off-by: David Brown <[email protected]>

zephyr: sys: queue: Add timeout to queue recv

d5ff559

Add an argument to the `recv` method on Queue to allow a timeout to be specified. This will allow channels to have timeout variants available. Signed-off-by: David Brown <[email protected]>

zephyr: sync: channel: Add IRQ safety comment

bdea978

Add a bit of commentary as to the limited situations where it safe to use channels from IRQ context. Signed-off-by: David Brown <[email protected]>

d3zd3z requested review from teburd, Ablu, cfriedt and ithinuel November 27, 2024 20:17

Ablu approved these changes Nov 28, 2024

View reviewed changes

cfriedt reviewed Nov 28, 2024

View reviewed changes

samples/philosophers/Kconfig Outdated Show resolved Hide resolved

d3zd3z added 5 commits November 29, 2024 12:31

samples: philosophers: typo fix

1bea58e

Fix a typo in the description of the Kconfig setting. Signed-off-by: David Brown <[email protected]>

zephyr: sync: channel: Panic on drop

e5c269d

Change behavior to panic if all handles are dropped. Add a comment describing this behavior, and the reason behind it. It could also be made to leak, but still should be documented. Signed-off-by: David Brown <[email protected]>

zephyr: sync: channel: Rename loop variable

bcd7481

Rename this loop variable to better match what each element is, and avoid a conflict with a variable in an outer scope. Signed-off-by: David Brown <[email protected]>

zephyr: sync: channel: Add safety comments

6738156

Add comments describe the safey of unsafe blocks. Signed-off-by: David Brown <[email protected]>

zephyr: sync: channel: Pin the bounded message slots

6b38734

Although this is a private field and never accessed within this code, explicitly mark it as `Pin` to make it clear that it is important that the data never be moved. Signed-off-by: David Brown <[email protected]>

d3zd3z force-pushed the bounded branch from 4c319a1 to a1c08b9 Compare November 29, 2024 21:16

CI: Fix twister invocation

635c6ff

The `west twister` command seems to be broken right now. Fix this by directly invoking the twister script better. Signed-off-by: David Brown <[email protected]>

d3zd3z force-pushed the bounded branch from a1c08b9 to 635c6ff Compare November 29, 2024 21:24

jilu-ot reviewed Dec 4, 2024

View reviewed changes

jilu-ot reviewed Dec 5, 2024

View reviewed changes

cfriedt approved these changes Dec 5, 2024

View reviewed changes

zephyr: sync: channel: Fix typo in panic message

1b3198a

Fix so that it has valid grammar. Signed-off-by: David Brown <[email protected]>

jilu-ot approved these changes Dec 6, 2024

View reviewed changes

d3zd3z merged commit d537adc into main Dec 11, 2024
1 check passed

Implement Bounded channels #33

Implement Bounded channels #33

Uh oh!

Conversation

d3zd3z commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ablu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d3zd3z commented Nov 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

d3zd3z commented Nov 26, 2024 •

edited

Loading