-
Notifications
You must be signed in to change notification settings - Fork 0
Use mio to replace Epoll #2
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
Conversation
b5d2117 to
d91521f
Compare
vhost-user-backend/src/event_loop.rs
Outdated
| pub struct VringEpollHandler<T: VhostUserBackend> { | ||
| epoll: Epoll, | ||
| poller: Mutex<Poll>, | ||
| fd_map: DashMap<RawFd, Vec<(Interest, u64)>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because mio couldn't deregister a single Interest and Token, we use this structure to record which Interest and Token we need. Maybe are there any better ways?
|
I found a deadlock problem. https://github.com/uran0sH/vhost/pull/2/files#diff-e1c9dc7802234bc118de8314ec9cacc98066eea325a6f14ff5887ece3466d5a3R260 Here will hold the lock to listen for events's arrival. https://github.com/uran0sH/vhost/pull/2/files#diff-e1c9dc7802234bc118de8314ec9cacc98066eea325a6f14ff5887ece3466d5a3R193 A lock is also needed here to register events. So this is what happens solve this problem by using the following method, but is there any other better method? @stefano-garzarella @germag @osteffenrh |
|
@uran0sH looking at the documentation, it seems that So, when you create WDYT? |
I try it, it can work. And there seems to be no unsafe behavior |
17621b6 to
add1eeb
Compare
d529720 to
ba2d4ee
Compare
vhost-user-backend/src/event_loop.rs
Outdated
|
|
||
| pub type VringEpollResult<T> = std::result::Result<T, VringPollError>; | ||
|
|
||
| // According https://github.com/tokio-rs/mio/blob/master/src/interest.rs#L27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a permalink, like https://github.com/tokio-rs/mio/blob/cd972977e7a8234e62c7ba8bd4d4ec1da208e576/src/interest.rs#L27
vhost-user-backend/src/event_loop.rs
Outdated
| epoll: Epoll, | ||
| poller: Mutex<Poll>, | ||
| registry: Registry, | ||
| // Record which fd and interest are registered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add "why" we need to register them.
vhost-user-backend/src/event_loop.rs
Outdated
| thread_id: usize, | ||
| ) -> VringEpollResult<Self> { | ||
| let epoll = Epoll::new().map_err(VringEpollError::EpollCreateFd)?; | ||
| // let epoll = Epoll::new().map_err(VringEpollError::EpollCreateFd)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover
vhost-user-backend/src/event_loop.rs
Outdated
| .iter() | ||
| .position(|(interest, d)| *interest == ev_type && data == *d) | ||
| { | ||
| interests.remove(pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also call self.registry.deregister() and call again self.registry.register() with the remaining interestes ?
| self.registry | ||
| .register(&mut SourceFd(&fd), Token(data as usize), ev_type) | ||
| .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://docs.rs/mio/latest/mio/struct.Registry.html#method.register:
Callers must ensure that if a source being registered with a Poll instance was previously registered with that Poll instance, then a call to deregister has already occurred. Consecutive calls to register is unspecified behavior.
So maybe we can't do that, without calling deregister
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to call reregister, see https://docs.rs/mio/latest/mio/struct.Registry.html#method.reregister
Re-registering an event source allows changing the details of the registration. Specifically, it allows updating the associated token and interests specified in previous register and reregister calls.
|
Today, talking with @stefano-garzarella we agreed that we don't need to use So, we don't really need the map, since we don't need to track the registered interest, but since the documentation for We need to keep track of the already register FD's, so a set should be enough. Something that should make I'll look into alternatives (for a more sane API), since on epoll registering twice returns an error instead of UB, and kevent will modify the already registered one. |
OK. If I understand correctly, normally we will only register each fd once, if it is registered twice we should throw an exception to the user to tell it that it is registered twice. So when calling deregister we can deregister the entire fd |
Bumps [rust-vmm-ci](https://github.com/rust-vmm/rust-vmm-ci) from `af54708` to `0b1cb86`. - [Commits](rust-vmm/rust-vmm-ci@af54708...0b1cb86) --- updated-dependencies: - dependency-name: rust-vmm-ci dependency-version: 0b1cb86353cc093f2e17d7e9f6820de80a6c274d dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
bd38446 to
478864f
Compare
germag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I think is in a good shape, just apply my suggestions and send a PR, we are approaching the end of the gsoc, so we need to have the public PR
vhost-user-backend/src/event_loop.rs
Outdated
| impl From<VringPollEvent> for Interest { | ||
| fn from(value: VringPollEvent) -> Self { | ||
| let mut interest = None; | ||
| if value == VringPollEvent::READABLE { | ||
| interest = interest | ||
| .map(|interest| Interest::READABLE | interest) | ||
| .or(Some(Interest::READABLE)); | ||
| } | ||
| if value == VringPollEvent::WRITABLE { | ||
| interest = interest | ||
| .map(|interest| Interest::WRITABLE | interest) | ||
| .or(Some(Interest::WRITABLE)); | ||
| } | ||
| return interest.expect("Interest is invalid"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we use this?, I mean, why we need to convert from mio::Interest to VringPollEvent?, if we don't do it, better removing it, remember we want to hide mio::Interest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this one. This one is to convert VringPollEvent to Interest. It will be used here: https://github.com/uran0sH/vhost/pull/2/files#diff-e1c9dc7802234bc118de8314ec9cacc98066eea325a6f14ff5887ece3466d5a3R242. User can use like this: https://github.com/uran0sH/vhost/pull/2/files#diff-e1c9dc7802234bc118de8314ec9cacc98066eea325a6f14ff5887ece3466d5a3R358
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget what I said, I see that mio::Interest is not public anymore, implementing impl From<VringPollEvent> for Interest is Ok
vhost-user-backend/src/event_loop.rs
Outdated
| impl From<Interest> for VringPollEvent { | ||
| fn from(value: Interest) -> Self { | ||
| let mut event = VringPollEvent::empty(); | ||
| if value.is_readable() { | ||
| event |= VringPollEvent::READABLE; | ||
| } | ||
| if value.is_writable() { | ||
| event |= VringPollEvent::WRITABLE; | ||
| } | ||
| return event; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this expose the mio::Interest type?, since VringPollEvent is public, this impl is also public. Maybe just a private function is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is unecessary. I will delete it.
vhost-user-backend/src/event_loop.rs
Outdated
|
|
||
| bitflags! { | ||
| #[derive(Debug, PartialEq, PartialOrd)] | ||
| pub struct VringPollEvent: u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VringPollEvent is too verbose, is a type that will be part of the public API of VringEpollHandler, so maybe something like:
EventTypeInterest(if you choose this one, please also change the parameters' nameev_typeinregister_listener()andunregister_listener())
are better
Epoll is linux-specific. So we use mio, which is a cross-platform event notification, to replace Epoll. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
|
See rust-vmm#316 |
Epoll is linux-specific. So we use mio, which is a cross-platform event notification, to replace Epoll.
This is a draft version.