Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Wed, Jan 22, 2025 at 7:36 PM Gary Guo <gary@xxxxxxxxxxx> wrote:
>
> On Thu, 16 Jan 2025 13:40:58 +0900
> FUJITA Tomonori <fujita.tomonori@xxxxxxxxx> wrote:
>
> > Add read_poll_timeout functions which poll periodically until a
> > condition is met or a timeout is reached.
> >
> > C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro
> > and a simple wrapper for Rust doesn't work. So this implements the
> > same functionality in Rust.
> >
> > The C version uses usleep_range() while the Rust version uses
> > fsleep(), which uses the best sleep method so it works with spans that
> > usleep_range() doesn't work nicely with.
> >
> > Unlike the C version, __might_sleep() is used instead of might_sleep()
> > to show proper debug info; the file name and line
> > number. might_resched() could be added to match what the C version
> > does but this function works without it.
> >
> > The sleep_before_read argument isn't supported since there is no user
> > for now. It's rarely used in the C version.
> >
> > core::panic::Location::file() doesn't provide a null-terminated string
> > so add __might_sleep_precision() helper function, which takes a
> > pointer to a string with its length.
> >
> > read_poll_timeout() can only be used in a nonatomic context. This
> > requirement is not checked by these abstractions, but it is intended
> > that klint [1] or a similar tool will be used to check it in the
> > future.
> >
> > Link: https://rust-for-linux.com/klint [1]
> > Co-developed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
> > ---
> >  include/linux/kernel.h |  2 +
> >  kernel/sched/core.c    | 28 +++++++++++---
> >  rust/helpers/helpers.c |  1 +
> >  rust/helpers/kernel.c  | 13 +++++++
> >  rust/kernel/cpu.rs     | 13 +++++++
> >  rust/kernel/error.rs   |  1 +
> >  rust/kernel/io.rs      |  5 +++
> >  rust/kernel/io/poll.rs | 84 ++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs     |  2 +
> >  9 files changed, 144 insertions(+), 5 deletions(-)
> >  create mode 100644 rust/helpers/kernel.c
> >  create mode 100644 rust/kernel/cpu.rs
> >  create mode 100644 rust/kernel/io.rs
> >  create mode 100644 rust/kernel/io/poll.rs
> >
> > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> > new file mode 100644
> > index 000000000000..033f3c4e4adf
> > --- /dev/null
> > +++ b/rust/kernel/io.rs
> > @@ -0,0 +1,5 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Input and Output.
> > +
> > +pub mod poll;
> > diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> > new file mode 100644
> > index 000000000000..da8e975d8e50
> > --- /dev/null
> > +++ b/rust/kernel/io/poll.rs
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! IO polling.
> > +//!
> > +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> > +
> > +use crate::{
> > +    cpu::cpu_relax,
> > +    error::{code::*, Result},
> > +    time::{delay::fsleep, Delta, Instant},
> > +};
> > +
> > +use core::panic::Location;
> > +
> > +/// Polls periodically until a condition is met or a timeout is reached.
> > +///
> > +/// Public but hidden since it should only be used from public macros.
> > +///
> > +/// ```rust
> > +/// use kernel::io::poll::read_poll_timeout;
> > +/// use kernel::time::Delta;
> > +/// use kernel::sync::{SpinLock, new_spinlock};
> > +///
> > +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> > +/// let g = lock.lock();
> > +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Delta::from_micros(42));
> > +/// drop(g);
> > +///
> > +/// # Ok::<(), Error>(())
> > +/// ```
> > +#[track_caller]
> > +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>
> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
> `&T` instead. I can see this being useful in many cases.
>
> I know that quite often `T` is just an integer so you'd want to pass by
> value, but I think almost always `Cond` is a very simple closure so
> inlining would take place and they won't make a performance difference.

Yeah, I think it should be

Cond: FnMut(&T) -> bool,

with FnMut as well.

Alice