Message-ID: <87y0wx9hpk.fsf@kernel.org>
Date: Sat, 22 Mar 2025 17:02:31 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: linux-kernel@...r.kernel.org,  Daniel Almeida
 <daniel.almeida@...labora.com>,  rust-for-linux@...r.kernel.org,
  netdev@...r.kernel.org,  andrew@...n.ch,  hkallweit1@...il.com,
  tmgross@...ch.edu,  ojeda@...nel.org,  alex.gaynor@...il.com,
  gary@...yguo.net,  bjorn3_gh@...tonmail.com,  benno.lossin@...ton.me,
  a.hindborg@...sung.com,  aliceryhl@...gle.com,  anna-maria@...utronix.de,
  frederic@...nel.org,  tglx@...utronix.de,  arnd@...db.de,
  jstultz@...gle.com,  sboyd@...nel.org,  mingo@...hat.com,
  peterz@...radead.org,  juri.lelli@...hat.com,
  vincent.guittot@...aro.org,  dietmar.eggemann@....com,
  rostedt@...dmis.org,  bsegall@...gle.com,  mgorman@...e.de,
  vschneid@...hat.com,  tgunders@...hat.com,  me@...enk.dev,
  david.laight.linux@...il.com
Subject: Re: [PATCH v11 7/8] rust: Add read_poll_timeout functions

Hi Tomonori,

FUJITA Tomonori <fujita.tomonori@...il.com> writes:

> 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.
>
> Link: https://rust-for-linux.com/klint [1]
> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---
>  rust/helpers/helpers.c |   1 +
>  rust/helpers/kernel.c  |  18 +++++++
>  rust/kernel/cpu.rs     |  13 +++++
>  rust/kernel/error.rs   |   1 +
>  rust/kernel/io.rs      |   2 +
>  rust/kernel/io/poll.rs | 120 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  7 files changed, 156 insertions(+)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/kernel/cpu.rs
>  create mode 100644 rust/kernel/io/poll.rs
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 9565485a1a54..16d256897ccb 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -14,6 +14,7 @@
>  #include "cred.c"
>  #include "device.c"
>  #include "err.c"
> +#include "kernel.c"
>  #include "fs.c"
>  #include "io.c"
>  #include "jump_label.c"
> diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
> new file mode 100644
> index 000000000000..f04c04d4cc4f
> --- /dev/null
> +++ b/rust/helpers/kernel.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kernel.h>
> +
> +void rust_helper_cpu_relax(void)
> +{
> +	cpu_relax();
> +}
> +
> +void rust_helper___might_sleep_precision(const char *file, int len, int line)
> +{
> +	__might_sleep_precision(file, len, line);
> +}
> +
> +void rust_helper_might_resched(void)
> +{
> +	might_resched();
> +}
> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
> new file mode 100644
> index 000000000000..eeeff4be84fa
> --- /dev/null
> +++ b/rust/kernel/cpu.rs
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Processor related primitives.
> +//!
> +//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h).
> +
> +/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
> +///
> +/// It also happens to serve as a compiler barrier.
> +pub fn cpu_relax() {
> +    // SAFETY: FFI call.

I don't think this safety comment is sufficient. There are two other
similar comments further down.

> +    unsafe { bindings::cpu_relax() }
> +}
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index f6ecf09cb65f..8858eb13b3df 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -64,6 +64,7 @@ macro_rules! declare_err {
>      declare_err!(EPIPE, "Broken pipe.");
>      declare_err!(EDOM, "Math argument out of domain of func.");
>      declare_err!(ERANGE, "Math result not representable.");
> +    declare_err!(ETIMEDOUT, "Connection timed out.");
>      declare_err!(ERESTARTSYS, "Restart the system call.");
>      declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
>      declare_err!(ERESTARTNOHAND, "Restart if no handler.");
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee..be63742f517b 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
>  use crate::error::{code::EINVAL, Result};
>  use crate::{bindings, build_assert};
>  
> +pub mod poll;
> +
>  /// Raw representation of an MMIO region.
>  ///
>  /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// 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},
> +};
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.
> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.
> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore
> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
> +///     // The `op` closure reads the value of a specific status register.
> +///     let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +///     // The `cond` closure takes a reference to the value returned by `op`
> +///     // and checks whether the hardware is ready.
> +///     let cond = |val: &u16| *val == HW_READY;
> +///
> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```
> +///
> +/// ```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), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```

I am guessing this example is present to test the call to `might_sleep`.
Could you document the reason for the test. As an example, this code is
not really usable. `#[test]` was staged for 6.15, so perhaps move this
to a unit test instead?

The test throws this BUG, which is what I think is also your intention:

  BUG: sleeping function called from invalid context at rust/doctests_kernel_generated.rs:3523
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 171, name: kunit_try_catch
  preempt_count: 1, expected: 0
  RCU nest depth: 0, expected: 0
  1 lock held by kunit_try_catch/171:
  #0: ffff8881003ce598 (rust/doctests_kernel_generated.rs:3521){+.+.}-{3:3}, at: rust_helper_spin_lock+0xd/0x10
  CPU: 0 UID: 0 PID: 171 Comm: kunit_try_catch Tainted: G                 N 6.14.0-rc7+ #14
  Tainted: [N]=TEST
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
  Call Trace:
  <TASK>
  dump_stack_lvl+0x7b/0xa0
  dump_stack+0x14/0x16
  __might_resched_precision+0x22f/0x240
  __might_sleep_precision+0x39/0x70
  _RNvNtNtCs1cdwasc6FUb_6kernel2io4poll11might_sleep+0x19/0x20
  rust_doctest_kernel_io_poll_rs_0+0xa5/0x1f0
  kunit_try_run_case+0x73/0x150
  ? trace_hardirqs_on+0x5a/0x90
  kunit_generic_run_threadfn_adapter+0x1a/0x30
  kthread+0x219/0x230
  ? kunit_try_catch_run+0x230/0x230
  ? __do_trace_sched_kthread_stop_ret+0x50/0x50
  ret_from_fork+0x35/0x40
  ? __do_trace_sched_kthread_stop_ret+0x50/0x50
  ret_from_fork_asm+0x11/0x20
  </TASK>

Kunit does not pick this up as a failure, but it _should_, and hopefully
it will soon (TM).

So, we should probably expect failure when we get that fixed. And
perhaps for now disable the test or add a TODO to change to expect fail
when we fix kunit.


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists