Message-ID: <Z3Lp6Ce0YJa5yEYZ@boqun-archlinux>
Date: Mon, 30 Dec 2024 10:43:52 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: linux-kernel@...r.kernel.org, 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
Subject: Re: [PATCH v7 6/7] rust: Add read_poll_timeout functions

On Fri, Dec 20, 2024 at 03:18:52PM +0900, FUJITA Tomonori 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.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> core::panic::Location::file() doesn't provide a null-terminated string
> so add __might_sleep_precision() helper function, which takes a
> pointer to a string with its length.
> 
> 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]
> Co-developed-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---
>  include/linux/kernel.h |  2 +
>  kernel/sched/core.c    | 27 +++++++++++---
>  rust/helpers/helpers.c |  1 +
>  rust/helpers/kernel.c  | 13 +++++++
>  rust/kernel/cpu.rs     | 13 +++++++
>  rust/kernel/error.rs   |  1 +
>  rust/kernel/io.rs      |  5 +++
>  rust/kernel/io/poll.rs | 84 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |  2 +
>  9 files changed, 143 insertions(+), 5 deletions(-)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/kernel/cpu.rs
>  create mode 100644 rust/kernel/io.rs
>  create mode 100644 rust/kernel/io/poll.rs
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index be2e8c0a187e..086ee1dc447e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>  extern void __might_resched(const char *file, int line, unsigned int offsets);
>  extern void __might_sleep(const char *file, int line);
> +extern void __might_sleep_precision(const char *file, int len, int line);
>  extern void __cant_sleep(const char *file, int line, int preempt_offset);
>  extern void __cant_migrate(const char *file, int line);
>  
> @@ -145,6 +146,7 @@ extern void __cant_migrate(const char *file, int line);
>    static inline void __might_resched(const char *file, int line,
>  				     unsigned int offsets) { }
>  static inline void __might_sleep(const char *file, int line) { }
> +static inline void __might_sleep_precision(const char *file, int len, int line) { }
>  # define might_sleep() do { might_resched(); } while (0)
>  # define cant_sleep() do { } while (0)
>  # define cant_migrate()		do { } while (0)
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 3e5a6bf587f9..6ed70c801172 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8670,7 +8670,10 @@ void __init sched_init(void)
>  
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>  
> -void __might_sleep(const char *file, int line)
> +extern inline void __might_resched_precision(const char *file, int len,
> +					     int line, unsigned int offsets);

Instead of declaring this as an "extern inline" I think you can just
make it a static inline and move the function body up here. It should
resolve the sparse warning.

Regards,
Boqun

> +
> +void __might_sleep_precision(const char *file, int len, int line)
>  {
>  	unsigned int state = get_current_state();
>  	/*
> @@ -8684,7 +8687,14 @@ void __might_sleep(const char *file, int line)
>  			(void *)current->task_state_change,
>  			(void *)current->task_state_change);
>  
> -	__might_resched(file, line, 0);
> +	__might_resched_precision(file, len, line, 0);
> +}
> +
> +void __might_sleep(const char *file, int line)
> +{
> +	long len = strlen(file);
> +
> +	__might_sleep_precision(file, len, line);
>  }
>  EXPORT_SYMBOL(__might_sleep);
>  
> @@ -8709,7 +8719,7 @@ static inline bool resched_offsets_ok(unsigned int offsets)
>  	return nested == offsets;
>  }
>  
> -void __might_resched(const char *file, int line, unsigned int offsets)
> +void __might_resched_precision(const char *file, int len, int line, unsigned int offsets)
>  {
>  	/* Ratelimiting timestamp: */
>  	static unsigned long prev_jiffy;
> @@ -8732,8 +8742,8 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	/* Save this before calling printk(), since that will clobber it: */
>  	preempt_disable_ip = get_preempt_disable_ip(current);
>  
> -	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
> -	       file, line);
> +	pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
> +	       len, file, line);
>  	pr_err("in_atomic(): %d, irqs_disabled(): %d, non_block: %d, pid: %d, name: %s\n",
>  	       in_atomic(), irqs_disabled(), current->non_block_count,
>  	       current->pid, current->comm);
> @@ -8758,6 +8768,13 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	dump_stack();
>  	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
>  }
> +
> +void __might_resched(const char *file, int line, unsigned int offsets)
> +{
> +	long len = strlen(file);
> +
> +	__might_resched_precision(file, len, line, offsets);
> +}
>  EXPORT_SYMBOL(__might_resched);
>  
>  void __cant_sleep(const char *file, int line, int preempt_offset)
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index d16aeda7a558..7ab71a6d4603 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
>  #include "build_bug.c"
>  #include "cred.c"
>  #include "err.c"
> +#include "kernel.c"
>  #include "fs.c"
>  #include "jump_label.c"
>  #include "kunit.c"
> diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
> new file mode 100644
> index 000000000000..9dff28f4618e
> --- /dev/null
> +++ b/rust/helpers/kernel.c
> @@ -0,0 +1,13 @@
> +// 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);
> +}
> 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.
> +    unsafe { bindings::cpu_relax() }
> +}
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 914e8dec1abd..b5016083a115 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -63,6 +63,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
> new file mode 100644
> index 000000000000..033f3c4e4adf
> --- /dev/null
> +++ b/rust/kernel/io.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Input and Output.
> +
> +pub mod poll;
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..da8e975d8e50
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,84 @@
> +// 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},
> +};
> +
> +use core::panic::Location;
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// Public but hidden since it should only be used from public macros.
> +///
> +/// ```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), Delta::from_micros(42));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> +    mut op: Op,
> +    cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Delta,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: Fn(T) -> bool,
> +{
> +    let start = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +    let timeout = !timeout_delta.is_zero();
> +
> +    might_sleep(Location::caller());
> +
> +    let val = loop {
> +        let val = op()?;
> +        if cond(val) {
> +            // Unlike the C version, we immediately return.
> +            // We know a condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if timeout && start.elapsed() > timeout_delta {
> +            // Should we return Err(ETIMEDOUT) here instead of call op() again
> +            // without a sleep between? But we follow the C version. op() could
> +            // take some time so might be worth checking again.
> +            break op()?;
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    };
> +
> +    if cond(val) {
> +        Ok(val)
> +    } else {
> +        Err(ETIMEDOUT)
> +    }
> +}
> +
> +fn might_sleep(loc: &Location<'_>) {
> +    // SAFETY: FFI call.
> +    unsafe {
> +        crate::bindings::__might_sleep_precision(
> +            loc.file().as_ptr().cast(),
> +            loc.file().len() as i32,
> +            loc.line() as i32,
> +        )
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e1065a7551a3..2e9722f980bd 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -33,6 +33,7 @@
>  #[cfg(CONFIG_BLOCK)]
>  pub mod block;
>  mod build_assert;
> +pub mod cpu;
>  pub mod cred;
>  pub mod device;
>  pub mod error;
> @@ -40,6 +41,7 @@
>  pub mod firmware;
>  pub mod fs;
>  pub mod init;
> +pub mod io;
>  pub mod ioctl;
>  pub mod jump_label;
>  #[cfg(CONFIG_KUNIT)]
> -- 
> 2.43.0
> 
> 

Powered by blists - more mailing lists