Virt/selection policy#13
Conversation
| - **[Virtual SPI](src/virtualizers/virtual_spi.rs)**: Shared SPI and fixed chip select pins. | ||
| - **[Virtual Timer](src/virtualizers/virtual_timer.rs)**: Shared timer. | ||
| - **[Virtual UART](src/virtualizers/virtual_uart.rs)**: Shared UART bus. | ||
| - **[Selection Policy](src/virtualizers/selection_policy.rs)**: Policy that determines which device is selected first. |
There was a problem hiding this comment.
| - **[Selection Policy](src/virtualizers/selection_policy.rs)**: Policy that determines which device is selected first. | |
| selected first. |
This is not a capsule, I would not add it in this list.
| } | ||
| } | ||
|
|
||
| // Constructor with an option for custom selection policy |
There was a problem hiding this comment.
| // Constructor with an option for custom selection policy | |
| /// Create a new Mux with a custom selection policy. |
Detail this please.
| impl<'a> MuxUart<'a> { | ||
| pub fn new(uart: &'a dyn uart::Uart<'a>, buffer: &'static mut [u8], speed: u32) -> MuxUart<'a> { | ||
| impl<'a, P: SelectionPolicy<&'a UartDevice<'a, P>>> MuxUart<'a, P> { | ||
| // Backward compatible constructor |
There was a problem hiding this comment.
| // Backward compatible constructor | |
| /// Create a new Mux with the [`InsertionFIrstPolicy`] selection policy. |
|
|
||
| use core::cell::Cell; | ||
|
|
||
| pub trait SelectionPolicy<I> { |
312dd1d to
33269b3
Compare
eva-cosma
left a comment
There was a problem hiding this comment.
The code itself looks good, just a few comments about the documentation.
I would try to document the SelectionPolicy less as "what device is chosen first" and more as in "in what order the devices are selected for execution". Purely code-wise, yes, we determine what the "first" element that matches our criteria is, that is completely true, but functionally we determine what is the order of execution. Moreso, SelectionPolicy can also be used as a prioritization mechanism.
I'm not going to block the PR from being upstreamed with this documentation, since this is a minor thing, just to improve clarity and for it to be easier to understand what the grand scope of this selection policy is.
As a sidenote, my rule-of-thumb when it comes to documentation is "try to answer first 'Why?' not 'How?'". Why do we need a selection policy? After that, How do I use a selection policy?
| $P | ||
| ) | ||
| }; | ||
| // Added for backward compatibility |
There was a problem hiding this comment.
| // Added for backward compatibility | |
| // By default, if no selection policy is provided we will use the `InsertionFirstPolicy`. This option has been chosen as the default to maintain backwards compatibility. |
| - **[Virtual SPI](src/virtualizers/virtual_spi.rs)**: Shared SPI and fixed chip select pins. | ||
| - **[Virtual Timer](src/virtualizers/virtual_timer.rs)**: Shared timer. | ||
| - **[Virtual UART](src/virtualizers/virtual_uart.rs)**: Shared UART bus. | ||
| - **[Selection Policy](src/virtualizers/selection_policy.rs)**: Policy that determines which device is selected first. |
There was a problem hiding this comment.
| - **[Selection Policy](src/virtualizers/selection_policy.rs)**: Policy that determines which device is selected first. | |
| - **[Selection Policy](src/virtualizers/selection_policy.rs)**: Policy that determines in what order the virtual peripherals are selected for execution. |
Feel free to change the phrasing a bit on this one.
|
@alexandruradovici is the benchmark failing due to @frihetselsker 's code, or due to something on the repo that should pass when we upstream this PR? |
@frihetselsker can you apply the changes to the documentation? |
|
On it |
33269b3 to
2da4851
Compare
2da4851 to
1a37768
Compare
Pull Request Overview
This pull request fixes the fairness problem of virtual devices. So far it only implements it for the UART Mux, but if approved, the same fix will be easily implemented for the rest of the affected Muxes.
The Fairness Problem
When searching for the next operation, multiplexers (except the Alarm) will always iterate the devices list from beginning to end. This means that if one device always has a job and it was inserted earlier, it will always be picked.
The following examples illustrates the problem for the UART Mux. If
debug!()is called frequently (for example in theUartMux), theDebugConsolehas a monopoly over the UART Multiplexer, as it will be the only devices chosen to print.Solution
We introduce the
SelectionPolicytrait that provides Muxes with the device selection logic. It is a generic type and should not add any overhead, neither in speed, neither in code space.We define two policies:
InsertionFirstPolicythat behaves like Muxes used to behave so far, it iterates the list from the beginning to the end. This provides backward compatibility.RoundRobinPolicythat iterates the devices list in a round robin manner. Using this policy with the previousdebug!examples allows all device to print.This solution offers a structural change that gives users the freedom to choose the policy they want. By default, the UART Mux uses the
InsertionFirstPolicyand there are no changes required in the code.Users will now able to implement their own selection policies for the UART Mux.
Testing Strategy
This pull request was tested by using a custom kernel of
qemu_rv32_virtwith UART peripheral, and three equivalent applications that try to useprintf()simultaneously.TODO or Help Wanted
Feedback
Documentation Updated
/docs, or no updates are required.We are unsure of the proper placement of the documentation in the
READMEfile. Help is needed here as well.Formatting
make prepush.AI Use
code in this PR, if any, and I have manually checked and
personally certify the entire contents of this PR.