IdAllocator documentation and implementation #9
Conversation
studychao
left a comment
studychao
left a comment
There was a problem hiding this comment.
Hi, I have added some code review. Please take a look :)
studychao
left a comment
studychao
left a comment
There was a problem hiding this comment.
I've added some few more comments, please take a look at them:)
andreeaflorescu
left a comment
andreeaflorescu
left a comment
There was a problem hiding this comment.
Thanks for splitting this effort in smaller PRs that are easy to review! 🎊
I did an initial round of review, and I have some question. I still have to look at the tests, and the allocate_id function can be optimized a bit I believe. That's nothing that can't be changed later though.
src/id_allocator.rs
Outdated
| /// # Example | ||
| /// | ||
| /// ```rust | ||
| /// # use vm_allocator::IdAllocator; |
There was a problem hiding this comment.
We are shifting to writing examples without using unwrap when that is possible, you can take a look at an example here: https://github.com/rust-vmm/vm-fdt/blob/9cfa0c8d7c3032f79589c7b01c8722a37f4cf44f/src/writer.rs#L289
The example should also be a bit more close to how you'd use it in a real life scenario. I'd say that there's value in just adjusting this kind of things before publishing on crates.io, instead of blocking the PR.
There was a problem hiding this comment.
done! ptal.
| # vm-allocator | ||
|
|
||
| ## Design | ||
| `vm-allocator` is a crate designed to to provide allocation and release strategies |
There was a problem hiding this comment.
I think one relevant thing that is missing from this document is the design choice of having one allocator per resource. We should somehow write about this as well.
src/id_allocator.rs
Outdated
| /// Creates a new instance of IdAllocator that will be used to manage the | ||
| /// allocation and release of ids from the interval specified by | ||
| /// `range_base` and `range_max` | ||
| pub fn new(range_base: u32, range_max: u32) -> Self { |
There was a problem hiding this comment.
I find u64 to be more future proof because then we could easily re-use the code for addresses as well. We can leave it as is for now though.
Cargo.toml
Outdated
| authors = [TODO] | ||
| authors = ["rust-vmm AWS maintainers <[email protected]>"] | ||
| description = "Allocation of resources needed by VMM" | ||
| keywords = ["Id", "Address space", "IRQ", "MSI", "MSI-x"] |
There was a problem hiding this comment.
You can't use space in keywords, it will fail when you'll try to publish it. I also think the keywords are not speaking so much about what the crate does.
I am thinking about a keyword that speaks more about the main action of the crate "allocating resources" than about what is the type of resources that you can allocate. At this point, I can't come up with anything.
There was a problem hiding this comment.
This comment about spaces in keywords has not been addressed.
There was a problem hiding this comment.
I have removed keywords to not block this PR in this. We will add it when we find something to put there.
src/id_allocator.rs
Outdated
| // All ids from the range specified are exhausted. | ||
| Overflow, | ||
| // An id that is not part of the specified range was requested to be released. | ||
| InvalidInput(u32), |
There was a problem hiding this comment.
If this is only used for out of range IDs, can we rename this to OutOfRange or similar so that the error variant is more verbose?
There was a problem hiding this comment.
It is not used only for out of range IDs. This is also returned when we try to release an ID that was never allocated or already released.
There was a problem hiding this comment.
Can we have another error for that one instead? InvalidInput is a bit too generic because most of errors are caused by an invalid input in the end.
| /// ``` | ||
| /// Id allocator representation. | ||
| pub struct IdAllocator { |
There was a problem hiding this comment.
Can you add a few comments about the internal design choices of this structure? What I am finding difficult to understand is why do we need to use an interval tree as oppose to just a vector in which we keep the freed ids. I would assume that some trade-offs were made and that these advantages and disadvantages were put in balance, but this kind of information tends to be forgotten, so it's better to keep it next to the code. What I don't particularly like about BTree is Rust is that it's making you implement useful operations that are not yet stabilized (the one that would be useful for us is pop_first).
Also, the allocation strategy should be mentioned somewhere IMHO (i.e. we first try to reserve IDs from previously freed ones), but I am not sure what is the best way to deliver the message.
|
|
||
| ```rust | ||
| /// Id allocator representation. | ||
| pub struct IdAllocator { |
There was a problem hiding this comment.
If we duplicate part of the implementation in README, then we need to make sure to keep them in sync if something changes (method signatures, doc comments, etc). For example, the current code snippet is already inconsistent: range_end vs range_max in the constructor.
There was a problem hiding this comment.
We can also just move the examples either in the code, or in an examples directory. A layout example is available here: https://doc.rust-lang.org/cargo/guide/project-layout.html
src/id_allocator.rs
Outdated
| /// println!("Interrupt number allocation failed: {:?}", irq_nr); | ||
| /// } | ||
| /// let irq = 8; | ||
| /// let freed_id_result = legacy_irq_allocator.free_id(irq); |
There was a problem hiding this comment.
What happens if an unallocated id is freed?
We can't guarantee that the allocated irq is 8 here.
There was a problem hiding this comment.
If an ID that was not allocated before we will return an error. In this line we check to see if the id requested is in the range we are working with. We explicitly check that the ID was allocated self.next_id < id if no we return InvalidInput for now but I think the error name will change.
|
It would be great to mention the origin of the design/algorithm, either from Cloud Hypervisor or crosvm:) |
AlexandruCihodaru
requested review from
andreeaflorescu,
gsserge and
jiangliu
|
@georgepisaltu @andreeaflorescu @gsserge @studychao @jiangliu thank you for your reviews, I have addressed your comments, please take another look. |
|
Two small comments regarding the keywords and the Error impl, otherwise looks good to me. |
AlexandruCihodaru
dismissed stale reviews from georgepisaltu and gsserge
via
95c02f7
Signed-off-by: AlexandruCihodaru <[email protected]>
Signed-off-by: AlexandruCihodaru <[email protected]>
Added IdAllocator initial implementation and tests. Signed-off-by: AlexandruCihodaru <[email protected]>
Signed-off-by: AlexandruCihodaru <[email protected]>
Signed-off-by: AlexandruCihodaru <[email protected]>
Signed-off-by: AlexandruCihodaru <[email protected]>
|
LGTM:) |
"great"? I think that such a mention is mandatory, for submissions which are derived from other work. Obviously, for basic licensing reasons, if we re-use any other work we need to know what we're using and where it comes from, and be sure that the licence is compatible. When an engineer adds their So it's already absolutely required that they know the origin and didn't just paste random stuff they found on Stack Overflow, or the whole project is in jeopardy. Given that, it costs nothing — and demonstrates that they have done their necessary due diligence — to expect them to state it. And is basic courtesy. If this isn't already documented as a fundamental requirement for the project, it should be. (Although it should also go without saying, as it's just basic engineer discipline.) |
7 participants
The documentation and implementation of AddressAllocator with IntervalTree backend will be added in a subsequent PR.