[v9,1/8] sched/core: Add __might_sleep_precision()

Commit Message

FUJITA Tomonori Jan. 25, 2025, 10:18 a.m. UTC

Add __might_sleep_precision(), Rust friendly version of
__might_sleep(), which takes a pointer to a string with the length
instead of a null-terminated string.

Rust's core::panic::Location::file(), which gives the file name of a
caller, doesn't provide a null-terminated
string. __might_sleep_precision() uses a precision specifier in the
printk format, which specifies the length of a string; a string
doesn't need to be a null-terminated.

Modify __might_sleep() to call __might_sleep_precision() but the
impact should be negligible. strlen() isn't called in a normal case;
it's called only when printing the error (sleeping function called
from invalid context).

Note that Location::file() providing a null-terminated string for
better C interoperability is under discussion [1].

[1]: https://github.com/rust-lang/libs-team/issues/466
Co-developed-by: Boqun Feng <[e-mail]>
Signed-off-by: Boqun Feng <[e-mail]>
Signed-off-by: FUJITA Tomonori <[e-mail]>
---
 include/linux/kernel.h |  2 ++
 kernel/sched/core.c    | 55 ++++++++++++++++++++++++++----------------
 2 files changed, 36 insertions(+), 21 deletions(-)

Comments

Alice Ryhl Jan. 27, 2025, 9:41 a.m. UTC | #1

On Sat, Jan 25, 2025 at 11:20 AM FUJITA Tomonori
<[e-mail]> wrote:
>
> Add __might_sleep_precision(), Rust friendly version of
> __might_sleep(), which takes a pointer to a string with the length
> instead of a null-terminated string.
>
> Rust's core::panic::Location::file(), which gives the file name of a
> caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
>
> Modify __might_sleep() to call __might_sleep_precision() but the
> impact should be negligible. strlen() isn't called in a normal case;
> it's called only when printing the error (sleeping function called
> from invalid context).
>
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].
>
> [1]: https://github.com/rust-lang/libs-team/issues/466
> Co-developed-by: Boqun Feng <[e-mail]>
> Signed-off-by: Boqun Feng <[e-mail]>
> Signed-off-by: FUJITA Tomonori <[e-mail]>

Reviewed-by: Alice Ryhl <[e-mail]>

Peter Zijlstra Jan. 28, 2025, 11:37 a.m. UTC | #2

On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
> Add __might_sleep_precision(), Rust friendly version of
> __might_sleep(), which takes a pointer to a string with the length
> instead of a null-terminated string.
> 
> Rust's core::panic::Location::file(), which gives the file name of a
> caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
> 
> Modify __might_sleep() to call __might_sleep_precision() but the
> impact should be negligible. strlen() isn't called in a normal case;
> it's called only when printing the error (sleeping function called
> from invalid context).
> 
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].

Urgh :/

FUJITA Tomonori Jan. 29, 2025, 11:56 p.m. UTC | #3

On Tue, 28 Jan 2025 12:37:38 +0100
Peter Zijlstra <[e-mail]> wrote:

> On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
>> Add __might_sleep_precision(), Rust friendly version of
>> __might_sleep(), which takes a pointer to a string with the length
>> instead of a null-terminated string.
>> 
>> Rust's core::panic::Location::file(), which gives the file name of a
>> caller, doesn't provide a null-terminated
>> string. __might_sleep_precision() uses a precision specifier in the
>> printk format, which specifies the length of a string; a string
>> doesn't need to be a null-terminated.
>> 
>> Modify __might_sleep() to call __might_sleep_precision() but the
>> impact should be negligible. strlen() isn't called in a normal case;
>> it's called only when printing the error (sleeping function called
>> from invalid context).
>> 
>> Note that Location::file() providing a null-terminated string for
>> better C interoperability is under discussion [1].
> 
> Urgh :/

Yeah... so not acceptable?

Then I switch to the implementation with Rust macros, which gives a
null terminated string.

Boqun Feng Jan. 30, 2025, 1:14 a.m. UTC | #4

On Thu, Jan 30, 2025 at 08:56:44AM +0900, FUJITA Tomonori wrote:
> On Tue, 28 Jan 2025 12:37:38 +0100
> Peter Zijlstra <[e-mail]> wrote:
> 
> > On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
> >> Add __might_sleep_precision(), Rust friendly version of
> >> __might_sleep(), which takes a pointer to a string with the length
> >> instead of a null-terminated string.
> >> 
> >> Rust's core::panic::Location::file(), which gives the file name of a
> >> caller, doesn't provide a null-terminated
> >> string. __might_sleep_precision() uses a precision specifier in the
> >> printk format, which specifies the length of a string; a string
> >> doesn't need to be a null-terminated.
> >> 
> >> Modify __might_sleep() to call __might_sleep_precision() but the
> >> impact should be negligible. strlen() isn't called in a normal case;
> >> it's called only when printing the error (sleeping function called
> >> from invalid context).
> >> 
> >> Note that Location::file() providing a null-terminated string for
> >> better C interoperability is under discussion [1].
> > 
> > Urgh :/
> 
> Yeah... so not acceptable?
> 

I would like to see some concrete and technical reasons for why it's not
acceptable ;-) I'm not sure whether Peter was against this patch or just
not happy about Location::file() providing a null-terminated string is a
WIP.

To me, null-terminated string literals don't provide much benefits
other than you can pass it via only one pointer value, the cost is that
you will always need to calculate the length of the string when needed,
so hard to say it's a straightforward win.

Regards,
Boqun

> Then I switch to the implementation with Rust macros, which gives a
> null terminated string.

Patch

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 88a9a515b2ba..69d77c14ad33 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8666,24 +8666,6 @@  void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
-{
-	unsigned int state = get_current_state();
-	/*
-	 * Blocking primitives will set (and therefore destroy) current->state,
-	 * since we will exit with TASK_RUNNING make sure we enter with it,
-	 * otherwise we will destroy state.
-	 */
-	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
-			"do not call blocking ops when !TASK_RUNNING; "
-			"state=%x set at [<%p>] %pS\n", state,
-			(void *)current->task_state_change,
-			(void *)current->task_state_change);
-
-	__might_resched(file, line, 0);
-}
-EXPORT_SYMBOL(__might_sleep);
-
 static void print_preempt_disable_ip(int preempt_offset, unsigned long ip)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_PREEMPT))
@@ -8705,7 +8687,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;
@@ -8728,8 +8711,10 @@  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);
+	if (len < 0)
+		len = strlen(file);
+	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);
@@ -8754,8 +8739,36 @@  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)
+{
+	__might_resched_precision(file, -1, line, offsets);
+}
 EXPORT_SYMBOL(__might_resched);
 
+void __might_sleep_precision(const char *file, int len, int line)
+{
+	unsigned int state = get_current_state();
+	/*
+	 * Blocking primitives will set (and therefore destroy) current->state,
+	 * since we will exit with TASK_RUNNING make sure we enter with it,
+	 * otherwise we will destroy state.
+	 */
+	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
+			"do not call blocking ops when !TASK_RUNNING; "
+			"state=%x set at [<%p>] %pS\n", state,
+			(void *)current->task_state_change,
+			(void *)current->task_state_change);
+
+	__might_resched_precision(file, len, line, 0);
+}
+
+void __might_sleep(const char *file, int line)
+{
+	__might_sleep_precision(file, -1, line);
+}
+EXPORT_SYMBOL(__might_sleep);
+
 void __cant_sleep(const char *file, int line, int preempt_offset)
 {
 	static unsigned long prev_jiffy;