Add VHOST_USER_SHARED_OBJECT support #268
Conversation
dorindabassey
requested review from
Ablu,
eryugey,
germag,
jiangliu,
sboeuf,
slp and
stefano-garzarella
as code owners
dorindabassey
force-pushed
the
shd_object
branch
from
f6121a8
to
46f60bf
Compare
dorindabassey
force-pushed
the
shd_object
branch
from
46f60bf
to
d192cbf
Compare
dorindabassey
force-pushed
the
shd_object
branch
2 times, most recently
from
cde090c
to
309ce46
Compare
vhost-user-backend/src/backend.rs
Outdated
| @@ -87,6 +89,18 @@ pub trait VhostUserBackend: Send + Sync { | |||
| /// function. | |||
| fn set_backend_req_fd(&self, _backend: Backend) {} | |||
|
|
|||
| /// This function gets an owned file descriptor that the front-end can use otherwise | |||
There was a problem hiding this comment.
the front-end can use otherwise no file descritptor, What would you like to say here? It sounds like a cut sentence.
| // 1. The file descriptor returned by `into_raw_fd()` is valid and open. | ||
| // 2. We ensure that `OwnedFd` will properly close the file descriptor when it is dropped. | ||
| let owned_fd = unsafe { OwnedFd::from_raw_fd(file.into_raw_fd()) }; | ||
| Ok(owned_fd) |
There was a problem hiding this comment.
Sorry, but this code is still obscure to me, do you have an implementation where this is tested?
My main question is: Why do we return an OwnedFd?
IMO we should do it like for inflight_fd, because I expect a backend to open a file, and then keep it open. When a frontend ask for it, then clone the file or fd and pass it to the caller of get_shared_object(), but it should not give ownership to the caller, but just a clone, because otherwise the caller closes it just after sending the message to the frontend, or did I miss something?
There was a problem hiding this comment.
this is supposed to be tested on another vhost-device, no implementation in place yet, but the vhost-device-gpu has this resource_assign_uuid command type that exports a resource uuid to a table using the shared_object_add implementation (for reference: draft needs this qemu patch) with the expectation that another vhost device will import a shared object using the shared_object_lookup. no implementation yet as this is meant to be done on another vhost-device. but testing locally is by calling the shared_object_lookup which triggers QEMU to use the VHOST_USER_GET_SHARED_OBJECT command.
My main question is: Why do we return an OwnedFd?
OwnedFd is more preferred for resources that use file descriptors but are not necessarily files, see
252 comment
IMO we should do it like for inflight_fd, because I expect a backend to open a file, and then keep it open. When a frontend ask for it, then clone the file or fd and pass it to the caller of get_shared_object(), but it should not give ownership to the caller, but just a clone, because otherwise the caller closes it just after sending the message to the frontend, or did I miss something?
IIUC, your assumption is that immediately after the call to get_shared_object() the fd is closed, but what happens in this case is that it transfers ownership of the file descriptor to the caller of get_shared_object() which use the OwnedFd within the scope, and the descriptor will automatically close once it goes out of scope, not immediately after the call to get_shared_object().
tbh, I have no strong opposition to using File though and i can change it to return File
There was a problem hiding this comment.
My main question is: Why do we return an OwnedFd?
OwnedFdis more preferred for resources that use file descriptors but are not necessarily files, see 252 comment
Okay, but why OwnedFd and not BorrowedFd ?
I mean my question was why passing the ownership of the fd to the caller?
IMO we should do it like for inflight_fd, because I expect a backend to open a file, and then keep it open. When a frontend ask for it, then clone the file or fd and pass it to the caller of get_shared_object(), but it should not give ownership to the caller, but just a clone, because otherwise the caller closes it just after sending the message to the frontend, or did I miss something?
IIUC, your assumption is that immediately after the call to
get_shared_object()the fd is closed, but what happens in this case is that it transfers ownership of the file descriptor to the caller ofget_shared_object()which use theOwnedFdwithin the scope, and the descriptor will automatically close once it goes out of scope, not immediately after the call toget_shared_object(). tbh, I have no strong opposition to usingFilethough and i can change it to returnFile
Just to be clear, BorrowedFd is fine by me, but I still don't get why OwnedFd.
Can you give an example of a backend implementing it?
For example, for me this example in a dummy backend is not clear.
Are we expecting a backend to open that fd just when get_shared_object() is called, then forget about that object?
Maybe I'm missing something, since I don't know how dmabuf works, so a better commit description could help.
There was a problem hiding this comment.
In principle, the importer will get the fd and attach itself to the buffer, but the exporter shall retain ownership. More than one importer is allowed, with the refcount increased for each one.
So I agree that it shouldn't be OwnedFd.
There was a problem hiding this comment.
Just to be clear, BorrowedFd is fine by me, but I still don't get why OwnedFd.
Ok, BorrowedFd it is.
In principle, the importer will get the fd and attach itself to the buffer, but the exporter shall retain ownership. More than one importer is allowed, with the refcount increased for each one.
So I agree that it shouldn't be OwnedFd.
Noted, i saw that in virtio-dmabuf and assumed that both importer and exporter needed ownership.
Can you give an example of a backend implementing it?
For example, for me this example in a dummy backend is not clear.
Are we expecting a backend to open that fd just when get_shared_object() is called, then forget about that object?
currently, no backend is implementing this, but the next plan is to implement it in the vhost-device-gpu which will return the dmabuf fd associated with the uuid to the importer that will get the fd and attach itself to the buffer when a lookup message is sent from another vhost device.
i don't mind using borrowed fd that way we keep a temporary access to the fd without transferring ownership.
There was a problem hiding this comment.
Noted, i saw that in virtio-dmabuf and assumed that both importer and exporter needed ownership.
Uhm, in the linked doc it states that caller retains ownership, being the caller, the vhost-user handling code. It is a developer's detail. Just because the virtio-dmabuf API is storing a pointer, but has no control over its life cycle.
There was a problem hiding this comment.
currently, no backend is implementing this, but the next plan is to implement it in the vhost-device-gpu which will return the dmabuf fd associated with the uuid to the
importer that will get the fd and attach itself to the bufferwhen a lookup message is sent from another vhost device.
My suggestion for the future, when adding new API, is to provide also a real example. I guess with a real device, we can see that the fd's ownership can or not be passed to the caller. This will help and speed up the review phase IMHO.
i don't mind using borrowed fd that way we keep a temporary access to the fd without transferring ownership.
Honestly, I can't see how it can work in a backend with a OwnedFd, for that reason I was asking for a working example. Also because I don't know anything about dmabuf, so I may be wrong on some assumption.
There was a problem hiding this comment.
I just realized that returning borrowedFd implicitly means that i have to use as_raw_fd() to get the raw fd when creating a BorrowedFd. this also means that the trait implementation will error cannot return value referencing temporary value which is related to the lifetime of the borrowed value. Hence, in a bid to solve this I use as_raw_fd() in the backend_req_handler.rs to get the raw fd for the BorrowedFd. I mean this is what we are trying to avoid already - #252 + it's introducing a lot of unsafe blocks. so I will just stick to using File, I believe that's a more simpler approach.
dorindabassey
force-pushed
the
shd_object
branch
from
309ce46
to
ed3a4e0
Compare
Add support for the `VHOST_USER_GET_SHARED_OBJECT` request in the backend. The GET_SHARED_OBJECT message is only usable when VhostUserProtocolFeatures::SHARED_OBJECT feature is negotiated. This support involves implementing a new method `get_shared_object` to enable the frontend to retrieve a fd from the backend about shared objects. When the frontend sends this `GET_SHARED_OBJECT` message to the backend, the backend opens a fd for use, and sends it's fd to the frontend. The implementation of the `get_shared_object` method is optional, and ensure that the function `get_shared_object` returns an error if the backend does not implement it. If the back-end supports shared objects, it should return a `File` representing the fd. Signed-off-by: Dorinda Bassey <[email protected]>
Add frontend support of `VHOST_GET_SHARED_OBJECT` message to request a file descriptor from the backend. Signed-off-by: Dorinda Bassey <[email protected]>
add test to trigger the VHOST_USER_SHARED_OBJECT message on both frontend and backend. Signed-off-by: Dorinda Bassey <[email protected]>
Add VHOST_USER_SHARED_OBJECT support in CHANGELOG.md files Signed-off-by: Dorinda Bassey <[email protected]>
dorindabassey
force-pushed
the
shd_object
branch
from
ed3a4e0
to
c1c186a
Compare
Summary of the PR
This PR adds support for the VHOST_USER_GET_SHARED_OBJECT message as described in the QEMU specification
When the VHOST_USER_GET_SHARED_OBJECT is triggered from a vhost device, this message is expected to return a fd with reference to the resources specified by the uuid. This message can be used if a vhost device export resources (SHARED_OBJECT_ADD), and the UUID for that resource is found in the exporters cache. This message allows forwarding of the fd to another vhost device in form of SHARED_OBJECT_LOOKUP message which imports the resources.
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.