| Message ID | 20241016035214.2229-6-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rust: Add IO polling | expand |
On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori <fujita.tomonori@gmail.com> 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. > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > Link: https://rust-for-linux.com/klint [1] > --- > rust/helpers/helpers.c | 1 + > rust/helpers/time.c | 8 ++++++++ > rust/kernel/time.rs | 4 +++- > rust/kernel/time/delay.rs | 31 +++++++++++++++++++++++++++++++ > 4 files changed, 43 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 9b0537b63cf7..d58daff6f928 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..dc7e2b3a0ab2 > --- /dev/null > +++ b/rust/kernel/time/delay.rs > @@ -0,0 +1,31 @@ > +// 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. > +/// > +/// `Delta` must be longer than one microsecond. Why is this required? Right now you just round up to one microsecond, which seems okay. > +/// 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_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong, You probably want this: delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC This would avoid a crash if someone passes i64::MAX nanoseconds and CONFIG_RUST_OVERFLOW_CHECKS is enabled. Alice
On Wed, Oct 16, 2024 at 10:29 AM Alice Ryhl <aliceryhl@google.com> wrote: > > You probably want this: > > delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC > > This would avoid a crash if someone passes i64::MAX nanoseconds and > CONFIG_RUST_OVERFLOW_CHECKS is enabled. I think we should document whether `fsleep` is expected to be usable for "forever" values. It sounds like that, given "too large" values in `msecs_to_jiffies` mean "infinite timeout". Cheers, Miguel
> +/// 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_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong, You add a as_sec() helper, but then don't use it? If the helper does not do what you want, maybe the helper is wrong? This is part of why we say all APIs should have a user. Andrew
On Wed, 16 Oct 2024 10:29:12 +0200 Alice Ryhl <aliceryhl@google.com> wrote: >> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs >> new file mode 100644 >> index 000000000000..dc7e2b3a0ab2 >> --- /dev/null >> +++ b/rust/kernel/time/delay.rs >> @@ -0,0 +1,31 @@ >> +// 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. >> +/// >> +/// `Delta` must be longer than one microsecond. > > Why is this required? Right now you just round up to one microsecond, > which seems okay. Oops, it should have been removed. >> +/// 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_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong, > > You probably want this: > > delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC > > This would avoid a crash if someone passes i64::MAX nanoseconds and > CONFIG_RUST_OVERFLOW_CHECKS is enabled. Ah, I'll fix. Thanks!
On Wed, 16 Oct 2024 11:42:07 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Wed, Oct 16, 2024 at 10:29 AM Alice Ryhl <aliceryhl@google.com> wrote: >> >> You probably want this: >> >> delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC >> >> This would avoid a crash if someone passes i64::MAX nanoseconds and >> CONFIG_RUST_OVERFLOW_CHECKS is enabled. > > I think we should document whether `fsleep` is expected to be usable > for "forever" values. > > It sounds like that, given "too large" values in `msecs_to_jiffies` > mean "infinite timeout". Do you mean msecs_to_jiffies() returns MAX_JIFFY_OFFSET ((LONG_MAX >> 1)-1) with a value exceeding i32::MAX milliseconds?
On Thu, Oct 24, 2024 at 2:22 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Do you mean msecs_to_jiffies() returns MAX_JIFFY_OFFSET ((LONG_MAX >> > 1)-1) with a value exceeding i32::MAX milliseconds? Yeah -- well, I was referring to its docs: * - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET) * * - 'too large' values [that would result in larger than * MAX_JIFFY_OFFSET values] mean 'infinite timeout' too. So I assume `fsleep` is meant to inherit that. Cheers, Miguel
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 9b0537b63cf7..d58daff6f928 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..dc7e2b3a0ab2 --- /dev/null +++ b/rust/kernel/time/delay.rs @@ -0,0 +1,31 @@ +// 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. +/// +/// `Delta` must be longer than one microsecond. +/// +/// 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_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong, + ) + } +}
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. Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Link: https://rust-for-linux.com/klint [1] --- rust/helpers/helpers.c | 1 + rust/helpers/time.c | 8 ++++++++ rust/kernel/time.rs | 4 +++- rust/kernel/time/delay.rs | 31 +++++++++++++++++++++++++++++++ 4 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 rust/helpers/time.c create mode 100644 rust/kernel/time/delay.rs