| Message ID | 20250207132623.168854-2-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rust: Add IO polling | expand |
On Fri, 7 Feb 2025 22:26:16 +0900 FUJITA Tomonori <fujita.tomonori@gmail.com> 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@google.com> > 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 | 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;
On Fri, 7 Feb 2025 18:12:58 +0000 David Laight <david.laight.linux@gmail.com> wrote: >> 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. Yeah, I'll update. >> + 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. Ah, that's right. Just passing the maximum precision (1<<15-1) works. The precision specifies the maximum length. vsnprintf() always iterates through a string until it reaches the maximum length or encounters the null terminator. So strlen() here is useless. Alice and Boqun, the above change is fine? Can I keep the tags?
On Sat, Feb 8, 2025 at 4:01 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Fri, 7 Feb 2025 18:12:58 +0000 > David Laight <david.laight.linux@gmail.com> wrote: > > >> 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. > > Yeah, I'll update. > > >> + 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. > > Ah, that's right. Just passing the maximum precision (1<<15-1) works. > > The precision specifies the maximum length. vsnprintf() always > iterates through a string until it reaches the maximum length or > encounters the null terminator. So strlen() here is useless. > > Alice and Boqun, the above change is fine? Can I keep the tags? I'd probably like a comment explaining the meaning of this constant somewhere, but sure ok with me. Alice
On Mon, Feb 10, 2025 at 10:41:00AM +0100, Alice Ryhl wrote: > On Sat, Feb 8, 2025 at 4:01 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > > On Fri, 7 Feb 2025 18:12:58 +0000 > > David Laight <david.laight.linux@gmail.com> wrote: > > > > >> 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. > > > > Yeah, I'll update. > > > > >> + 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. > > > > Ah, that's right. Just passing the maximum precision (1<<15-1) works. > > > > The precision specifies the maximum length. vsnprintf() always > > iterates through a string until it reaches the maximum length or > > encounters the null terminator. So strlen() here is useless. > > > > Alice and Boqun, the above change is fine? Can I keep the tags? > > I'd probably like a comment explaining the meaning of this constant > somewhere, but sure ok with me. > Agreed. The code should be fine but need some comments. Regards, Boqun > Alice
On Sun, 16 Feb 2025 17:51:32 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: >> > >> + 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. >> > >> > Ah, that's right. Just passing the maximum precision (1<<15-1) works. >> > >> > The precision specifies the maximum length. vsnprintf() always >> > iterates through a string until it reaches the maximum length or >> > encounters the null terminator. So strlen() here is useless. >> > >> > Alice and Boqun, the above change is fine? Can I keep the tags? >> >> I'd probably like a comment explaining the meaning of this constant >> somewhere, but sure ok with me. >> > > Agreed. The code should be fine but need some comments. Of course, I'll add some in the next version. Thanks!
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, + 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); + 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;