[RFC] Simplify type-level handling of vm_memory's Bitmap trait#325
Open
roypat wants to merge 2 commits intorust-vmm:mainfrom
Open
[RFC] Simplify type-level handling of vm_memory's Bitmap trait#325roypat wants to merge 2 commits intorust-vmm:mainfrom
Bitmap trait#325roypat wants to merge 2 commits intorust-vmm:mainfrom
Conversation
roypat
requested review from
alexandruag,
andreeaflorescu,
epilys,
jiangliu,
slp,
stefano-garzarella and
stsquad
as code owners
roypat
force-pushed
the
bitmap-bounds-fixup
branch
from
e81af28 to
b0f1a4c
Compare
The `DescriptorChain::read` and `DescriptorChain::write` define a `B: BitmapSlice` type parameter. However, the type of the bitmap that will be associated with the `VolatileSlices` is already transitively prescribed by the `M` type parameter on the `DescriptorChain` object itself. To reconcile this, these functions need lengthy `WithBitmapSlice` bounds (which tells the compiler "these two different BitMap type variables are actually the same"). Drop the `B: Bitmap` slice parameter from these functions, and instead simply return `Reader<'a, MS<M::Target>>`, where `MS<M::Target>` resolves to the bitmap type induced by the guest memory implementation. This avoids the `WithBitmapSlice` bounds completely. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
bitmap-bounds-fixup
branch
from
b0f1a4c to
2031379
Compare
Member
Yep, I'm a bit worried too.
I think you should try the vhost-devices workspace where there are the devices that really consume the queues, maybe there we have more to change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the PR
Hi vm-virtio folks!
I was messing around with vm-memory's bitmap tracking functionality, in hopes of making it less of a mess (commit in my fork: rust-vmm/vm-memory@ce80a91 - admittedly not sure if I succeeded), but in doing so, I found out why vm-virtio needs those
WithBitmapSlicetrait bounds. So this PR eliminates them (and has the added advantage of making vm-virtio forward compatible with my bitmap proposal to vm-memory).Admittedly, I'm a bit worried about all those explicit type hints the compiler suddenly needs in the tests (update: figured out what these are able:
Reader::writedoes not refer toSelfas a type at all now, so when I writeReader::<T>::new()the return value will be completely unrelated toT. ugh.) , but figured I'd throw it y'all's way anyway :)I compiled the vhost workspace against this, and everything goes through without modifications there
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.