-
Notifications
You must be signed in to change notification settings - Fork 996
internal/task: add interrupt-safe queue #1002
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
Conversation
This commit adds a queue wrapper type which can be used to transfer tasks between interrupts and the scheduler.
aykevl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comments below. I think it's mostly fine (the only thing I'm worried about is the defer). It's mainly suggestions how some code might be more clarified, as it certainly wasn't obvious to me how it works.
Asynchronous programming is usually not obvious and hard to get right.
| // Avoid nesting interrupts inside here. | ||
| var nest nonest | ||
| nest.Lock() | ||
| defer nest.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to avoid defer here. defer might allocate heap memory (hopefully not in this case!) and the heap is currently not interrupt-safe and likely will never be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I wonder if this could be written more clearly like so?
var mask uintptr
if interrupt.SupportsNesting {
// If the system allows nested interrupts, disable them during this operation to make it atomic.
mask := interrupt.DisableInterrupts()
}
q.queues[1-q.bufSelect.Get()].Push(t)
if interrupt.SupportsNesting {
interrupt.EnableInterrupts(mask)
}That's a bit more verbose and of course needs some additions to the runtime/interrupts package, but it does look more obvious to me. What do you think @jaddr2line?
One thing in particular that makes me a little bit nervous is that the nonest mechanism essentially has a blacklist of architectures that allow nesting, but it is certainly possible more get added in the future (for example, it's possible to have nested interrupts in RISC-V although the current PLIC makes it a bit cumbersome).
| // InterruptQueue is a specialized version of Queue, designed for working with interrupts. | ||
| // It can be safely pushed to from an interrupt (assuming that a memory reference to the task remains elsewhere), and popped outside of an interrupt. | ||
| // It cannot be pushed to outside of an interrupt or popped inside an interrupt. | ||
| type InterruptQueue struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a reference for this scheme, or is this something you came up with yourself? I believe you once said it was based on interrupt/scheduler communication in another RTOS.
If it is based on an existing system, a reference would be very useful to understand the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An older implementation of InterruptQueue was based off of https://arxiv.org/pdf/0709.4558.pdf
However, it was complicated, and we can do better on platforms with atomics, so I just decided to swap it with a double buffer.
The scheme of seperating the interrupt queue from the runqueue was something I came up with myself, and it seemed to make more sense based on the constraints involved in TinyGo.
| // This implementation uses a double-buffer of queues. | ||
| // bufSelect contains the index of the queue currently available for pop operations. | ||
| // The opposite queue is available for push operations. | ||
| bufSelect volatile.Register8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr: volatile is probably fine (no change needed) but atomics might be more appropriate.
I wonder if this should use atomics instead? (See #1113). Atomics have somewhat different guarantees and are intended for synchronization (where volatile is intended for memory-mapped I/O). In practice it will usually have the same effect, especially on microcontrollers.
Specifically:
- Volatile guarantees there is exactly one load or store instruction. This is necessary for some memory-mapped I/O but prevents certain (probably irrelevant) compiler optimizations where values are speculatively loaded when they might not be needed. It does not guarantee the value is loaded or stored in a single operation (although, unofficially, you can rely on there being only one operation if the hardware has a load/store instruction for the given bit width).
- Atomics guarantee that (from the perspective of other threads or interrupts), the value is loaded/stored atomically. This means, for example, that a 64-bit value is also loaded/stored atomically even if the processor does not support such an operation. In the case of Go (and usually C++ too), the operations will happen in the expected order respective to similar atomic operations (but not necessarily to regular load/store instructions). This is mostly relevant to out-of-ourder and/or multicore systems.
I think volatile will be fine in this case, but just wanted to warn for this particular (somewhat semantic) difference. No change is needed from my POV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, maybe something like popSelect or receiverSelect would be clearer? Because apparently it's the queue used by the receiver, and the sender (pusher) uses the other one. This is not immediately clear from the name bufSelect.
(Just a suggestion, not necessary to change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this intentionally does not use atomics. On platforms with a hardware atomic CAS, this can be done a lot more efficiently. I wanted to separate this to have a different approach.
That said, I could probably get this to work with atomic loads and stores instead of volatile if we added an atomic 8-bit load/store, as this is what is needed to function efficiently on AVR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Note that with #1113, atomics are supported in most places. Not yet on AVR, but that should not be very difficult to implement (although it might be inefficient). 32-bit load/store atomic instructions on Cortex-M are basically free so they can be used.
|
|
||
| // Check if the queue is empty. | ||
| // This will return false if any tasks were pushed strictly before this call. | ||
| // If any pushes occur during the call, the queue may or may not be marked as empty. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the queue may or may not be marked as empty.
Could be me, but 'marked' sounds like it changes the queue, while it doesn't (at least, not in a relevant way). It only returns the current state of the queue.
|
Closing in favor of #1142 |
This commit adds a queue wrapper type which can be used to transfer tasks between interrupts and the scheduler.