Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Thu, Sep 07, 2023 at 08:09:33AM -0500, Eric Blake wrote: > On Wed, Sep 06, 2023 at 05:02:06PM -0500, Eric Blake wrote: > > > Okay, I'll merge in that aspect of my original along with your other > > > improvements, and push something soon. Fingers crossed that we'll > > > finally get a green CI run today. > > > > Now in as 75979812. Still not a green CI run; but the BSD builds now > > work. The only remaining holdout is OpenSUSE Leap 15.5, where OCaml is > > stuck at 4.06 and therefore lacks List.to_seq |> Map.of_seq. I'm not > > sure if it is worth trying to copy the 4.07 implementations just so we > > can keep the two sorted name maps (one of the reasons you WANT to use > > a standard library implementation of Map is that writing a correct > > self-balancing binary tree implementation is not trivial). But do we > > need the resulting sets of APIs to be sorted by name? If we just > > stick to constructs in 4.06, we may be hit with slower O(n^2) list > > filtering when comparing which APIs go in which of the two lists (sync > > vs async), but is our list of APIs so large that it actually makes a > > noticeable difference to compile time? > > > > Rich, do you have any more ideas beyond Erik's recommendation [1] of > > just dropping OpenSUSE 15.5 and focusing only on Tumbleweed? > > > > [1] https://gitlab.com/nbdkit/libnbd/-/merge_requests/5#note_1535510343 > > Thanks to stackoverflow [2], I came up with an alternative (using > List.fold_left to build up from NameMap.empty through repeated > NameMap.add, without any intermediate representation in Seq). As of > commit db9f6bf6, we finally have green CI in libnbd again! > > [2] > https://stackoverflow.com/questions/61220655/how-to-initialize-map-from-list-in-ocaml Excellent news, thanks! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Wed, Sep 06, 2023 at 05:02:06PM -0500, Eric Blake wrote: > > Okay, I'll merge in that aspect of my original along with your other > > improvements, and push something soon. Fingers crossed that we'll > > finally get a green CI run today. > > Now in as 75979812. Still not a green CI run; but the BSD builds now > work. The only remaining holdout is OpenSUSE Leap 15.5, where OCaml is > stuck at 4.06 and therefore lacks List.to_seq |> Map.of_seq. I'm not > sure if it is worth trying to copy the 4.07 implementations just so we > can keep the two sorted name maps (one of the reasons you WANT to use > a standard library implementation of Map is that writing a correct > self-balancing binary tree implementation is not trivial). But do we > need the resulting sets of APIs to be sorted by name? If we just > stick to constructs in 4.06, we may be hit with slower O(n^2) list > filtering when comparing which APIs go in which of the two lists (sync > vs async), but is our list of APIs so large that it actually makes a > noticeable difference to compile time? > > Rich, do you have any more ideas beyond Erik's recommendation [1] of > just dropping OpenSUSE 15.5 and focusing only on Tumbleweed? > > [1] https://gitlab.com/nbdkit/libnbd/-/merge_requests/5#note_1535510343 Thanks to stackoverflow [2], I came up with an alternative (using List.fold_left to build up from NameMap.empty through repeated NameMap.add, without any intermediate representation in Seq). As of commit db9f6bf6, we finally have green CI in libnbd again! [2] https://stackoverflow.com/questions/61220655/how-to-initialize-map-from-list-in-ocaml -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Tue, Sep 05, 2023 at 09:45:44AM -0500, Eric Blake wrote: > On Mon, Sep 04, 2023 at 06:59:15PM +, Tage Johansson wrote: > > > > > > +// can be both readable and writable, but we survive just fine > > > > +// if we only see one direction even when both are available. > > > > +poll.registry().register( > > > > + SourceFd(), > > > > +Token(0), > > > > +MioInterest::READABLE | MioInterest::WRITABLE, > > > > +)?; > > > > +poll.poll( events, Some(Duration::ZERO))?; > > > Why do we want 'poll.poll()?;', that is, to fail this function if the > > > poll returns an error? We _expect_ poll to sometimes return an error > > > (namely, the fact that it timed out) if there is nothing pending on > > > the fd, at which point we WANT to successfully clear the ready_guard > > > for both read and write, rather than to error out of this function. > > > > > > You are right. I thought that the poll() call would return Ok(()) upon > > timeout, but according to the documentation: > > > > > Currently if the timeout elapses without any readiness events triggering > > > this will return Ok(()). However we’re not guaranteeing this behaviour > > > as this depends on the OS. > > > > So I guess it is best to ignore any errors from the poll call as in your > > patch. > > Okay, I'll merge in that aspect of my original along with your other > improvements, and push something soon. Fingers crossed that we'll > finally get a green CI run today. Now in as 75979812. Still not a green CI run; but the BSD builds now work. The only remaining holdout is OpenSUSE Leap 15.5, where OCaml is stuck at 4.06 and therefore lacks List.to_seq |> Map.of_seq. I'm not sure if it is worth trying to copy the 4.07 implementations just so we can keep the two sorted name maps (one of the reasons you WANT to use a standard library implementation of Map is that writing a correct self-balancing binary tree implementation is not trivial). But do we need the resulting sets of APIs to be sorted by name? If we just stick to constructs in 4.06, we may be hit with slower O(n^2) list filtering when comparing which APIs go in which of the two lists (sync vs async), but is our list of APIs so large that it actually makes a noticeable difference to compile time? Rich, do you have any more ideas beyond Erik's recommendation [1] of just dropping OpenSUSE 15.5 and focusing only on Tumbleweed? [1] https://gitlab.com/nbdkit/libnbd/-/merge_requests/5#note_1535510343 -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Mon, Sep 04, 2023 at 06:59:15PM +, Tage Johansson wrote: > > > > +// can be both readable and writable, but we survive just fine > > > +// if we only see one direction even when both are available. > > > +poll.registry().register( > > > + SourceFd(), > > > +Token(0), > > > +MioInterest::READABLE | MioInterest::WRITABLE, > > > +)?; > > > +poll.poll( events, Some(Duration::ZERO))?; > > Why do we want 'poll.poll()?;', that is, to fail this function if the > > poll returns an error? We _expect_ poll to sometimes return an error > > (namely, the fact that it timed out) if there is nothing pending on > > the fd, at which point we WANT to successfully clear the ready_guard > > for both read and write, rather than to error out of this function. > > > You are right. I thought that the poll() call would return Ok(()) upon > timeout, but according to the documentation: > > > Currently if the timeout elapses without any readiness events triggering > > this will return Ok(()). However we’re not guaranteeing this behaviour > > as this depends on the OS. > > So I guess it is best to ignore any errors from the poll call as in your > patch. Okay, I'll merge in that aspect of my original along with your other improvements, and push something soon. Fingers crossed that we'll finally get a green CI run today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On 9/4/2023 8:23 PM, Eric Blake wrote:
On Mon, Sep 04, 2023 at 04:07:54PM +, Tage Johansson wrote:
From: Eric Blake
CI shows our async handle fails to build on FreeBSD and MacOS (where
epoll() is not available as a syscall, and therefore not available as
a Rust crate). We can instead accomplish the same level-probing
effects by doing a zero-timeout poll with mio (which defers under the
hood to epoll on Linux, and kqueue on BSD).
Fixes: 223a9965 ("rust: async: Create an async friendly handle type")
CC: Tage Johansson
Signed-off-by: Eric Blake
---
rust/Cargo.toml | 3 ++-
rust/src/async_handle.rs | 40 +++-
2 files changed, 25 insertions(+), 18 deletions(-)
-// Use epoll to check the current read/write availability on the fd.
+// Use mio poll to check the current read/write availability on the fd.
// This is needed because Tokio supports only edge-triggered
// notifications but Libnbd requires level-triggered notifications.
-let mut revent = epoll::Event { data: 0, events: 0 };
// Setting timeout to 0 means that it will return immediately.
-epoll::wait(epfd, 0, std::slice::from_mut( revent))?;
-let revents = Events::from_bits(revent.events).unwrap();
-if !revents.contains(Events::EPOLLIN) {
-ready_guard.clear_ready_matching(IoReady::READABLE);
-}
-if !revents.contains(Events::EPOLLOUT) {
-ready_guard.clear_ready_matching(IoReady::WRITABLE);
+// mio states that it is OS-dependent on whether a single event
+// can be both readable and writable, but we survive just fine
+// if we only see one direction even when both are available.
+poll.registry().register(
+ SourceFd(),
+Token(0),
+MioInterest::READABLE | MioInterest::WRITABLE,
+)?;
+poll.poll( events, Some(Duration::ZERO))?;
Why do we want 'poll.poll()?;', that is, to fail this function if the
poll returns an error? We _expect_ poll to sometimes return an error
(namely, the fact that it timed out) if there is nothing pending on
the fd, at which point we WANT to successfully clear the ready_guard
for both read and write, rather than to error out of this function.
You are right. I thought that the poll() call would return Ok(()) upon
timeout, but according to the documentation:
Currently if the timeout elapses without any readiness events
triggering this will return Ok(()). However we’re not guaranteeing
this behaviour as this depends on the OS.
So I guess it is best to ignore any errors from the poll call as in your
patch.
--
Best regards,
Tage
+for event in {
+if !event.is_readable() {
+ready_guard.clear_ready_matching(IoReady::READABLE);
+}
+if !event.is_writable() {
+ready_guard.clear_ready_matching(IoReady::WRITABLE);
+}
}
ready_guard.retain_ready();
+poll.registry().deregister( SourceFd())?;
Furthermore, if the regsiter()/deregister() should always occur in
pairs, the early poll()? exit breaks that assumption (I don't know if
Rust has enough smarts to automatically clean up the unpaired
registration).
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Mon, Sep 04, 2023 at 04:07:54PM +, Tage Johansson wrote:
> From: Eric Blake
>
> CI shows our async handle fails to build on FreeBSD and MacOS (where
> epoll() is not available as a syscall, and therefore not available as
> a Rust crate). We can instead accomplish the same level-probing
> effects by doing a zero-timeout poll with mio (which defers under the
> hood to epoll on Linux, and kqueue on BSD).
>
> Fixes: 223a9965 ("rust: async: Create an async friendly handle type")
> CC: Tage Johansson
> Signed-off-by: Eric Blake
> ---
> rust/Cargo.toml | 3 ++-
> rust/src/async_handle.rs | 40 +++-
> 2 files changed, 25 insertions(+), 18 deletions(-)
>
> -// Use epoll to check the current read/write availability on the fd.
> +// Use mio poll to check the current read/write availability on the
> fd.
> // This is needed because Tokio supports only edge-triggered
> // notifications but Libnbd requires level-triggered notifications.
> -let mut revent = epoll::Event { data: 0, events: 0 };
> // Setting timeout to 0 means that it will return immediately.
> -epoll::wait(epfd, 0, std::slice::from_mut( revent))?;
> -let revents = Events::from_bits(revent.events).unwrap();
> -if !revents.contains(Events::EPOLLIN) {
> -ready_guard.clear_ready_matching(IoReady::READABLE);
> -}
> -if !revents.contains(Events::EPOLLOUT) {
> -ready_guard.clear_ready_matching(IoReady::WRITABLE);
> +// mio states that it is OS-dependent on whether a single event
> +// can be both readable and writable, but we survive just fine
> +// if we only see one direction even when both are available.
> +poll.registry().register(
> + SourceFd(),
> +Token(0),
> +MioInterest::READABLE | MioInterest::WRITABLE,
> +)?;
> +poll.poll( events, Some(Duration::ZERO))?;
Why do we want 'poll.poll()?;', that is, to fail this function if the
poll returns an error? We _expect_ poll to sometimes return an error
(namely, the fact that it timed out) if there is nothing pending on
the fd, at which point we WANT to successfully clear the ready_guard
for both read and write, rather than to error out of this function.
> +for event in {
> +if !event.is_readable() {
> +ready_guard.clear_ready_matching(IoReady::READABLE);
> +}
> +if !event.is_writable() {
> +ready_guard.clear_ready_matching(IoReady::WRITABLE);
> +}
> }
> ready_guard.retain_ready();
> +poll.registry().deregister( SourceFd())?;
Furthermore, if the regsiter()/deregister() should always occur in
pairs, the early poll()? exit breaks that assumption (I don't know if
Rust has enough smarts to automatically clean up the unpaired
registration).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
From: Eric Blake
CI shows our async handle fails to build on FreeBSD and MacOS (where
epoll() is not available as a syscall, and therefore not available as
a Rust crate). We can instead accomplish the same level-probing
effects by doing a zero-timeout poll with mio (which defers under the
hood to epoll on Linux, and kqueue on BSD).
Fixes: 223a9965 ("rust: async: Create an async friendly handle type")
CC: Tage Johansson
Signed-off-by: Eric Blake
---
rust/Cargo.toml | 3 ++-
rust/src/async_handle.rs | 40 +++-
2 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 0879b34..678848a 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -49,9 +49,10 @@ thiserror = "1.0.40"
log = { version = "0.4.19", optional = true }
libc = "0.2.147"
tokio = { optional = true, version = "1.29.1", default-features = false,
features = ["rt", "sync", "net"] }
-epoll = "4.3.3"
+mio = { optional = true, version = "0.8.0" }
[features]
+tokio = ["dep:tokio", "dep:mio"]
default = ["log", "tokio"]
[dev-dependencies]
diff --git a/rust/src/async_handle.rs b/rust/src/async_handle.rs
index 6793ce9..8f3e8df 100644
--- a/rust/src/async_handle.rs
+++ b/rust/src/async_handle.rs
@@ -35,9 +35,11 @@ use crate::sys;
use crate::Handle;
use crate::{Error, FatalErrorKind, Result};
use crate::{AIO_DIRECTION_BOTH, AIO_DIRECTION_READ, AIO_DIRECTION_WRITE};
-use epoll::Events;
+use mio::unix::SourceFd;
+use mio::{Events, Interest as MioInterest, Poll, Token};
use std::sync::Arc;
use std::sync::Mutex;
+use std::time::Duration;
use tokio::io::{unix::AsyncFd, Interest, Ready as IoReady};
use tokio::sync::Notify;
use tokio::task;
@@ -176,13 +178,8 @@ async fn polling_task(handle_data: ) ->
Result<(), FatalErrorKind> {
} = handle_data;
let fd = handle.aio_get_fd().map_err(Error::to_fatal)?;
let tokio_fd = AsyncFd::new(fd)?;
-let epfd = epoll::create(false)?;
-epoll::ctl(
-epfd,
-epoll::ControlOptions::EPOLL_CTL_ADD,
-fd,
-epoll::Event::new(Events::EPOLLIN | Events::EPOLLOUT, 42),
-)?;
+let mut events = Events::with_capacity(1);
+let mut poll = Poll::new()?;
// The following loop does approximately the following things:
//
@@ -248,19 +245,28 @@ async fn polling_task(handle_data: ) ->
Result<(), FatalErrorKind> {
}
drop(pending_cmds_lock);
-// Use epoll to check the current read/write availability on the fd.
+// Use mio poll to check the current read/write availability on the fd.
// This is needed because Tokio supports only edge-triggered
// notifications but Libnbd requires level-triggered notifications.
-let mut revent = epoll::Event { data: 0, events: 0 };
// Setting timeout to 0 means that it will return immediately.
-epoll::wait(epfd, 0, std::slice::from_mut( revent))?;
-let revents = Events::from_bits(revent.events).unwrap();
-if !revents.contains(Events::EPOLLIN) {
-ready_guard.clear_ready_matching(IoReady::READABLE);
-}
-if !revents.contains(Events::EPOLLOUT) {
-ready_guard.clear_ready_matching(IoReady::WRITABLE);
+// mio states that it is OS-dependent on whether a single event
+// can be both readable and writable, but we survive just fine
+// if we only see one direction even when both are available.
+poll.registry().register(
+ SourceFd(),
+Token(0),
+MioInterest::READABLE | MioInterest::WRITABLE,
+)?;
+poll.poll( events, Some(Duration::ZERO))?;
+for event in {
+if !event.is_readable() {
+ready_guard.clear_ready_matching(IoReady::READABLE);
+}
+if !event.is_writable() {
+ready_guard.clear_ready_matching(IoReady::WRITABLE);
+}
}
ready_guard.retain_ready();
+poll.registry().deregister( SourceFd())?;
}
}
base-commit: 9afb980d05d6144129c899285e44779757a380e8
prerequisite-patch-id: 8d1779610795021ed5a3d0973ddf9ef854cd6a24
prerequisite-patch-id: 1f0f11f11ac9b1ff0be08f5aa0a9904ba4de
prerequisite-patch-id: dc4af7343c57a4f99dc82918c07470030e542747
prerequisite-patch-id: 8e8f7a043c80d6c24e883967f5bd952a64db1228
prerequisite-patch-id: ba7b3482e2e16f76b5f285daeeda30b31a841912
prerequisite-patch-id: 219a9595e550a8caf43d466dcb2b044114e1b7bf
prerequisite-patch-id: 3de46c9673221bff1d897970aa983b3f8e6cab74
prerequisite-patch-id: 4235d5e174fce05b9a947b3b838bebf968f0fa6a
prerequisite-patch-id: 07773355d5718e0593c4030a8f035fc11fea3715
prerequisite-patch-id: f023deea8b706706e3c2980403064d90a254af3c
prerequisite-patch-id: dd415a31fa127d90bcc7d993a2659e39d7d4cae8
prerequisite-patch-id: 618cc5f574207f36818ef803f2e586314c2458f5
prerequisite-patch-id: 8639c6cc4fec58f4761771c5d8a9476d538c6251
prerequisite-patch-id:
