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

Update documentation of avail_idx & used_idx #174

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

andreeaflorescu
Copy link
Member

@andreeaflorescu andreeaflorescu commented Jun 14, 2022

Summary of the PR

The avail_idx and used_idx methods panic if the ordering is not the right one. As checking the ordering incurs a performance degradation, the VMM will be responsible for calling these functions only with the right ordering. We're now making this panic explicit by adding it to the public documentation.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • Any newly added unsafe code is properly documented.

@andreeaflorescu andreeaflorescu force-pushed the check_ordering branch 2 times, most recently from 02a92c8 to 1c7a6f1 Compare June 14, 2022 09:44
gsserge
gsserge previously approved these changes Jun 16, 2022
lauralt
lauralt previously approved these changes Jun 16, 2022
@jiangliu
Copy link
Member

How abouting using "debug_assert()" instead of runtime checking?

@andreeaflorescu
Copy link
Member Author

andreeaflorescu commented Jun 20, 2022

How abouting using "debug_assert()" instead of runtime checking?

@jiangliu this will still cause a panic. In case we don't want to fix this by checking the operation, than we can just leave it as is and add a comment to the function specifying that the function panics in case it's called with unsupported ordering. WDYT?

@jiangliu
Copy link
Member

How abouting using "debug_assert()" instead of runtime checking?

@jiangliu this will still cause a panic. In case we don't want to fix this by checking the operation, than we can just leave it as is and add a comment to the function specifying that the function panics in case it's called with unsupported ordering. WDYT?

I feel it's a type of program error instead of a runtime error, so not sure about the runtime performance impact.

These methods panic if the ordering is not the expected one. Make this
clear in the documentation as well.

Signed-off-by: Andreea Florescu <[email protected]>
@andreeaflorescu andreeaflorescu changed the title don't allow invalid Ordering on queue operations Update documentation of avail_idx & used_idx Jun 21, 2022
@jiangliu jiangliu merged commit fdba651 into rust-vmm:main Jun 21, 2022
@andreeaflorescu andreeaflorescu deleted the check_ordering branch June 21, 2022 08:29
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