Message-ID: <ZyvhDbNAhPTqIoVi@boqun-archlinux>
Date: Wed, 6 Nov 2024 13:35:09 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: anna-maria@...utronix.de, frederic@...nel.org, tglx@...utronix.de,
	jstultz@...gle.com, sboyd@...nel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, rust-for-linux@...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, arnd@...db.de,
	Petr Mladek <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v5 6/7] rust: Add read_poll_timeout functions

On Fri, Nov 01, 2024 at 10:01:20AM +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.
> 
> For the proper debug info, readx_poll_timeout() and __might_sleep()
> are implemented as a macro. We could implement them as a normal
> function if there is a clean way to get a null-terminated string
> without allocation from core::panic::Location::file().
> 

So printk() actually support printing a string with a precison value,
that is: a format string "%.*s" would take two inputs, one for the length
and the other for the pointer to the string, for example you can do:

	char *msg = "hello";

	printk("%.*s\n", 5, msg);

This is similar to printf() in glibc [1].

If we add another __might_sleep_precision() which accepts a file name
length:

	void __might_sleep_precision(const char *file, int len, int line)

then we don't need to use macro here, I've attached a diff based
on your whole patchset, and it seems working.

Cc printk folks to if they know any limitation on using precision.

Regards,
Boqun

[1]: https://www.gnu.org/software/libc/manual/html_node/Output-Conversion-Syntax.html#Output-Conversion-Syntax

> 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.
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---

--------------------------------------------->8
diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index dabb772c468f..4d368ce80db6 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -18,7 +18,7 @@
     Driver,
 };
 use kernel::prelude::*;
-use kernel::readx_poll_timeout;
+use kernel::read_poll_timeout;
 use kernel::sizes::{SZ_16K, SZ_8K};
 use kernel::time::Delta;
 
@@ -95,7 +95,7 @@ fn probe(dev: &mut phy::Device) -> Result<()> {
         // The micro-controller will start running from SRAM.
         dev.write(C45::new(Mmd::PCS, 0xe854), 0x0040)?;
 
-        readx_poll_timeout!(
+        read_poll_timeout(
             || dev.read(C45::new(Mmd::PCS, 0xd7fd)),
             |val| val != 0x00 && val != 0x10,
             Delta::from_millis(50),
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index be2e8c0a187e..b405b0d19bac 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -87,6 +87,8 @@ 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_resched_precision(const char *file, int len, int line, unsigned int offsets);
+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);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..f872aa18eaf0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8543,7 +8543,7 @@ void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
+void __might_sleep_precision(const char *file, int len, int line)
 {
 	unsigned int state = get_current_state();
 	/*
@@ -8557,7 +8557,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);
 
@@ -8582,7 +8589,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;
@@ -8605,8 +8612,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);
@@ -8631,6 +8638,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/kernel.c b/rust/helpers/kernel.c
index da847059260b..9dff28f4618e 100644
--- a/rust/helpers/kernel.c
+++ b/rust/helpers/kernel.c
@@ -7,7 +7,7 @@ void rust_helper_cpu_relax(void)
 	cpu_relax();
 }
 
-void rust_helper___might_sleep(const char *file, int line)
+void rust_helper___might_sleep_precision(const char *file, int len, int line)
 {
-	__might_sleep(file, line);
+	__might_sleep_precision(file, len, line);
 }
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index a8caa08f86f2..d7e5be162b6e 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -10,10 +10,25 @@
     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.
-#[doc(hidden)]
+///
+/// ```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,
@@ -28,6 +43,8 @@ pub fn read_poll_timeout<Op, Cond, T: Copy>(
     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) {
@@ -55,41 +72,13 @@ pub fn read_poll_timeout<Op, Cond, T: Copy>(
     }
 }
 
-/// Print debug information if it's called inside atomic sections.
-///
-/// Equivalent to the kernel's [`__might_sleep`].
-#[macro_export]
-macro_rules! __might_sleep {
-    () => {
-        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
-        // SAFETY: FFI call.
-        unsafe {
-            $crate::bindings::__might_sleep(
-                c_str!(::core::file!()).as_char_ptr(),
-                ::core::line!() as i32,
-            )
-        }
-    };
-}
-
-/// Polls periodically until a condition is met or a timeout is reached.
-///
-/// `op` is called repeatedly until `cond` returns `true` or the timeout is
-///  reached. The return value of `op` is passed to `cond`.
-///
-/// `sleep_delta` is the duration to sleep between calls to `op`.
-/// If `sleep_delta` is less than one microsecond, the function will busy-wait.
-///
-/// `timeout_delta` is the maximum time to wait for `cond` to return `true`.
-///
-/// This macro can only be used in a nonatomic context.
-#[macro_export]
-macro_rules! readx_poll_timeout {
-    ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{
-        if !$sleep_delta.is_zero() {
-            $crate::__might_sleep!();
-        }
-
-        $crate::io::poll::read_poll_timeout($op, $cond, $sleep_delta, $timeout_delta)
-    }};
+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,
+        )
+    }
 }

Powered by blists - more mailing lists