Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 16 Oct 2024 10:37:46 +0200
Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:

> On Wed, Oct 16, 2024 at 5:54 AM 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.
>>
>> For the proper debug info, readx_poll_timeout() is implemented as a
>> macro.
>>
>> readx_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.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
>> Link: https://rust-for-linux.com/klint [1]
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
> 
> Duplicated SOB? This should just be:
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>

Oops, I'll fix.

>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// Public but hidden since it should only be used from public macros.
>> +#[doc(hidden)]
>> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>> +    mut op: Op,
>> +    cond: Cond,
>> +    sleep_delta: Delta,
>> +    timeout_delta: Delta,
>> +    sleep_before_read: bool,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: Fn(T) -> bool,
>> +{
>> +    let timeout = Ktime::ktime_get() + timeout_delta;
>> +    let sleep = !sleep_delta.is_zero();
>> +
>> +    if sleep_before_read && sleep {
>> +        fsleep(sleep_delta);
>> +    }
> 
> You always pass `false` for `sleep_before_read` so perhaps just remove
> this and the argument entirely?

Most of users of C's this function use false buf some use true. It
would be better to provide this option?

Would be better to provide only read_poll_timeout() which takes the
sleep_before_read argument?; No readx_poll_timeout wrapper.

>> +        if cond(val) {
>> +            break val;
>> +        }
> 
> This breaks out to another cond(val) check below. Perhaps just `return
> Ok(val)` here to avoid the double condition check?

I think that that's how the C version works. But `return Ok(val)` here
is fine, I guess.

>> +        if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout {
>> +            break op()?;
>> +        }
> 
> Shouldn't you just return `Err(ETIMEDOUT)` here? I don't think you're
> supposed to call `op` twice without a sleep in between.

I think that it's how the C version works.

>> +        if sleep {
>> +            fsleep(sleep_delta);
>> +        }
>> +        // SAFETY: FFI call.
>> +        unsafe { bindings::cpu_relax() }
> 
> Should cpu_relax() be in an else branch?

I think that we could. I'll remove if you prefer. The C version always
calls cpu_relax(). I think that the following commit explains why:

commit b407460ee99033503993ac7437d593451fcdfe44
Author: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
Date:   Fri Jun 2 10:50:36 2023 +0200

    iopoll: Call cpu_relax() in busy loops


> Also, please add a safe wrapper function around cpu_relax.

Sure, which file do you think is best to add the wrapper to?

rust/kernel/processor.rs

?

>> +macro_rules! readx_poll_timeout {
>> +    ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{
>> +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
>> +        if !$sleep_delta.is_zero() {
>> +            // SAFETY: FFI call.
>> +            unsafe {
>> +                $crate::bindings::__might_sleep(
>> +                    ::core::file!().as_ptr() as *const i8,
>> +                    ::core::line!() as i32,
>> +                )
>> +            }
>> +        }
> 
> It could be nice to introduce a might_sleep macro that does this
> internally? Then we can reuse this logic in other places.

I think that with #[track_caller], we can use a normal function
instead of a macro.

Using might_sleep name for a wrapper of __might_sleep is confusing
since the C side has also might_sleep. __foo function name is
acceptable?