epoll to mio migration - #362
Conversation
When register_event we pass in the value of u32/u64, so we don't need to convert it to u16 and then pass it to handle_event. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
We can safely use usize instead of u64, because normally we will not register a data that exceeds the size of usize Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
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>
change the logging od undefined events in vring are logged using `println!` to `log::warn!`. Signed-off-by: EricMwangi <ericmwas01@gmail.com>
Rename the generic third paremeter in un/register_listener to `queue_idx`. This explicitly describes the function of the parameter. Signed-off-by: EricMwangi <ericmwas01@gmail.com>
Add is_error,is_read_closed and is_write_closed to exhaust states of event. This provides better clarity into the state of events. Signed-off-by: EricMwangi <ericmwas01@gmail.com>
Modify `VringPollHandler` to pass `&mut Poll` to `run`, removing the need for a Mutex wrapper around Poller. This simplifies compliance with the `Send` + `Sync` requirements of `VringPollHandler` and removes false implications of multithreaded safety. Signed-off-by: EricMwangi <ericmwas01@gmail.com>
| fn handle_event( | ||
| &self, | ||
| device_event: u16, | ||
| device_event: usize, |
There was a problem hiding this comment.
This change will break every existing implementation of the trait. Would it be possible to keep u16 and cast as needed?
| let vring = VringRwLock::new(mem, 0x1000).unwrap(); | ||
| backend | ||
| .handle_event(0x1, EventSet::IN, &[vring], 0) | ||
| .handle_event(0x1, EventSet::Readable, &[vring], 0) |
There was a problem hiding this comment.
same idea here: most implementations already expect EventSet::IN (although it's ignored in most cases).
| } | ||
|
|
||
| impl EventSet { | ||
| fn to_interest(self) -> Interest { |
There was a problem hiding this comment.
nit: Consider implementing std::convert::Into<Interest> instead.
| } | ||
| } | ||
|
|
||
| fn event_to_event_set(evt: &Event) -> Result<EventSet> { |
There was a problem hiding this comment.
nit: consider std::convert::TryFrom<Event>
| &self, | ||
| fd: RawFd, | ||
| ev_type: EventSet, | ||
| queue_idx: usize, |
There was a problem hiding this comment.
nit: This parameter may not be a queue index, event_id might be a better name.
| let handler = Arc::new( | ||
| VringEpollHandler::new(backend.clone(), thread_vrings, thread_id) | ||
| .map_err(VhostUserHandlerError::CreateEpollHandler)?, | ||
| VringPollHandler::new(backend.clone(), thread_vrings, thread_id, &poller) |
There was a problem hiding this comment.
Why is poller borrowed here and in the call to run instead of simply owned by VringPollHandler? Given that nothing else seems to use it.
There was a problem hiding this comment.
From pr-316 review, poller was part of VringPollHandler and required locking for the sync trait. Mio is inherently single threaded. The above implementation presents an alternative where poller is borrowed without object access being synced with a mutex across multiple threads.
The initial suggestion was made in PR-316.
3 participants
Summary of the PR
Epoll is linux specific. To support unix systems this PR changes I/O support to mio.
PR is also a continuation of #316.
The PR completes the work needed to move from epoll to mio.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
[ x ] All commits in this PR have Signed-Off-By trailers (with
git commit -s), and the commit message has max 60 characters for the
summary and max 75 characters for each description line.
[ x ] All added/changed functionality has a corresponding unit/integration
test.
[ x ] All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
[ x ] Any newly added unsafe code is properly documented.