linux-kernel - Re: [PATCH v1 2/2] rust: Add read_poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJm9A_D-zlJtbV6X@google.com>
Date: Mon, 11 Aug 2025 09:50:59 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: a.hindborg@...nel.org, alex.gaynor@...il.com, ojeda@...nel.org, 
	anna-maria@...utronix.de, bjorn3_gh@...tonmail.com, boqun.feng@...il.com, 
	dakr@...nel.org, frederic@...nel.org, gary@...yguo.net, jstultz@...gle.com, 
	linux-kernel@...r.kernel.org, lossin@...nel.org, lyude@...hat.com, 
	rust-for-linux@...r.kernel.org, sboyd@...nel.org, tglx@...utronix.de, 
	tmgross@...ch.edu, acourbot@...dia.com, daniel.almeida@...labora.com, 
	Fiona Behrens <me@...enk.dev>
Subject: Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

On Mon, Aug 11, 2025 at 01:10:38PM +0900, FUJITA Tomonori wrote:
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
> 
> The 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.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> 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.

I would drop this paragraph. You have a call to might_sleep() now.

> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Option<Delta>,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,

I would consider just writing this as:

pub fn read_poll_timeout<T>(
    mut op: impl FnMut() -> Result<T>,
    mut cond: impl FnMut(&T) -> bool,
    sleep_delta: Delta,
    timeout_delta: Option<Delta>,
) -> Result<T>

And I would also consider adding a new error type called TimeoutError
similar to BadFdError in `rust/kernel/fs/file.rs`. That way, we promise
to the caller that we never return error codes other than a timeout.

Another thing is the `timeout_delta` option. I would just have written
it as two methods, one that takes a timeout and one that doesn't. That
way, callers that don't need a timeout do not need to handle timeout
errors. (Do we have any users without a timeout? If not, maybe just
remove the Option.)

> +{
> +    let start: Instant<Monotonic> = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +
> +    // Unlike the C version, we always call `might_sleep()`.
> +    might_sleep();
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if let Some(timeout_delta) = timeout_delta {
> +            if start.elapsed() > timeout_delta {
> +                // Unlike the C version, we immediately return.
> +                // We have just called `op()` so we don't need to call it again.
> +                return Err(ETIMEDOUT);
> +            }
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }

I would just do:

if !sleep_delta.is_zero() {
    fsleep(sleep_delta);
}

instead of the extra variable.

> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> -- 
> 2.43.0
>