Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Alice Ryhl <aliceryhl@google.com>]

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
>