Message-Id: <20250814.151147.29094382820492173.fujita.tomonori@gmail.com>
Date: Thu, 14 Aug 2025 15:11:47 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: aliceryhl@...gle.com
Cc: fujita.tomonori@...il.com, 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,
 me@...enk.dev
Subject: Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

On Mon, 11 Aug 2025 09:50:59 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:

> 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.

Do you mean that, since it’s obvious might_sleep() can only be used in
a non-atomic context, the above statement is redundant and can be
dropped?

>> +#[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>

Surely, I'll do.

> 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.

Understood, I'll.

> 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.)

I'll remove the Option and let's see if we’ll need a version without a
timeout.


>> +{
>> +    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.

I'll in the next version.

Powered by blists - more mailing lists