Re: [PATCH v10 1/8] sched/core: Add __might_sleep_precision()

[David Laight]

On Fri,  7 Feb 2025 22:26:16 +0900
FUJITA Tomonori <fujita.tomonori@xxxxxxxxx> 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].
> 
> Link: https://github.com/rust-lang/libs-team/issues/466 [1]
> Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> Co-developed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
> ---
>  include/linux/kernel.h |  2 ++
>  kernel/sched/core.c    | 55 ++++++++++++++++++++++++++----------------
>  2 files changed, 36 insertions(+), 21 deletions(-)
> 
> 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 165c90ba64ea..d308f2a8692e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8678,24 +8678,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))
> @@ -8717,7 +8699,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,

For clarity that ought to be file_len.

> +				      unsigned int offsets)
>  {
>  	/* Ratelimiting timestamp: */
>  	static unsigned long prev_jiffy;
> @@ -8740,8 +8723,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);

No need for strlen(), just use a big number instead of -1.
Anything bigger than a sane upper limit on the filename length will do.

	David

> +	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);
> @@ -8766,8 +8751,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;