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

RFE: add high level wrappers for iterating over descriptor memory from virtiofsd #33

Closed
wants to merge 3 commits into from

Conversation

elmarco
Copy link

@elmarco elmarco commented Dec 10, 2020

virtiofsd has convenience Reader/Writer implementation which are generally useful for vhost-user implementations.

See also: https://gitlab.com/virtio-fs/virtiofsd-rs/-/issues/5

@slp

@elmarco
Copy link
Author

elmarco commented Dec 11, 2020

While increasing test coverage I found a split_at() issue: https://gitlab.com/virtio-fs/virtiofsd-rs/-/issues/6. I'll update the patch with the fix.

@slp
Copy link
Collaborator

slp commented Dec 14, 2020

Thanks, I like this. Perhaps we should move all queue related modules into their own queue directory?

@elmarco
Copy link
Author

elmarco commented Dec 14, 2020

Thanks, I like this. Perhaps we should move all queue related modules into their own queue directory?

That makes sense to me, let's do that.

@alexandruag
Copy link
Collaborator

Hi, thanks for the PR! I have started looking at it and will return with more comments in a couple of days.

Copy link
Collaborator

@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.

Hi again, and apologies for the delay. Things look good overall. I started by leaving a couple of high level comments, and I'll do another pass for some more specific points after we kinda get these out of the way first. Also, on a semi-related note, I was wondering if you have any thoughts to share regarding rust-vmm/vm-memory#125 (and the interface adjustments), since you folks seem to use VolatileSlice in multiple places.

@@ -178,3 +185,822 @@ impl<M: GuestAddressSpace> Iterator for DescriptorChainRwIter<M> {
}
}
}

#[derive(Clone, Debug)]
struct DescriptorChainConsumer<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you be ok with moving/renaming DescriptorChainConsumer, the other abstractions introduced here, and their tests as part of a separate module? (i.e. something along the lines of VolatileSliceConsumer, for lack of a better name :-s) They look quite useful for a number of use cases, but at the same time we might want to introduce other consumer abstractions as well in the future (i.e. maybe a lazier one).

Copy link
Author

Choose a reason for hiding this comment

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

DescriptorChainConsumer is internal/private details of DescriptorChain Reader/Writer. I think renaming it to VolatileSliceConsumer is fine. I can move the introduced code in a new descriptor_chain_rw.rs file if that helps.

Copy link
Author

Choose a reason for hiding this comment

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

@alexandruag ping?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I'll start looking at the latest changes tomorrow.

src/queue/descriptor_chain.rs Outdated Show resolved Hide resolved
@jiangliu
Copy link
Member

jiangliu commented Jan 5, 2021

How about splitting into two PRs:
one for splitting the Queue, which will be straight-forward,
the other for the virtiofs related Reader/Writer

@elmarco
Copy link
Author

elmarco commented Jan 11, 2021

How about splitting into two PRs:
one for splitting the Queue, which will be straight-forward,
the other for the virtiofs related Reader/Writer

It's basically asking me to make a PR for each patch. I don't mind.

/// Reader will skip iterating over descriptor chain when first writable
/// descriptor is encountered.
#[derive(Clone, Debug)]
pub struct Reader<'a> {

Choose a reason for hiding this comment

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

Is there any way to get address and length of underlying descriptor through reader/writer? It would be great to have addr and len methods.

Copy link
Author

Choose a reason for hiding this comment

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

I guess we could add a Reader::get_location() to return the current descriptor (addr, remaining_len). Is that what you would like? Perhaps you can work a patch on top? thanks

Copy link
Author

Choose a reason for hiding this comment

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

(imho, this kind of advanced function is out of scope of Reader/Writer helper though)

Choose a reason for hiding this comment

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

The underlying address and length are needed for virtio blk and scsi as DescriptorChain in this crate does not have addr and len fields but the cloud-hypervisor's vm-virtio's DescriptorChain does. Maybe this should be a separate issue.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I think it make sense to make it a separate issue.

Copy link
Collaborator

@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.

Had another look and left some more comments. There are a couple of other things to talk about as well afterwards (for example, the module split/naming question), but from now on I'll be able to reply much quicker. Thanks for all the discussions!

src/lib.rs Outdated Show resolved Hide resolved
src/queue/mod.rs Show resolved Hide resolved

#[derive(Clone, Debug)]
struct DescriptorChainConsumer<'a> {
buffers: VecDeque<VolatileSlice<'a>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, is using a VecDeque (instead of a Vec) really neccessary? pop_front can be replaced with keeping track of the index where the current "first element" resides, and push_front seems to be called only to effectively mutate the first element. Using a Vec has the advantage of being able to create a slice from it directly for the closure in consume, without allocating the extra vector.

Also, just wondering, what drove the choice to prepopule the collection of VolatileSlices? One alternative would be to keep the descriptor chain itself and lazily iterate as bytes are consumed (going through one slice at a time), which further reduces the number of extra operations. This changes the interface (for example, available_bytes would not be easily available anymore, the equivalent of consume would work with something like an Iterator instead of a slice, and split_at would need a different approach if still required), but are the differences important for any particular use case?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't write the code. Your arguments makes a lot of sense, what makes you think it couldn't be improved after the API is introduced? I would rather land something that works now, and improve later. Especially because I didn't write the code in the first place, and we have users of this code, so it works well enough.

Copy link
Author

Choose a reason for hiding this comment

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

As you suggested, I switched the code to use a Vec. Using an iterator for VolatileSlice seems a lot more involved, I would like to postpone it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm mainly trying to understand if some the approaches/methods that exist right now can be changed going forward, or they are actually required by certain use cases. In a sense, consuming bytes from a descriptor chain feels a bit more natural using stream operations vs random access ones, and this makes me wonder if (for example) methods like split_at and available_bytes can be removed/replaced in the future. Just trying to ensure I have the right model(s) in mind :D

Copy link
Author

@elmarco elmarco Jan 18, 2021

Choose a reason for hiding this comment

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

split_at() seems to be there for convenience (used by virtiofsd in multiple place), it could probably be removed. But I think internal implementation details could also allow a combined implementation, using a VolatileSlice iterator or a Vec, depending on the usage. This fall in performance details to me, that can be addressed later.

src/queue/descriptor_chain.rs Outdated Show resolved Hide resolved
src/queue/descriptor_chain.rs Show resolved Hide resolved
src/queue/descriptor_chain.rs Outdated Show resolved Hide resolved
}

fn collect_desc_chain_buffers<M: GuestAddressSpace>(
mem: &GuestMemoryMmap,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the mem parameter required vs. using the memory() method of DescriptorChain? Looks like the same question applies to Reader::new and Writer::new as well.

Copy link
Author

Choose a reason for hiding this comment

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

The guest memory may not be necessarily the same as the descriptor chain memory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please provide some more details about why this ends up being the case? Having what are effectively two different handles to memory objects here (or in similar situations) is quite confusing, and it may be an indication that we need to adjust some of the abstractions we're defining in vm-virtio.

Copy link
Author

@elmarco elmarco Jan 18, 2021

Choose a reason for hiding this comment

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

@alexandruag Ok, I tryed to get it to work with the memory from the DescriptorChain. My conclusion is that it's really hard, given that the GuestAddressSpace lifetime is associated with it. Even after significant effort to I didn't manage to refactor the code and use appropriate lifetime annotations. collect_desc_chain_buffers() cannot infer a lifetime for the return value, or take away the underlying GuestAddressSpace with it. Quite difficult tbh, and less powerful overall than having a seperate mem argument. Could you give it a try? If you manage to convince yourself it isn't possible as well, we may just leave it that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, I'll give it a shot as well.

src/error.rs Show resolved Hide resolved
The queue module is a bit crowded. It is easier to reason and extend by
slitting things a little bit. Extract out error, and move queue,
descriptor and descriptor_chain in a submodule.

A few fields and constants have been made pub(crate).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
src/queue/mod.rs Show resolved Hide resolved
fn position(&self, offset: usize) -> Option<(usize, usize)> {
let mut rem = offset;
for (pos, vs) in self.buffers[self.index..].iter().enumerate() {
if rem <= vs.len() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we should replace <= with a < here.

Copy link
Author

Choose a reason for hiding this comment

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

No, if rem fits in vs.len() we should return that. (if you replace with < it will fail)

}

fn collect_desc_chain_buffers<M: GuestAddressSpace>(
mem: &GuestMemoryMmap,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please provide some more details about why this ends up being the case? Having what are effectively two different handles to memory objects here (or in similar situations) is quite confusing, and it may be an indication that we need to adjust some of the abstractions we're defining in vm-virtio.

) -> Result<Reader<'a>> {
Ok(Reader {
buffer: DescriptorChainConsumer {
buffers: collect_desc_chain_buffers(mem, desc_chain.readable())?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tried experimenting a bit, and I encountered the following question: what's the usage pattern for the Reader and Writer when a descriptor chain has both device readable and writable descriptors? For example, if we start by creating a Reader, this uses desc_chain.readable(), which filters out all non-readable descriptors. What is the standard approach to reading bytes from the readable segments of a descriptor chain, and then writing stuff to the writable descriptors of the same chain using these abstractions?

Copy link
Author

Choose a reason for hiding this comment

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

Honestly, I don't think this is very common. In all cases, Reader/Writer is not meant to be the only high-level abstraction, if you have some weird case like this, then perhaps you shouldn't use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To give an example, virtio block device operation (described here) requires something like this. I think it can be solved with some tinkering on top of the current proposal; I'll try a few things out and return with a proposal in a couple of days tops. Meanwhile, if you can share some more details around the "why the need for two separate memory handles" above, that would be of great help as well.

Copy link
Author

Choose a reason for hiding this comment

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

yes, I am working on it. I am not very familiar with the vmm crates..

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll be happy to provide more context around the current state of things (or simply bounce ideas) if it helps. You can find myself and other folks from around here on the rust-vmm Slack workspace.

The Reader and Writer over the DescriptorChainConsumer are very
convenient because they allow to operate with a descriptor chain as if
was a regular buffer.

Move it from virtiofsd to vm-virtio so other vhost-user drivers can take
advantage of it.

https://gitlab.com/virtio-fs/virtiofsd-rs/-/issues/5

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@alexandruag
Copy link
Collaborator

I've been experimenting some more and here are the conclusions so far:

  • Yeah it's really complicated to use the memory from the DescriptorChain when getting full ownership of the object, and it remains difficult even when passing it as a mutable reference. While IMO more awkward w.r.t. the current DescriptorChain implementation, also passing a memory handle greatly simplifies things. Moreover, what pushed this over the edge for me as the preferred approach going forward is that, in generic terms, a DescriptorChain interface or even implementation doesn't have to own or even be aware of a GuestMemory handle.

  • I've added some slight changes here to tackle the issue of consuming a DescriptorChain that contains both readable and writable descriptors. Can you please have a look and see if they makes sense? The readers and writers only receive a mutable reference to a DescriptorChain now instead of full ownership, and the consumer knows about the type of descriptors we are interested in. I only checked that it builds for now, so apologies in advance for any functionality errors that slipped through.

  • It appears that DescriptorChainRwIter is not really that useful (and we can achieve the same effect by using something like Iterator::take_while with DescriptorChain). I know it's not in the scope of this PR, but I was just wondering WDYT about getting rid of it?

  • Last but not least, it would be great if we didn't actually have to support available_bytes and split_at for the consumers. What's the right place to start looking in the virtiofsd sources to better understand their usage scenarios?

@elmarco
Copy link
Author

elmarco commented Jan 21, 2021

I've been experimenting some more and here are the conclusions so far:

  • Yeah it's really complicated to use the memory from the DescriptorChain when getting full ownership of the object, and it remains difficult even when passing it as a mutable reference. While IMO more awkward w.r.t. the current DescriptorChain implementation, also passing a memory handle greatly simplifies things. Moreover, what pushed this over the edge for me as the preferred approach going forward is that, in generic terms, a DescriptorChain interface or even implementation doesn't have to own or even be aware of a GuestMemory handle.

Let's leave it then. It's not impossible to introduce a new function without it later on.

  • I've added some slight changes here to tackle the issue of consuming a DescriptorChain that contains both readable and writable descriptors. Can you please have a look and see if they makes sense? The readers and writers only receive a mutable reference to a DescriptorChain now instead of full ownership, and the consumer knows about the type of descriptors we are interested in. I only checked that it builds for now, so apologies in advance for any functionality errors that slipped through.

From an API usage, it is similar (the conversion to RwIter was internal). However, I prefer the approach that reuses the existing Iter. Furthermore, I also prefer to give out ownership, since that's also what the iterator need, and it's less confusing since you can do whatever you want with it, and trash it. So I would rather keep the current version,

  • It appears that DescriptorChainRwIter is not really that useful (and we can achieve the same effect by using something like Iterator::take_while with DescriptorChain). I know it's not in the scope of this PR, but I was just wondering WDYT about getting rid of it?

I am not sure it's worth it. You would break existing code and only make it slightly more complex for others to deal with. Furthermore, it looks like it could learn to deal with interleaved R/W buffers.

Another thing that looks odd to me is the handling of indirect descriptors. In the current implementation, it looks like you can have only one, and it must be the last buffer. The specification doesn't seem that limiting.

  • Last but not least, it would be great if we didn't actually have to support available_bytes and split_at for the consumers. What's the right place to start looking in the virtiofsd sources to better understand their usage scenarios?

I just grep in the source tree.
I think available_bytes() could be removed without much issue.
split_at() is useful convenience, used in read(), readdir() & readdirplus(). If you don't provide it, you will need a way to seek back in those functions, or buffer data. I can look at implementing Seek if you want, but what's the motivation behind getting rid of split_at exactly? maintainance & complexity burden? you are pushing it to other. performance? isn't that premature and have we looked at how it could addressed differently?

Overall, I would take a more pragmatic approach, take what exist and is being used so other can start using and sharing it. Address shortcoming, and introduce new approaches or new API if/when necessary. Drop deprecated stuff after some time.

@alexandruag
Copy link
Collaborator

Thanks for the details! As far as I understand, the handling of indirect descriptors is done in accordance with the 1.0 version of the standard. I think there are some differences for legacy devices, but we don't support those. My main motivation in asking about the previously mentioned functions is trying to understand different use cases better, if there's any missing logic on our side, and if we can identify some simplifications early (i.e. before first publishing the crate).

The only notable concern that remains is how to support consuming descriptor chains which are both readable and writable (for example maybe we can have a single abstraction that provides both read and write functionality). I'll think some more about this, and it would be great if other folks can chime in as well. Also, apologies for not being very responsive this week.

@alexandruag
Copy link
Collaborator

Hi, just a quick update: I've been trying some more stuff out, but for the last couple of days I switched over to #38 to see if we can unblock things there as well. I expect sometime next week to have some conclusions or at least clear next steps here.

@alexandruag
Copy link
Collaborator

Hi again, sorry for the delay. This is just a high level idea for now, but how about conflating the consumer, reader, and writer into a single entity that implements both Read and Write? (we can separate the buffers into one category or the other while parsing the chain)

@elmarco
Copy link
Author

elmarco commented Feb 15, 2021

Hi again, sorry for the delay. This is just a high level idea for now, but how about conflating the consumer, reader, and writer into a single entity that implements both Read and Write? (we can separate the buffers into one category or the other while parsing the chain)

Eventually, if that helps. As long as we don't break the existing API (& tests). Imho, this can be yet another TODO.

@alexandruag
Copy link
Collaborator

Hmm, I think that deferring the implementation of something like the proposed functionality until after an initial version is merged (which might not be 100% compatible) may cause more disruptive breakages down the line :( I'm not very keen on missing support for parsing mixed descriptor chains, and would like to give the alternatives a better look if we can invest some more time in this. I think one way that tries to avoid breaking compatibility even with the current PR is to add something like a method that creates both a Reader and a Writer from a single DescriptorChain. As a next step for me, I'll write a quick prototype to bring more data to the discussion as soon as I can circle back to this.

@jiangliu
Copy link
Member

@elmarco Should we restart the thread? Since the virtio-queue crate has been heavily refactored. And the project fuse-backend-rs, which is another implementation of virtiofsd, uses the same Reader/Writer too. It would be great if we could share the common code.

@Ablu
Copy link
Contributor

Ablu commented Jun 27, 2023

This could also be a neat place to add support for "0-copy" vectored read/writes. Virtiofsd currently is hand-rolling that using writev/readv calls.

It also goes a long way at making virtio devices easier to write I think.

@Ablu
Copy link
Contributor

Ablu commented Oct 16, 2023

@slp suggested taking a look at crosvm's abstractions on virtio-sound/vhost-device#35 (comment)

@MatiasVara
Copy link
Contributor

Hello, I think a high level wrapper for iterating over descriptor memory would be required for virtio-sound too. @slp suggested to use crosvm's abstraction for that. I am thinking to start to work on this. Would be the right approach to add descriptor_utils in vm-virtio/crates/virtio-queue/?

@Ablu
Copy link
Contributor

Ablu commented Oct 31, 2023

Would be the right approach to add descriptor_utils in vm-virtio/crates/virtio-queue/?

I think yes. It needs to be on that level (or higher) for people to be useful. And we already have quite a few layers. So virtio-queue sounds correct to me.

@MatiasVara
Copy link
Contributor

MatiasVara commented Nov 6, 2023

AFAIU, descriptor_utils.rs from virtiofsd is based on crosvm. Would it make sense to open a new PR that proposes to add descriptor_utils.rs from virtiofsd in vm-virtio/crates/virtio-queue/? This is an slightly modified version from crosvm.

@Ablu
Copy link
Contributor

Ablu commented Nov 6, 2023

AFAIU, descriptor_utils.rs from virtiofsd is based on crosvm.

I have not done the research on the differences between crosvm and virtiofsd's abstraction. But I understood this PR as already being based on the code from virtiofsd.

Would it make sense to open a new PR that proposes to add descriptor_utils.rs from virtiofsd in vm-virtio/crates/virtio-queue/? This is an slightly modified version from crosvm.

Sure! It would of course be great if the new PR would summarize what the difference is compared to this PR, virtiofsd's solution or crosvm's (if there is any).

@Ablu
Copy link
Contributor

Ablu commented Nov 15, 2023

Superseded by #278

@stefano-garzarella
Copy link
Member

We just merged #278, so I'm going to close this one.
Feel free to open other PRs to extend what we merged.

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.

8 participants