-
Notifications
You must be signed in to change notification settings - Fork 89
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
Improve the usability of queue interfaces #148
Conversation
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.
LGTM with a few tiny comments.
@@ -102,7 +102,7 @@ pub struct AvailIter<'b, M> { | |||
impl<'b, M> AvailIter<'b, M> | |||
where | |||
M: Deref, | |||
M::Target: GuestMemory + Sized, | |||
M::Target: GuestMemory, |
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.
Can you add a description about this in the commit message as well? It will help with writing the changelog.
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.
Hmm, I think it's better to relax the trait bounds for the other methods as well first via a separate PR, and add that to the changelog. Created #152 to track this.
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.
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.
Added an explanation, and mentioned the issue as well in the commit message.
@@ -190,4 +192,11 @@ pub trait QueueStateT: for<'a> QueueStateGuard<'a> { | |||
|
|||
/// Set the index for the next descriptor in the used ring. | |||
fn set_next_used(&mut self, next_used: u16); | |||
|
|||
/// Pop and return the next available descriptor chain, or `None` when there are no more |
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.
Can you also add some description about how this is suppose to be used (i.e. in respect to the already existing iterator; i think you mention this is an alternative way of consuming the iterator)?
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.
Added an extra phrase to the comment. Didn't mention the iterator, as it doesn't even exist strictly in the context of the QueueStateT
trait.
crates/virtio-queue/src/state.rs
Outdated
M::Target: GuestMemory, | ||
{ | ||
// Default, iter-based impl. Can most likely be improved. | ||
self.iter(mem).ok()?.next() |
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.
Should we for now log an error here instead of completely disregarding the error?
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.
sure
Can you also add some tests? After that, I think it should be ready for merging. |
This proposal looks good to me, except more work is needed about |
crates/virtio-queue/src/queue.rs
Outdated
|
||
/// Pop and return the next available descriptor chain, or `None` when there are no more | ||
/// descriptor chains available. | ||
pub fn pop_descriptor_chain(&mut self) -> Option<DescriptorChain<M::T>> { |
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.
Hmm the commit message says there are no methods added to Queue
, but looks like there are some. Am I missing something?
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.
You are definitely right, some methods from a previous iteration actually slipped through, I'll remove when with the next push, thanks for catching this!
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.
@jiangliu I was wondering if the more verbose comment above regarding go_to_previous_position
makes sense from your perspective. Basically I think there's a two-pronged answer here: functionality equivalent to go_to_previous_position
on the state is already callable right now (via the iterator) so we're not weakening the current posture to any extent, and I think we need to take any bigger changes meant to add extra safety to chain handling as a separate topic for a subsequent iteration as they will likely have more profound implication on how the abstractions are used.
crates/virtio-queue/src/queue.rs
Outdated
|
||
/// Pop and return the next available descriptor chain, or `None` when there are no more | ||
/// descriptor chains available. | ||
pub fn pop_descriptor_chain(&mut self) -> Option<DescriptorChain<M::T>> { |
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.
You are definitely right, some methods from a previous iteration actually slipped through, I'll remove when with the next push, thanks for catching this!
This commit also introduces a relaxation of the trait bound on the generic type parameter `M` for `QueueStateT::avail_idx` (i.e. it no longer has to be `Sized`, as it was implicitly the case before). This change reduces the number of trait bounds that have to be explicitly propagated to the newly introduced methods. We should apply a similar update to the rest of the interface as well (tracked by rust-vmm#152). Signed-off-by: Alexandru Agache <[email protected]>
Signed-off-by: Alexandru Agache <[email protected]>
Add the `pop_descriptor_chain` and `go_to_previous_position` methods to the interface of `QueueGuard` by leveraging the implementation already available for the inner state object. Updated an existing test to include the new functionality. No corresponding methods were added to `Queue`, pending discussions around potentially removing the abstraction in its current form. Signed-off-by: Alexandru Agache <[email protected]>
Rebased and pushed a small fix to address the last outstanding comment. Thank you everyone for the reviews! |
This RFC suggests a solution addressing the shortcomings mentioned in #144 (there are some very small orthogonal changes that got through as well). Not sure if the choice of naming for the new trait is the best, but the approach is to use that to explicitly gate operations that are only safe/sane when we know for sure accesses are not performed in a multi-threaded context.