[v4,4/7] rust: time: Add wrapper for fsleep function

Commit Message

FUJITA Tomonori Oct. 25, 2024, 3:31 a.m. UTC

Add a wrapper for fsleep, flexible sleep functions in
`include/linux/delay.h` which typically deals with hardware delays.

The kernel supports several `sleep` functions to handle various
lengths of delay. This adds fsleep, automatically chooses the best
sleep method based on a duration.

`sleep` functions including `fsleep` belongs to TIMERS, not
TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
abstraction for TIMEKEEPING. To make Rust abstractions match the C
side, add rust/kernel/time/delay.rs for this wrapper.

fsleep() can only be used in a nonatomic context. This requirement is
not checked by these abstractions, but it is intended that klint [1]
or a similar tool will be used to check it in the future.

Link: https://rust-for-linux.com/klint [1]
Signed-off-by: FUJITA Tomonori <[e-mail]>
---
 rust/helpers/helpers.c    |  1 +
 rust/helpers/time.c       |  8 ++++++++
 rust/kernel/time.rs       |  4 +++-
 rust/kernel/time/delay.rs | 30 ++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/time.c
 create mode 100644 rust/kernel/time/delay.rs

Comments

Boqun Feng Oct. 25, 2024, 10:03 p.m. UTC | #1

On Fri, Oct 25, 2024 at 12:31:15PM +0900, FUJITA Tomonori wrote:
> Add a wrapper for fsleep, flexible sleep functions in
> `include/linux/delay.h` which typically deals with hardware delays.
> 
> The kernel supports several `sleep` functions to handle various
> lengths of delay. This adds fsleep, automatically chooses the best
> sleep method based on a duration.
> 
> `sleep` functions including `fsleep` belongs to TIMERS, not
> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
> abstraction for TIMEKEEPING. To make Rust abstractions match the C
> side, add rust/kernel/time/delay.rs for this wrapper.
> 
> fsleep() can only be used in a nonatomic context. This requirement is
> not checked by these abstractions, but it is intended that klint [1]
> or a similar tool will be used to check it in the future.
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <[e-mail]>
> ---
>  rust/helpers/helpers.c    |  1 +
>  rust/helpers/time.c       |  8 ++++++++
>  rust/kernel/time.rs       |  4 +++-
>  rust/kernel/time/delay.rs | 30 ++++++++++++++++++++++++++++++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100644 rust/helpers/time.c
>  create mode 100644 rust/kernel/time/delay.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 30f40149f3a9..c274546bcf78 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -21,6 +21,7 @@
>  #include "slab.c"
>  #include "spinlock.c"
>  #include "task.c"
> +#include "time.c"
>  #include "uaccess.c"
>  #include "wait.c"
>  #include "workqueue.c"
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> new file mode 100644
> index 000000000000..7ae64ad8141d
> --- /dev/null
> +++ b/rust/helpers/time.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/delay.h>
> +
> +void rust_helper_fsleep(unsigned long usecs)
> +{
> +	fsleep(usecs);
> +}
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 3cc1a8a76777..cfc31f908710 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -2,12 +2,14 @@
>  
>  //! Time related primitives.
>  //!
> -//! This module contains the kernel APIs related to time and timers that
> +//! This module contains the kernel APIs related to time that
>  //! have been ported or wrapped for usage by Rust code in the kernel.
>  //!
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>  
> +pub mod delay;
> +
>  /// The number of nanoseconds per microsecond.
>  pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
>  
> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
> new file mode 100644
> index 000000000000..f80f35f50949
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Delay and sleep primitives.
> +//!
> +//! This module contains the kernel APIs related to delay and sleep that
> +//! have been ported or wrapped for usage by Rust code in the kernel.
> +//!
> +//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
> +
> +use crate::time;
> +use core::ffi::c_ulong;
> +
> +/// Sleeps for a given duration at least.
> +///
> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
> +/// which automatically chooses the best sleep method based on a duration.
> +///
> +/// The function sleeps infinitely (MAX_JIFFY_OFFSET) if `Delta` is negative
> +/// or exceedes i32::MAX milliseconds.
> +///

I know Miguel has made his suggestion:

	https://lore.kernel.org/rust-for-linux/[e-mail]/

, but I think what we should really do here is just panic if `Delta` is
negative or exceedes i32::MAX milliseconds, and document clearly that
this function expects `Delta` to be in a certain range, i.e. it's the
user's responsibility to check. Because:

*	You can simply call schedule() with task state set properly to
	"sleep infinitely".

*	Most of the users of fsleep() don't need this "sleep infinitely"
	functionality. Instead, they want to sleep with a reasonable
	short time.

> +/// This function can only be used in a nonatomic context.
> +pub fn fsleep(delta: time::Delta) {
> +    // SAFETY: FFI call.
> +    unsafe {
> +        // Convert the duration to microseconds and round up to preserve
> +        // the guarantee; fsleep sleeps for at least the provided duration,
> +        // but that it may sleep for longer under some circumstances.
> +        bindings::fsleep(delta.as_micros_ceil() as c_ulong)

If delta is 0x10000_0000i64 * 1000_000 (=0xf424000000000i64), which
exceeds i32::MAX milliseconds, the result of `delta.as_micros_ceil() as
c_ulong` is:

*	0 on 32bit
*	0x3e800000000 on 64bit

, if I got my math right. The first is obviously not "sleeps
infinitely".

Continue on 64bit case, in C's fsleep(), 0x3e800000000 will be cast to
"int" (to call msleep()), which results as 0, still not "sleep
infinitely"?

Regards,
Boqun

> +    }
> +}
> -- 
> 2.43.0
> 
>

Boqun Feng Oct. 26, 2024, 12:16 a.m. UTC | #2

On Fri, Oct 25, 2024 at 03:03:37PM -0700, Boqun Feng wrote:
[...]
> > +/// Sleeps for a given duration at least.
> > +///
> > +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
> > +/// which automatically chooses the best sleep method based on a duration.
> > +///
> > +/// The function sleeps infinitely (MAX_JIFFY_OFFSET) if `Delta` is negative
> > +/// or exceedes i32::MAX milliseconds.
> > +///
> 
> I know Miguel has made his suggestion:
> 
> 	https://lore.kernel.org/rust-for-linux/[e-mail]/
> 

"made his suggestion" is probably a wrong statement from me, looking
back the emails, Miguel was simply talking about we should document this
and his assumption that `fsleep()` should inherit the behavior/semantics
of msecs_to_jiffies().

But yeah, I still suggest doing it differently, i.e. panic on negative
or exceeding i32::MAX milliseconds.

Regards,
Boqun

[...]

FUJITA Tomonori Oct. 28, 2024, 12:50 a.m. UTC | #3

On Fri, 25 Oct 2024 15:03:37 -0700
Boqun Feng <[e-mail]> wrote:

>> +/// Sleeps for a given duration at least.
>> +///
>> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
>> +/// which automatically chooses the best sleep method based on a duration.
>> +///
>> +/// The function sleeps infinitely (MAX_JIFFY_OFFSET) if `Delta` is negative
>> +/// or exceedes i32::MAX milliseconds.
>> +///
> 
> I know Miguel has made his suggestion:
> 
> 	https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> , but I think what we should really do here is just panic if `Delta` is
> negative or exceedes i32::MAX milliseconds, and document clearly that
> this function expects `Delta` to be in a certain range, i.e. it's the
> user's responsibility to check. Because:
> 
> *	You can simply call schedule() with task state set properly to
> 	"sleep infinitely".
> 
> *	Most of the users of fsleep() don't need this "sleep infinitely"
> 	functionality. Instead, they want to sleep with a reasonable
> 	short time.

I agree with the above reasons but I'm not sure about just panic with
a driver's invalid argument.

Can we just return an error instead?

>> +/// This function can only be used in a nonatomic context.
>> +pub fn fsleep(delta: time::Delta) {
>> +    // SAFETY: FFI call.
>> +    unsafe {
>> +        // Convert the duration to microseconds and round up to preserve
>> +        // the guarantee; fsleep sleeps for at least the provided duration,
>> +        // but that it may sleep for longer under some circumstances.
>> +        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
> 
> If delta is 0x10000_0000i64 * 1000_000 (=0xf424000000000i64), which
> exceeds i32::MAX milliseconds, the result of `delta.as_micros_ceil() as
> c_ulong` is:
> 
> *	0 on 32bit
> *	0x3e800000000 on 64bit
> 
> , if I got my math right. The first is obviously not "sleeps
> infinitely".
> 
> Continue on 64bit case, in C's fsleep(), 0x3e800000000 will be cast to
> "int" (to call msleep()), which results as 0, still not "sleep
> infinitely"?

You mean "unsigned int" (to call msleep())?

You are correct that we can't say "the function sleeps infinitely
(MAX_JIFFY_OFFSET) if `Delta` is negative or exceeds i32::MAX
milliseconds.". There are some exceptional ranges.

Considering that Rust-for-Linux might eventually support 32-bit
systems, fsleep's arguments must be less than u32::MAX (usecs).
Additionally, Because of DIV_ROUND_UP (to call msleep()), it must be
less than u32::MAX - 1000. To simplify the expression, the maximum
Delta is u32::MAX / 2 (usecs)? I think that it's long enough for
the users of fsleep().

Boqun Feng Oct. 28, 2024, 4:38 a.m. UTC | #4

On Mon, Oct 28, 2024 at 09:50:30AM +0900, FUJITA Tomonori wrote:
> On Fri, 25 Oct 2024 15:03:37 -0700
> Boqun Feng <[e-mail]> wrote:
> 
> >> +/// Sleeps for a given duration at least.
> >> +///
> >> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
> >> +/// which automatically chooses the best sleep method based on a duration.
> >> +///
> >> +/// The function sleeps infinitely (MAX_JIFFY_OFFSET) if `Delta` is negative
> >> +/// or exceedes i32::MAX milliseconds.
> >> +///
> > 
> > I know Miguel has made his suggestion:
> > 
> > 	https://lore.kernel.org/rust-for-linux/[e-mail]/
> > 
> > , but I think what we should really do here is just panic if `Delta` is
> > negative or exceedes i32::MAX milliseconds, and document clearly that
> > this function expects `Delta` to be in a certain range, i.e. it's the
> > user's responsibility to check. Because:
> > 
> > *	You can simply call schedule() with task state set properly to
> > 	"sleep infinitely".
> > 
> > *	Most of the users of fsleep() don't need this "sleep infinitely"
> > 	functionality. Instead, they want to sleep with a reasonable
> > 	short time.
> 
> I agree with the above reasons but I'm not sure about just panic with
> a driver's invalid argument.
> 

If a driver blindly trusts a user-space input or a value chosen by the
hardware, I would say it's a bug in the driver. So IMO drivers should
check the input of fsleep().

> Can we just return an error instead?
> 

That also works for me, but an immediate question is: do we put
#[must_use] on `fsleep()` to enforce the use of the return value? If
yes, then the normal users would need to explicitly ignore the return
value:

	let _ = fsleep(1sec);

The "let _ =" would be a bit annoying for every user that just uses a
constant duration.

If no, then a user with incorrect input can just skip the return value
check:

	fsleep(duration_based_on_user_input);

Would it be an issue? For example, you put an fsleep() in the loop and
expect it at least sleep for a bit time, however, a craft user input
can cause the sleep never happen, and as a result, turn the loop into a
busy waiting.

All I'm trying to say is that 99% users of fsleep() will just use a
constant and small value, it seems a bit over-doing to me to return an
error just for the <1% users in theory that don't check the input. But
that's not a blocker from me.

> >> +/// This function can only be used in a nonatomic context.
> >> +pub fn fsleep(delta: time::Delta) {
> >> +    // SAFETY: FFI call.
> >> +    unsafe {
> >> +        // Convert the duration to microseconds and round up to preserve
> >> +        // the guarantee; fsleep sleeps for at least the provided duration,
> >> +        // but that it may sleep for longer under some circumstances.
> >> +        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
> > 
> > If delta is 0x10000_0000i64 * 1000_000 (=0xf424000000000i64), which
> > exceeds i32::MAX milliseconds, the result of `delta.as_micros_ceil() as
> > c_ulong` is:
> > 
> > *	0 on 32bit
> > *	0x3e800000000 on 64bit
> > 
> > , if I got my math right. The first is obviously not "sleeps
> > infinitely".
> > 
> > Continue on 64bit case, in C's fsleep(), 0x3e800000000 will be cast to
> > "int" (to call msleep()), which results as 0, still not "sleep
> > infinitely"?
> 
> You mean "unsigned int" (to call msleep())?
> 

Ah, yes.

> You are correct that we can't say "the function sleeps infinitely
> (MAX_JIFFY_OFFSET) if `Delta` is negative or exceeds i32::MAX
> milliseconds.". There are some exceptional ranges.
> 
> Considering that Rust-for-Linux might eventually support 32-bit
> systems, fsleep's arguments must be less than u32::MAX (usecs).
> Additionally, Because of DIV_ROUND_UP (to call msleep()), it must be
> less than u32::MAX - 1000. To simplify the expression, the maximum
> Delta is u32::MAX / 2 (usecs)? I think that it's long enough for
> the users of fsleep().
> 

Agreed. Though I'm attempting to make msleep() takes a `unsigned long`,
but I already have a lot in my plate...

Regards,
Boqun

FUJITA Tomonori Oct. 28, 2024, 11:30 p.m. UTC | #5

On Sun, 27 Oct 2024 21:38:41 -0700
Boqun Feng <[e-mail]> wrote:

> On Mon, Oct 28, 2024 at 09:50:30AM +0900, FUJITA Tomonori wrote:
>> On Fri, 25 Oct 2024 15:03:37 -0700
>> Boqun Feng <[e-mail]> wrote:
>> 
>> >> +/// Sleeps for a given duration at least.
>> >> +///
>> >> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
>> >> +/// which automatically chooses the best sleep method based on a duration.
>> >> +///
>> >> +/// The function sleeps infinitely (MAX_JIFFY_OFFSET) if `Delta` is negative
>> >> +/// or exceedes i32::MAX milliseconds.
>> >> +///
>> > 
>> > I know Miguel has made his suggestion:
>> > 
>> > 	https://lore.kernel.org/rust-for-linux/[e-mail]/
>> > 
>> > , but I think what we should really do here is just panic if `Delta` is
>> > negative or exceedes i32::MAX milliseconds, and document clearly that
>> > this function expects `Delta` to be in a certain range, i.e. it's the
>> > user's responsibility to check. Because:
>> > 
>> > *	You can simply call schedule() with task state set properly to
>> > 	"sleep infinitely".
>> > 
>> > *	Most of the users of fsleep() don't need this "sleep infinitely"
>> > 	functionality. Instead, they want to sleep with a reasonable
>> > 	short time.
>> 
>> I agree with the above reasons but I'm not sure about just panic with
>> a driver's invalid argument.
>> 
> 
> If a driver blindly trusts a user-space input or a value chosen by the
> hardware, I would say it's a bug in the driver. So IMO drivers should
> check the input of fsleep().
> 
>> Can we just return an error instead?
>> 
> 
> That also works for me, but an immediate question is: do we put
> #[must_use] on `fsleep()` to enforce the use of the return value? If
> yes, then the normal users would need to explicitly ignore the return
> value:
> 
> 	let _ = fsleep(1sec);
> 
> The "let _ =" would be a bit annoying for every user that just uses a
> constant duration.

Yeah, but I don't think that we have enough of an excuse here to break
the rule "Do not crash the kernel".

Another possible option is to convert an invalid argument to a safe
value (e.g., the maximum), possibly with WARN_ON_ONCE().

Thomas Gleixner Oct. 29, 2024, 7:55 a.m. UTC | #6

On Tue, Oct 29 2024 at 08:30, FUJITA Tomonori wrote:
> On Sun, 27 Oct 2024 21:38:41 -0700
> Boqun Feng <[e-mail]> wrote:
>> That also works for me, but an immediate question is: do we put
>> #[must_use] on `fsleep()` to enforce the use of the return value? If
>> yes, then the normal users would need to explicitly ignore the return
>> value:
>> 
>> 	let _ = fsleep(1sec);
>> 
>> The "let _ =" would be a bit annoying for every user that just uses a
>> constant duration.
>
> Yeah, but I don't think that we have enough of an excuse here to break
> the rule "Do not crash the kernel".
>
> Another possible option is to convert an invalid argument to a safe
> value (e.g., the maximum), possibly with WARN_ON_ONCE().

That makes sense.

FUJITA Tomonori Oct. 31, 2024, 8:31 a.m. UTC | #7

On Tue, 29 Oct 2024 08:55:50 +0100
Thomas Gleixner <[e-mail]> wrote:

> On Tue, Oct 29 2024 at 08:30, FUJITA Tomonori wrote:
>> On Sun, 27 Oct 2024 21:38:41 -0700
>> Boqun Feng <[e-mail]> wrote:
>>> That also works for me, but an immediate question is: do we put
>>> #[must_use] on `fsleep()` to enforce the use of the return value? If
>>> yes, then the normal users would need to explicitly ignore the return
>>> value:
>>> 
>>> 	let _ = fsleep(1sec);
>>> 
>>> The "let _ =" would be a bit annoying for every user that just uses a
>>> constant duration.
>>
>> Yeah, but I don't think that we have enough of an excuse here to break
>> the rule "Do not crash the kernel".
>>
>> Another possible option is to convert an invalid argument to a safe
>> value (e.g., the maximum), possibly with WARN_ON_ONCE().
> 
> That makes sense.

Thanks! I'll do the conversion in the next version.

Patch

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 30f40149f3a9..c274546bcf78 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -21,6 +21,7 @@ 
 #include "slab.c"
 #include "spinlock.c"
 #include "task.c"
+#include "time.c"
 #include "uaccess.c"
 #include "wait.c"
 #include "workqueue.c"
diff --git a/rust/helpers/time.c b/rust/helpers/time.c
new file mode 100644
index 000000000000..7ae64ad8141d
--- /dev/null
+++ b/rust/helpers/time.c
@@ -0,0 +1,8 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/delay.h>
+
+void rust_helper_fsleep(unsigned long usecs)
+{
+	fsleep(usecs);
+}
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 3cc1a8a76777..cfc31f908710 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -2,12 +2,14 @@ 
 
 //! Time related primitives.
 //!
-//! This module contains the kernel APIs related to time and timers that
+//! This module contains the kernel APIs related to time that
 //! have been ported or wrapped for usage by Rust code in the kernel.
 //!
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+pub mod delay;
+
 /// The number of nanoseconds per microsecond.
 pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
 
diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
new file mode 100644
index 000000000000..f80f35f50949
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,30 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Delay and sleep primitives.
+//!
+//! This module contains the kernel APIs related to delay and sleep that
+//! have been ported or wrapped for usage by Rust code in the kernel.
+//!
+//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
+
+use crate::time;
+use core::ffi::c_ulong;
+
+/// Sleeps for a given duration at least.
+///
+/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
+/// which automatically chooses the best sleep method based on a duration.
+///
+/// The function sleeps infinitely (MAX_JIFFY_OFFSET) if `Delta` is negative
+/// or exceedes i32::MAX milliseconds.
+///
+/// This function can only be used in a nonatomic context.
+pub fn fsleep(delta: time::Delta) {
+    // SAFETY: FFI call.
+    unsafe {
+        // Convert the duration to microseconds and round up to preserve
+        // the guarantee; fsleep sleeps for at least the provided duration,
+        // but that it may sleep for longer under some circumstances.
+        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
+    }
+}