[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20241109.183804.1515599584405139212.fujita.tomonori@gmail.com>
Date: Sat, 09 Nov 2024 18:38:04 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: boqun.feng@...il.com
Cc: fujita.tomonori@...il.com, 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,
pmladek@...e.com, rostedt@...dmis.org, andriy.shevchenko@...ux.intel.com,
linux@...musvillemoes.dk, senozhatsky@...omium.org, mingo@...hat.com,
peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
dietmar.eggemann@....com, bsegall@...gle.com, mgorman@...e.de,
vschneid@...hat.com
Subject: Re: [PATCH v5 6/7] rust: Add read_poll_timeout functions
On Wed, 6 Nov 2024 13:35:09 -0800
Boqun Feng <boqun.feng@...il.com> 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.
Powered by blists - more mailing lists