| Message ID | 20241025033118.44452-5-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | rust: Add IO polling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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 <fujita.tomonori@gmail.com> > --- > 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/CANiq72kWqSCSkUk1efZyAi+0ScNTtfALn+wiJY_aoQefu2TNvg@mail.gmail.com/ , 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 > >
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/CANiq72kWqSCSkUk1efZyAi+0ScNTtfALn+wiJY_aoQefu2TNvg@mail.gmail.com/ > "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 [...]
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) + } +}
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 <fujita.tomonori@gmail.com> --- 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