Re: [PATCH v5 6/7] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 6 Nov 2024 13:35:09 -0800
Boqun Feng <boqun.feng@xxxxxxxxx> wrote:

> 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.

Ah, I didn't know this.

> 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)

Cc scheduler people to ask if they would accept the above change for
Rust or prefer that Rust itself handles the null-terminated string
issue.