Skip to content
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

Merged
merged 3 commits into from
Mar 30, 2022
Merged

Conversation

alexandruag
Copy link
Collaborator

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.

@alexandruag alexandruag requested review from jiangliu and slp as code owners March 4, 2022 11:05
Copy link
Member

@andreeaflorescu andreeaflorescu left a 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,
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change required in this commit? If not, I would postpone it when fixing #152, if yes, I would still leave some sort of explanation about it in the commit message or at least give a pointer to #152.

Copy link
Collaborator Author

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
Copy link
Member

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)?

Copy link
Collaborator Author

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.

M::Target: GuestMemory,
{
// Default, iter-based impl. Can most likely be improved.
self.iter(mem).ok()?.next()
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@alexandruag alexandruag requested a review from stsquad as a code owner March 11, 2022 09:46
@alexandruag alexandruag changed the title [RFC] Improve the usability of queue interfaces Improve the usability of queue interfaces Mar 11, 2022
@lauralt
Copy link
Contributor

lauralt commented Mar 11, 2022

Can you also add some tests? After that, I think it should be ready for merging.

@jiangliu
Copy link
Member

This proposal looks good to me, except more work is needed about go_to_previous_position().


/// 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>> {
Copy link
Contributor

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?

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

@alexandruag alexandruag left a 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.


/// 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>> {
Copy link
Collaborator Author

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!

jiangliu
jiangliu previously approved these changes Mar 29, 2022
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]>
@alexandruag
Copy link
Collaborator Author

Rebased and pushed a small fix to address the last outstanding comment. Thank you everyone for the reviews!

@jiangliu jiangliu merged commit 7bfe164 into rust-vmm:main Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants