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@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 include/linux/kernel.h |  2 +
 kernel/sched/core.c    | 28 +++++++++++---
 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, 144 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..d9ac66dc66d3 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)
+static void __might_resched_precision(const char *file, int len,
+				      int line, unsigned int offsets);
+
+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,8 @@ static inline bool resched_offsets_ok(unsigned int offsets)
 	return nested == offsets;
 }
 
-void __might_resched(const char *file, int line, unsigned int offsets)
+static void __might_resched_precision(const char *file, int len, int line,
+				      unsigned int offsets)
 {
 	/* Ratelimiting timestamp: */
 	static unsigned long prev_jiffy;
@@ -8732,8 +8743,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 +8769,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 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
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 545d1170ee63..c477701b2efa 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -35,6 +35,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod cpu;
 pub mod cred;
 pub mod device;
 pub mod error;
@@ -42,6 +43,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

On Thu, 16 Jan 2025 13:40:58 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> 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@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  include/linux/kernel.h |  2 +
>  kernel/sched/core.c    | 28 +++++++++++---
>  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, 144 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/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>(

I wonder if we can lift the `T: Copy` restriction and have `Cond` take
`&T` instead. I can see this being useful in many cases.

I know that quite often `T` is just an integer so you'd want to pass by
value, but I think almost always `Cond` is a very simple closure so
inlining would take place and they won't make a performance difference.

> +    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());

This should only be called if `timeout` is true?

> +
> +    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()?;

Maybe the reason is `ktime_get` can take some time (due to its use of
seqlock and thus may require retrying?) Although this logic breaks down
when `read_poll_timeout_atomic` also has this extra `op(args)` despite
the condition being trivial.

So I really can't convince myself that this additional `op()` call is
needed. I can't think of any case where this behaviour would be
depended on by a driver, so I'd be tempted just to return ETIMEOUT
straight.

Regardless, even if you decide to keep it, you can hoist the `if
cond(val)` block below up or move the `op()` down, given that this is
the only place to break from the loop without returning.

> +        }
> +        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 545d1170ee63..c477701b2efa 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -35,6 +35,7 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +pub mod cpu;
>  pub mod cred;
>  pub mod device;
>  pub mod error;
> @@ -42,6 +43,7 @@
>  pub mod firmware;
>  pub mod fs;
>  pub mod init;
> +pub mod io;
>  pub mod ioctl;
>  pub mod jump_label;
>  #[cfg(CONFIG_KUNIT)]

On Wed, 22 Jan 2025 18:36:12 +0000
Gary Guo <gary@garyguo.net> wrote:

>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> 
> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
> `&T` instead. I can see this being useful in many cases.
> 
> I know that quite often `T` is just an integer so you'd want to pass by
> value, but I think almost always `Cond` is a very simple closure so
> inlining would take place and they won't make a performance difference.

Yeah, we can. More handy for the users of this function. I'll do.

>> +    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());
> 
> This should only be called if `timeout` is true?

Oops, I messed up this in v6 somehow. I'll fix.

>> +    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()?;
> 
> Maybe the reason is `ktime_get` can take some time (due to its use of
> seqlock and thus may require retrying?) Although this logic breaks down
> when `read_poll_timeout_atomic` also has this extra `op(args)` despite
> the condition being trivial.

ktime_get() might do retrying (read_seqcount) but compared to the op
function, I think that ktime_get() is fast (usually an op function
waits for hardware).

> So I really can't convince myself that this additional `op()` call is
> needed. I can't think of any case where this behaviour would be
> depended on by a driver, so I'd be tempted just to return ETIMEOUT
> straight.

As I commented in the code, I just mimic the logic of the C version,
which has been used for a long time. But as you said, looks like we
can return Err(ETIMEOUT) immediately here. I'll do that in the next
version.

On Wed, Jan 22, 2025 at 7:36 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Thu, 16 Jan 2025 13:40:58 +0900
> FUJITA Tomonori <fujita.tomonori@gmail.com> 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@gmail.com>
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > ---
> >  include/linux/kernel.h |  2 +
> >  kernel/sched/core.c    | 28 +++++++++++---
> >  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, 144 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/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>(
>
> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
> `&T` instead. I can see this being useful in many cases.
>
> I know that quite often `T` is just an integer so you'd want to pass by
> value, but I think almost always `Cond` is a very simple closure so
> inlining would take place and they won't make a performance difference.

Yeah, I think it should be

Cond: FnMut(&T) -> bool,

with FnMut as well.

Alice

On Wed, 22 Jan 2025 21:14:00 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> > +#[track_caller]
>> > +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>>
>> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
>> `&T` instead. I can see this being useful in many cases.
>>
>> I know that quite often `T` is just an integer so you'd want to pass by
>> value, but I think almost always `Cond` is a very simple closure so
>> inlining would take place and they won't make a performance difference.
> 
> Yeah, I think it should be
> 
> Cond: FnMut(&T) -> bool,
> 
> with FnMut as well.

Yeah, less restriction is better. I changed the code as follows:

#[track_caller]
pub fn read_poll_timeout<Op, Cond, T>(
    mut op: Op,
    mut cond: Cond,
    sleep_delta: Delta,
    timeout_delta: Delta,
) -> Result<T>
where
    Op: FnMut() -> Result<T>,
    Cond: FnMut(&T) -> bool,
{

On Thu, Jan 16, 2025 at 5:43 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> 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@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

[...]

> +void __might_sleep(const char *file, int line)
> +{
> +       long len = strlen(file);
> +
> +       __might_sleep_precision(file, len, line);
>  }
>  EXPORT_SYMBOL(__might_sleep);

I think these strlen() calls could be pretty expensive. You run them
every time might_sleep() runs even if the check does not fail.

How about changing __might_resched_precision() to accept a length of
-1 for nul-terminated strings, and having it compute the length with
strlen only *if* we know that we actually need the length?

if (len < 0) len = strlen(file);
pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
       len, file, line);

Another option might be to compile the lengths at compile-time by
having the macros use sizeof on __FILE__, but that sounds more tricky
to get right.

Alice

On Thu, 16 Jan 2025 10:45:00 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> +void __might_sleep(const char *file, int line)
>> +{
>> +       long len = strlen(file);
>> +
>> +       __might_sleep_precision(file, len, line);
>>  }
>>  EXPORT_SYMBOL(__might_sleep);
> 
> I think these strlen() calls could be pretty expensive. You run them
> every time might_sleep() runs even if the check does not fail.

Ah, yes.

> How about changing __might_resched_precision() to accept a length of
> -1 for nul-terminated strings, and having it compute the length with
> strlen only *if* we know that we actually need the length?
> 
> if (len < 0) len = strlen(file);
> pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
>        len, file, line);

Works for me.

> Another option might be to compile the lengths at compile-time by
> having the macros use sizeof on __FILE__, but that sounds more tricky
> to get right.

Yeah.

By the way, from what I saw in the discussion about Location::file(),
I got the impression that the feature for a null-terminated string
seems likely to be supported in the near future. Am I correct?

If so, rather than adding a Rust-specific helper function to the C
side, it would be better to solve the problem on the Rust side like
the previous versions with c_str()! and file()! for now?

On Thu, Jan 16, 2025 at 12:32 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 16 Jan 2025 10:45:00 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> +void __might_sleep(const char *file, int line)
> >> +{
> >> +       long len = strlen(file);
> >> +
> >> +       __might_sleep_precision(file, len, line);
> >>  }
> >>  EXPORT_SYMBOL(__might_sleep);
> >
> > I think these strlen() calls could be pretty expensive. You run them
> > every time might_sleep() runs even if the check does not fail.
>
> Ah, yes.
>
> > How about changing __might_resched_precision() to accept a length of
> > -1 for nul-terminated strings, and having it compute the length with
> > strlen only *if* we know that we actually need the length?
> >
> > if (len < 0) len = strlen(file);
> > pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
> >        len, file, line);
>
> Works for me.
>
> > Another option might be to compile the lengths at compile-time by
> > having the macros use sizeof on __FILE__, but that sounds more tricky
> > to get right.
>
> Yeah.
>
> By the way, from what I saw in the discussion about Location::file(),
> I got the impression that the feature for a null-terminated string
> seems likely to be supported in the near future. Am I correct?

There's a good chance, but it's also not guaranteed.

> If so, rather than adding a Rust-specific helper function to the C
> side, it would be better to solve the problem on the Rust side like
> the previous versions with c_str()! and file()! for now?

I would be okay with a scenario where older compilers just reference
the read_poll_timeout() function in the error message, and only newer
compilers reference the location of the caller. Of course, right now,
only older compilers exist. But if we don't get nul-terminated
location strings, then I do think we should make the change you're
currently making.

Alice

On Thu, 16 Jan 2025 12:42:57 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> If so, rather than adding a Rust-specific helper function to the C
>> side, it would be better to solve the problem on the Rust side like
>> the previous versions with c_str()! and file()! for now?
> 
> I would be okay with a scenario where older compilers just reference
> the read_poll_timeout() function in the error message, and only newer
> compilers reference the location of the caller. Of course, right now,
> only older compilers exist. But if we don't get nul-terminated
> location strings, then I do think we should make the change you're
> currently making.

Okay, let's see if we can get ACK from the scheduler maintainers with
your version, which has less impact on the C code.

On Thu, Jan 16, 2025 at 12:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 16 Jan 2025 12:42:57 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> If so, rather than adding a Rust-specific helper function to the C
> >> side, it would be better to solve the problem on the Rust side like
> >> the previous versions with c_str()! and file()! for now?
> >
> > I would be okay with a scenario where older compilers just reference
> > the read_poll_timeout() function in the error message, and only newer
> > compilers reference the location of the caller. Of course, right now,
> > only older compilers exist. But if we don't get nul-terminated
> > location strings, then I do think we should make the change you're
> > currently making.
>
> Okay, let's see if we can get ACK from the scheduler maintainers with
> your version, which has less impact on the C code.

You might want to split the might_sleep() changes into its own commit
to make it harder to miss. Right now, the title looks like something
that isn't changing the C side.

Alice