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 | 43 +++++++++++++++++++++++++++++++++++++++
4 files changed, 55 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 dcf827a61b52..d16aeda7a558 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -26,6 +26,7 @@
#include "slab.c"
#include "spinlock.c"
#include "task.c"
+#include "time.c"
#include "uaccess.c"
#include "vmalloc.c"
#include "wait.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 da54a70f8f1f..3be2bf578519 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..db5c08b0f230
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,43 @@
+// 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 super::Delta;
+use crate::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 0 or greater and no more than `u32::MAX / 2` microseconds.
+/// If a value outside the range is given, the function will sleep
+/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
+///
+/// This function can only be used in a nonatomic context.
+pub fn fsleep(delta: Delta) {
+ // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
+ // Considering that fsleep rounds up the duration to the nearest millisecond,
+ // set the maximum value to u32::MAX / 2 microseconds.
+ const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
+
+ let duration = if delta > MAX_DURATION || delta.is_negative() {
+ // TODO: add WARN_ONCE() when it's supported.
+ MAX_DURATION
+ } else {
+ 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(duration.as_micros_ceil() as c_ulong)
+ }
+}
--
2.43.0On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> +/// If a value outside the range is given, the function will sleep
> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
I would emphasize with something like:
`delta` must be within [0, `u32::MAX / 2`] microseconds;
otherwise, it is erroneous behavior. That is, it is considered a bug
to call this function with an out-of-range value, in which case the
function will sleep for at least the maximum value in the range and
may warn in the future.
In addition, I would add a new paragraph how the behavior differs
w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
`fsleep()` would do an infinite sleep instead. So I think it is
important to highlight that.
> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> + // set the maximum value to u32::MAX / 2 microseconds.
Nit: please use Markdown code spans in normal comments (no need for
intra-doc links there).
> + let duration = if delta > MAX_DURATION || delta.is_negative() {
> + // TODO: add WARN_ONCE() when it's supported.
Ditto (also "Add").
By the way, can this be written differently maybe? e.g. using a range
since it is `const`?
You can probably reuse `delta` as the new bindings name, since we
don't need the old one after this step.
Thanks!
Cheers,
Miguel
On Fri, 17 Jan 2025 19:59:15 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
>> +/// If a value outside the range is given, the function will sleep
>> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
>
> I would emphasize with something like:
>
> `delta` must be within [0, `u32::MAX / 2`] microseconds;
> otherwise, it is erroneous behavior. That is, it is considered a bug
> to call this function with an out-of-range value, in which case the
> function will sleep for at least the maximum value in the range and
> may warn in the future.
Thanks, I'll use the above instead.
> In addition, I would add a new paragraph how the behavior differs
> w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
> `fsleep()` would do an infinite sleep instead. So I think it is
> important to highlight that.
/// The above behavior differs from the kernel's [`fsleep`], which could sleep
/// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
Looks ok?
>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
>> + // set the maximum value to u32::MAX / 2 microseconds.
>
> Nit: please use Markdown code spans in normal comments (no need for
> intra-doc links there).
Understood.
>> + let duration = if delta > MAX_DURATION || delta.is_negative() {
>> + // TODO: add WARN_ONCE() when it's supported.
>
> Ditto (also "Add").
Oops, I'll fix.
> By the way, can this be written differently maybe? e.g. using a range
> since it is `const`?
A range can be used for a custom type?
> You can probably reuse `delta` as the new bindings name, since we
> don't need the old one after this step.
Do you mean something like the following?
const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
let delta = if delta > MAX_DELTA || delta.is_negative() {
// TODO: Add WARN_ONCE() when it's supported.
MAX_DELTA
} else {
delta
};
On Sat, 18 Jan 2025 17:02:24 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> On Fri, 17 Jan 2025 19:59:15 +0100
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> >> +/// If a value outside the range is given, the function will sleep
> >> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
> >
> > I would emphasize with something like:
> >
> > `delta` must be within [0, `u32::MAX / 2`] microseconds;
> > otherwise, it is erroneous behavior. That is, it is considered a bug
> > to call this function with an out-of-range value, in which case the
> > function will sleep for at least the maximum value in the range and
> > may warn in the future.
>
> Thanks, I'll use the above instead.
>
> > In addition, I would add a new paragraph how the behavior differs
> > w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
> > `fsleep()` would do an infinite sleep instead. So I think it is
> > important to highlight that.
>
> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
>
> Looks ok?
>
> >> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> >> + // set the maximum value to u32::MAX / 2 microseconds.
> >
> > Nit: please use Markdown code spans in normal comments (no need for
> > intra-doc links there).
>
> Understood.
>
> >> + let duration = if delta > MAX_DURATION || delta.is_negative() {
> >> + // TODO: add WARN_ONCE() when it's supported.
> >
> > Ditto (also "Add").
>
> Oops, I'll fix.
>
> > By the way, can this be written differently maybe? e.g. using a range
> > since it is `const`?
>
> A range can be used for a custom type?
Yes, you can say `!(Delta::ZERO..MAX_DURATION).contains(&delta)`.
(You'll need to add `Delta::ZERO`).
The `start..end` syntax is a fancy way of constructing a
`core::ops::Range` which has `contains` as long as `PartialOrd` is
implemented.
>
> > You can probably reuse `delta` as the new bindings name, since we
> > don't need the old one after this step.
>
> Do you mean something like the following?
>
> const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
>
> let delta = if delta > MAX_DELTA || delta.is_negative() {
> // TODO: Add WARN_ONCE() when it's supported.
> MAX_DELTA
> } else {
> delta
> };
On Wed, Jan 22, 2025 at 6:05 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Sat, 18 Jan 2025 17:02:24 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > On Fri, 17 Jan 2025 19:59:15 +0100
> > Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > > On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> > > <fujita.tomonori@gmail.com> wrote:
> > >>
> > >> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> > >> +/// If a value outside the range is given, the function will sleep
> > >> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
> > >
> > > I would emphasize with something like:
> > >
> > > `delta` must be within [0, `u32::MAX / 2`] microseconds;
> > > otherwise, it is erroneous behavior. That is, it is considered a bug
> > > to call this function with an out-of-range value, in which case the
> > > function will sleep for at least the maximum value in the range and
> > > may warn in the future.
> >
> > Thanks, I'll use the above instead.
> >
> > > In addition, I would add a new paragraph how the behavior differs
> > > w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
> > > `fsleep()` would do an infinite sleep instead. So I think it is
> > > important to highlight that.
> >
> > /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> > /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
> >
> > Looks ok?
> >
> > >> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> > >> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> > >> + // set the maximum value to u32::MAX / 2 microseconds.
> > >
> > > Nit: please use Markdown code spans in normal comments (no need for
> > > intra-doc links there).
> >
> > Understood.
> >
> > >> + let duration = if delta > MAX_DURATION || delta.is_negative() {
> > >> + // TODO: add WARN_ONCE() when it's supported.
> > >
> > > Ditto (also "Add").
> >
> > Oops, I'll fix.
> >
> > > By the way, can this be written differently maybe? e.g. using a range
> > > since it is `const`?
> >
> > A range can be used for a custom type?
>
> Yes, you can say `!(Delta::ZERO..MAX_DURATION).contains(&delta)`.
> (You'll need to add `Delta::ZERO`).
It would need to use ..= instead of .. to match the current check.
Alice
On Wed, 22 Jan 2025 18:06:58 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
>> > >> + let duration = if delta > MAX_DURATION || delta.is_negative() {
>> > >> + // TODO: add WARN_ONCE() when it's supported.
>> > >
>> > > Ditto (also "Add").
>> >
>> > Oops, I'll fix.
>> >
>> > > By the way, can this be written differently maybe? e.g. using a range
>> > > since it is `const`?
>> >
>> > A range can be used for a custom type?
>>
>> Yes, you can say `!(Delta::ZERO..MAX_DURATION).contains(&delta)`.
>> (You'll need to add `Delta::ZERO`).
>
> It would need to use ..= instead of .. to match the current check.
Neat, it works as follows.
let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
delta
} else {
MAX_DELTA
};
On Sat, Jan 18, 2025 at 9:02 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > /// The above behavior differs from the kernel's [`fsleep`], which could sleep > /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies). > > Looks ok? I think if that is meant as an intra-doc link, it would link to this function, rather than the C side one, so please add a link target to e.g. https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep. I would also say "the C side [`fsleep()`] or similar"; in other words, both are "kernel's" at this point. And perhaps I would simplify and say something like "The behavior above differs from the C side [`fsleep()`] for which out-of-range values mean "infinite timeout" instead." > A range can be used for a custom type? I was thinking of doing it through `as_nanos()`, but it may read worse, so please ignore it if so. > Do you mean something like the following? Yeah, I just meant using `let delta`. Cheers, Miguel
On Sat, 18 Jan 2025 13:17:29 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> On Sat, Jan 18, 2025 at 9:02 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
>> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
>>
>> Looks ok?
>
> I think if that is meant as an intra-doc link, it would link to this
> function, rather than the C side one, so please add a link target to
> e.g. https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep.
Added.
> I would also say "the C side [`fsleep()`] or similar"; in other words,
> both are "kernel's" at this point.
Agreed that "the C side" is better and updated the comment. I copied
that expression from the existing code; there are many "kernel's" in
rust/kernel/. "good first issues" for them?
You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
precedent for the C side functions.
> And perhaps I would simplify and say something like "The behavior
> above differs from the C side [`fsleep()`] for which out-of-range
> values mean "infinite timeout" instead."
Yeah, simpler is better. After applying the above changes, it ended up
as follows.
/// Sleeps for a given duration at least.
///
/// Equivalent to the C side [`fsleep`], flexible sleep function,
/// which automatically chooses the best sleep method based on a duration.
///
/// `delta` must be within [0, `i32::MAX`] microseconds;
/// otherwise, it is erroneous behavior. That is, it is considered a bug
/// to call this function with an out-of-range value, in which case the
/// function will sleep for at least the maximum value in the range and
/// may warn in the future.
///
/// The behavior above differs from the C side [`fsleep`] for which out-of-range
/// values mean "infinite timeout" instead.
///
/// This function can only be used in a nonatomic context.
///
/// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
pub fn fsleep(delta: Delta) {
>> A range can be used for a custom type?
>
> I was thinking of doing it through `as_nanos()`, but it may read
> worse, so please ignore it if so.
Ah, it might work. The following doesn't work. Seems that we need to
add another const like MAX_DELTA_NANOS or something. No strong
preference but I feel the current is simpler.
let delta = match delta.as_nanos() {
0..=MAX_DELTA.as_nanos() as i32 => delta,
_ => MAX_DELTA,
};
On Wed, Jan 22, 2025 at 7:57 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Agreed that "the C side" is better and updated the comment. I copied
> that expression from the existing code; there are many "kernel's" in
> rust/kernel/. "good first issues" for them?
Yeah, will do.
> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
> precedent for the C side functions.
There is no preference yet. It would be nice to be consistent, though.
The option of removing the `()` in all cases may be easier to check
for than the other, though the `()` give a bit of (possibly redundant)
information to the reader.
> Yeah, simpler is better. After applying the above changes, it ended up
> as follows.
Looks good, thanks!
Not sure if we should say "Equivalent" given it is not exactly the
same, but I am not a native speaker: I think it does not necessarily
need to be exactly the same to be "equivalent", but perhaps "Similar
to" or "Counterpart of" or something like that is better.
> Ah, it might work. The following doesn't work. Seems that we need to
> add another const like MAX_DELTA_NANOS or something. No strong
> preference but I feel the current is simpler.
>
> let delta = match delta.as_nanos() {
> 0..=MAX_DELTA.as_nanos() as i32 => delta,
> _ => MAX_DELTA,
> };
Yeah, don't worry about it too much :)
[ The language may get `const { ... }` to work there (it does in
nightly) though it wouldn't look good either. I think the `as i32`
would not be needed. ]
By the way, speaking of something related, do we want to make some of
the methods `fn`s be `const`?
Cheers,
Miguel
On Wed, 22 Jan 2025 11:21:51 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> Not sure if we should say "Equivalent" given it is not exactly the
> same, but I am not a native speaker: I think it does not necessarily
> need to be exactly the same to be "equivalent", but perhaps "Similar
> to" or "Counterpart of" or something like that is better.
I'm not a native speaker either, but seems that "equivalent" can be
used as "functionally equivalent". The official Rust docs also use
"equivalent" in this sense, "Equivalent to C’s unsigned char type"
https://doc.rust-lang.org/std/os/raw/type.c_uchar.html
There are many places where "equivalent" is used in this sense in
rust/kernel/. Seems that only rust/kernel/block/mq.rs uses a different
word, "counterpart" in this sense.
Possibly another "good first issue" to make this expression in the
tree consistent?
>> Ah, it might work. The following doesn't work. Seems that we need to
>> add another const like MAX_DELTA_NANOS or something. No strong
>> preference but I feel the current is simpler.
>>
>> let delta = match delta.as_nanos() {
>> 0..=MAX_DELTA.as_nanos() as i32 => delta,
>> _ => MAX_DELTA,
>> };
>
> Yeah, don't worry about it too much :)
>
> [ The language may get `const { ... }` to work there (it does in
> nightly) though it wouldn't look good either. I think the `as i32`
> would not be needed. ]
>
> By the way, speaking of something related, do we want to make some of
> the methods `fn`s be `const`?
Yeah. All the from_* functions are already const. Seems that making
as_* funcations (as_nanos, etc) const too make sense.
On Wed, Jan 22, 2025 at 7:57 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Sat, 18 Jan 2025 13:17:29 +0100
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Sat, Jan 18, 2025 at 9:02 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> >> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
> >>
> >> Looks ok?
> >
> > I think if that is meant as an intra-doc link, it would link to this
> > function, rather than the C side one, so please add a link target to
> > e.g. https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep.
>
> Added.
>
> > I would also say "the C side [`fsleep()`] or similar"; in other words,
> > both are "kernel's" at this point.
>
> Agreed that "the C side" is better and updated the comment. I copied
> that expression from the existing code; there are many "kernel's" in
> rust/kernel/. "good first issues" for them?
>
> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
> precedent for the C side functions.
I think that's a matter of taste. In the Rust ecosystem, fsleep is
more common, in the kernel ecosystem, fsleep() is more common. I've
seen both in Rust code at this point.
> > And perhaps I would simplify and say something like "The behavior
> > above differs from the C side [`fsleep()`] for which out-of-range
> > values mean "infinite timeout" instead."
>
> Yeah, simpler is better. After applying the above changes, it ended up
> as follows.
>
> /// Sleeps for a given duration at least.
> ///
> /// Equivalent to the C side [`fsleep`], flexible sleep function,
> /// which automatically chooses the best sleep method based on a duration.
> ///
> /// `delta` must be within [0, `i32::MAX`] microseconds;
I'd do `[0, i32::MAX]` instead for better rendering.
> /// otherwise, it is erroneous behavior. That is, it is considered a bug
> /// to call this function with an out-of-range value, in which case the
> /// function will sleep for at least the maximum value in the range and
> /// may warn in the future.
> ///
> /// The behavior above differs from the C side [`fsleep`] for which out-of-range
> /// values mean "infinite timeout" instead.
> ///
> /// This function can only be used in a nonatomic context.
> ///
> /// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
> pub fn fsleep(delta: Delta) {
>
>
> >> A range can be used for a custom type?
> >
> > I was thinking of doing it through `as_nanos()`, but it may read
> > worse, so please ignore it if so.
>
> Ah, it might work. The following doesn't work. Seems that we need to
> add another const like MAX_DELTA_NANOS or something. No strong
> preference but I feel the current is simpler.
>
> let delta = match delta.as_nanos() {
> 0..=MAX_DELTA.as_nanos() as i32 => delta,
> _ => MAX_DELTA,
> };
Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?
Alice
On Wed, 22 Jan 2025 09:23:33 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
>> > I would also say "the C side [`fsleep()`] or similar"; in other words,
>> > both are "kernel's" at this point.
>>
>> Agreed that "the C side" is better and updated the comment. I copied
>> that expression from the existing code; there are many "kernel's" in
>> rust/kernel/. "good first issues" for them?
>>
>> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
>> precedent for the C side functions.
>
> I think that's a matter of taste. In the Rust ecosystem, fsleep is
> more common, in the kernel ecosystem, fsleep() is more common. I've
> seen both in Rust code at this point.
Understood, I'll go with [`fsleep`].
>> > And perhaps I would simplify and say something like "The behavior
>> > above differs from the C side [`fsleep()`] for which out-of-range
>> > values mean "infinite timeout" instead."
>>
>> Yeah, simpler is better. After applying the above changes, it ended up
>> as follows.
>>
>> /// Sleeps for a given duration at least.
>> ///
>> /// Equivalent to the C side [`fsleep`], flexible sleep function,
>> /// which automatically chooses the best sleep method based on a duration.
>> ///
>> /// `delta` must be within [0, `i32::MAX`] microseconds;
>
> I'd do `[0, i32::MAX]` instead for better rendering.
Yeah, looks better after redering. I'll update it.
>> /// otherwise, it is erroneous behavior. That is, it is considered a bug
>> /// to call this function with an out-of-range value, in which case the
>> /// function will sleep for at least the maximum value in the range and
>> /// may warn in the future.
>> ///
>> /// The behavior above differs from the C side [`fsleep`] for which out-of-range
>> /// values mean "infinite timeout" instead.
>> ///
>> /// This function can only be used in a nonatomic context.
>> ///
>> /// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
>> pub fn fsleep(delta: Delta) {
>>
>>
>> >> A range can be used for a custom type?
>> >
>> > I was thinking of doing it through `as_nanos()`, but it may read
>> > worse, so please ignore it if so.
>>
>> Ah, it might work. The following doesn't work. Seems that we need to
>> add another const like MAX_DELTA_NANOS or something. No strong
>> preference but I feel the current is simpler.
>>
>> let delta = match delta.as_nanos() {
>> 0..=MAX_DELTA.as_nanos() as i32 => delta,
>> _ => MAX_DELTA,
>> };
>
> Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?
We need Delta type here so you meant:
let delta = std::cmp::min(delta, MAX_DELTA);
?
We also need to convert a negative delta to MAX_DELTA so we could do:
let delta = if delta.is_negative() {
MAX_DELTA
} else {
min(delta, MAX_DELTA)
};
looks a bit readable than the original code?
On Wed, Jan 22, 2025 at 07:44:05PM +0900, FUJITA Tomonori wrote: > On Wed, 22 Jan 2025 09:23:33 +0100 > Alice Ryhl <aliceryhl@google.com> wrote: > > >> > I would also say "the C side [`fsleep()`] or similar"; in other words, > >> > both are "kernel's" at this point. > >> > >> Agreed that "the C side" is better and updated the comment. I copied > >> that expression from the existing code; there are many "kernel's" in > >> rust/kernel/. "good first issues" for them? > >> > >> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any > >> precedent for the C side functions. > > > > I think that's a matter of taste. In the Rust ecosystem, fsleep is > > more common, in the kernel ecosystem, fsleep() is more common. I've > > seen both in Rust code at this point. > > Understood, I'll go with [`fsleep`]. > I would suggest using [`fsleep()`], in the same spirit of this paragraph in Documentation/process/maintainer-tip.rst: """ When a function is mentioned in the changelog, either the text body or the subject line, please use the format 'function_name()'. Omitting the brackets after the function name can be ambiguous:: Subject: subsys/component: Make reservation_count static reservation_count is only used in reservation_stats. Make it static. The variant with brackets is more precise:: Subject: subsys/component: Make reservation_count() static reservation_count() is only called from reservation_stats(). Make it static. """ , since fsleep() falls into the areas of tip tree. Regards, Boqun [...]
On Wed, 22 Jan 2025 23:03:24 -0800 Boqun Feng <boqun.feng@gmail.com> wrote: > On Wed, Jan 22, 2025 at 07:44:05PM +0900, FUJITA Tomonori wrote: >> On Wed, 22 Jan 2025 09:23:33 +0100 >> Alice Ryhl <aliceryhl@google.com> wrote: >> >> >> > I would also say "the C side [`fsleep()`] or similar"; in other words, >> >> > both are "kernel's" at this point. >> >> >> >> Agreed that "the C side" is better and updated the comment. I copied >> >> that expression from the existing code; there are many "kernel's" in >> >> rust/kernel/. "good first issues" for them? >> >> >> >> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any >> >> precedent for the C side functions. >> > >> > I think that's a matter of taste. In the Rust ecosystem, fsleep is >> > more common, in the kernel ecosystem, fsleep() is more common. I've >> > seen both in Rust code at this point. >> >> Understood, I'll go with [`fsleep`]. >> > > I would suggest using [`fsleep()`], in the same spirit of this paragraph > in Documentation/process/maintainer-tip.rst: > > """ > When a function is mentioned in the changelog, either the text body or the > subject line, please use the format 'function_name()'. Omitting the > brackets after the function name can be ambiguous:: > > Subject: subsys/component: Make reservation_count static > > reservation_count is only used in reservation_stats. Make it static. > > The variant with brackets is more precise:: > > Subject: subsys/component: Make reservation_count() static > > reservation_count() is only called from reservation_stats(). Make it > static. > """ > > , since fsleep() falls into the areas of tip tree. Thanks, I overlooked this. I had better to go with [`fsleep()`] then. I think that using two styles under rust/kernel isn't ideal. Should we make the [`function_name()`] style the preference under rust/kernel? Unless there's a subsystem that prefers the [`function_name`] style, that would be a different story.
On Thu, Jan 23, 2025 at 9:40 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Thanks, I overlooked this. I had better to go with [`fsleep()`] then. > > I think that using two styles under rust/kernel isn't ideal. Should we > make the [`function_name()`] style the preference under rust/kernel? Sounds good to me. I have a guidelines series on the backlog, so I can add it there. Though there is a question about whether we should do the same for Rust ones or not. For Rust, it is a bit clearer already since it uses e.g. CamelCase for types. > Unless there's a subsystem that prefers the [`function_name`] style, > that would be a different story. I think we have a good chance to try to make things consistent across the kernel (for Rust) wherever possible, like for formatting, since all the code is new, i.e. to try to avoid variations between subsystems, even if in the C side had them originally. Exceptions as usual may be needed, but unless there is a good reason, I think we should try to stay consistent. Cheers, Miguel
On Wed, Jan 22, 2025 at 11:44 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> >> >> A range can be used for a custom type?
> >> >
> >> > I was thinking of doing it through `as_nanos()`, but it may read
> >> > worse, so please ignore it if so.
> >>
> >> Ah, it might work. The following doesn't work. Seems that we need to
> >> add another const like MAX_DELTA_NANOS or something. No strong
> >> preference but I feel the current is simpler.
> >>
> >> let delta = match delta.as_nanos() {
> >> 0..=MAX_DELTA.as_nanos() as i32 => delta,
> >> _ => MAX_DELTA,
> >> };
> >
> > Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?
>
> We need Delta type here so you meant:
>
> let delta = std::cmp::min(delta, MAX_DELTA);
If `Delta` implements the `Ord` trait, then you can write `Delta::min`
to take the minimum of two deltas. It's equivalent to `std::cmp::min`,
of course.
> We also need to convert a negative delta to MAX_DELTA so we could do:
>
> let delta = if delta.is_negative() {
> MAX_DELTA
> } else {
> min(delta, MAX_DELTA)
> };
>
> looks a bit readable than the original code?
At that point we might as well write
if delta.is_negative() || delta > MAX_DELTA
and skip the call to `min`.
Alice
On Wed, 22 Jan 2025 11:47:38 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
>> >> Ah, it might work. The following doesn't work. Seems that we need to
>> >> add another const like MAX_DELTA_NANOS or something. No strong
>> >> preference but I feel the current is simpler.
>> >>
>> >> let delta = match delta.as_nanos() {
>> >> 0..=MAX_DELTA.as_nanos() as i32 => delta,
>> >> _ => MAX_DELTA,
>> >> };
>> >
>> > Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?
>>
>> We need Delta type here so you meant:
>>
>> let delta = std::cmp::min(delta, MAX_DELTA);
>
> If `Delta` implements the `Ord` trait, then you can write `Delta::min`
> to take the minimum of two deltas. It's equivalent to `std::cmp::min`,
> of course.
Ah, nice.
>> We also need to convert a negative delta to MAX_DELTA so we could do:
>>
>> let delta = if delta.is_negative() {
>> MAX_DELTA
>> } else {
>> min(delta, MAX_DELTA)
>> };
>>
>> looks a bit readable than the original code?
>
> At that point we might as well write
>
> if delta.is_negative() || delta > MAX_DELTA
>
> and skip the call to `min`.
Yeah, it's what the original code does. I'll leave this code alone.
On Thu, Jan 16, 2025 at 5:42 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.
>
> 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 | 43 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 55 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 dcf827a61b52..d16aeda7a558 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -26,6 +26,7 @@
> #include "slab.c"
> #include "spinlock.c"
> #include "task.c"
> +#include "time.c"
> #include "uaccess.c"
> #include "vmalloc.c"
> #include "wait.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 da54a70f8f1f..3be2bf578519 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..db5c08b0f230
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,43 @@
> +// 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 super::Delta;
> +use crate::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 0 or greater and no more than `u32::MAX / 2` microseconds.
> +/// If a value outside the range is given, the function will sleep
> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
> +///
> +/// This function can only be used in a nonatomic context.
> +pub fn fsleep(delta: Delta) {
> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> + // set the maximum value to u32::MAX / 2 microseconds.
> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
Hmm, is this value correct on 64-bit platforms?
> + let duration = if delta > MAX_DURATION || delta.is_negative() {
> + // TODO: add WARN_ONCE() when it's supported.
> + MAX_DURATION
> + } else {
> + delta
> + };
> +
> + // SAFETY: FFI call.
> + unsafe {
This safety comment is incomplete. You can say this:
// SAFETY: It is always safe to call fsleep with any duration.
Alice
On Thu, 16 Jan 2025 10:27:02 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
>> +/// This function can only be used in a nonatomic context.
>> +pub fn fsleep(delta: Delta) {
>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
>> + // set the maximum value to u32::MAX / 2 microseconds.
>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>
> Hmm, is this value correct on 64-bit platforms?
You meant that the maximum can be longer on 64-bit platforms? 2147484
milliseconds is long enough for fsleep's duration?
If you prefer, I use different maximum durations for 64-bit and 32-bit
platforms, respectively.
>> + let duration = if delta > MAX_DURATION || delta.is_negative() {
>> + // TODO: add WARN_ONCE() when it's supported.
>> + MAX_DURATION
>> + } else {
>> + delta
>> + };
>> +
>> + // SAFETY: FFI call.
>> + unsafe {
>
> This safety comment is incomplete. You can say this:
>
> // SAFETY: It is always safe to call fsleep with any duration.
Thanks, I'll use your safety comment.
On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> On Thu, 16 Jan 2025 10:27:02 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>>> +/// This function can only be used in a nonatomic context.
>>> +pub fn fsleep(delta: Delta) {
>>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
>>> + // set the maximum value to u32::MAX / 2 microseconds.
>>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>>
>> Hmm, is this value correct on 64-bit platforms?
>
> You meant that the maximum can be longer on 64-bit platforms? 2147484
> milliseconds is long enough for fsleep's duration?
>
> If you prefer, I use different maximum durations for 64-bit and 32-bit
> platforms, respectively.
How about the following?
const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > On Thu, 16 Jan 2025 10:27:02 +0100
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >>> +/// This function can only be used in a nonatomic context.
> >>> +pub fn fsleep(delta: Delta) {
> >>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> >>> + // set the maximum value to u32::MAX / 2 microseconds.
> >>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> >>
> >> Hmm, is this value correct on 64-bit platforms?
> >
> > You meant that the maximum can be longer on 64-bit platforms? 2147484
> > milliseconds is long enough for fsleep's duration?
> >
> > If you prefer, I use different maximum durations for 64-bit and 32-bit
> > platforms, respectively.
>
> How about the following?
>
> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
Why is there a maximum in the first place? Are you worried about
overflow on the C side?
Alice
On Fri, 17 Jan 2025 10:13:08 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
> On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>
>> > On Thu, 16 Jan 2025 10:27:02 +0100
>> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >
>> >>> +/// This function can only be used in a nonatomic context.
>> >>> +pub fn fsleep(delta: Delta) {
>> >>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> >>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
>> >>> + // set the maximum value to u32::MAX / 2 microseconds.
>> >>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> >>
>> >> Hmm, is this value correct on 64-bit platforms?
>> >
>> > You meant that the maximum can be longer on 64-bit platforms? 2147484
>> > milliseconds is long enough for fsleep's duration?
>> >
>> > If you prefer, I use different maximum durations for 64-bit and 32-bit
>> > platforms, respectively.
>>
>> How about the following?
>>
>> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
>
> Why is there a maximum in the first place? Are you worried about
> overflow on the C side?
Yeah, Boqun is concerned that an incorrect input (a negative value or
an overflow on the C side) leads to unintentional infinite sleep:
https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 10:13:08 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >>
> >> > On Thu, 16 Jan 2025 10:27:02 +0100
> >> > Alice Ryhl <aliceryhl@google.com> wrote:
> >> >
> >> >>> +/// This function can only be used in a nonatomic context.
> >> >>> +pub fn fsleep(delta: Delta) {
> >> >>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >> >>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> >> >>> + // set the maximum value to u32::MAX / 2 microseconds.
> >> >>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> >> >>
> >> >> Hmm, is this value correct on 64-bit platforms?
> >> >
> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
> >> > milliseconds is long enough for fsleep's duration?
> >> >
> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
> >> > platforms, respectively.
> >>
> >> How about the following?
> >>
> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
> >
> > Why is there a maximum in the first place? Are you worried about
> > overflow on the C side?
>
> Yeah, Boqun is concerned that an incorrect input (a negative value or
> an overflow on the C side) leads to unintentional infinite sleep:
>
> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
Okay, can you explain in the comment that this maximum value prevents
integer overflow inside fsleep?
Alice
On Fri, 17 Jan 2025 14:05:52 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
> On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Fri, 17 Jan 2025 10:13:08 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
>> > <fujita.tomonori@gmail.com> wrote:
>> >>
>> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
>> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> >>
>> >> > On Thu, 16 Jan 2025 10:27:02 +0100
>> >> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >> >
>> >> >>> +/// This function can only be used in a nonatomic context.
>> >> >>> +pub fn fsleep(delta: Delta) {
>> >> >>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> >> >>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
>> >> >>> + // set the maximum value to u32::MAX / 2 microseconds.
>> >> >>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> >> >>
>> >> >> Hmm, is this value correct on 64-bit platforms?
>> >> >
>> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
>> >> > milliseconds is long enough for fsleep's duration?
>> >> >
>> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
>> >> > platforms, respectively.
>> >>
>> >> How about the following?
>> >>
>> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
>> >
>> > Why is there a maximum in the first place? Are you worried about
>> > overflow on the C side?
>>
>> Yeah, Boqun is concerned that an incorrect input (a negative value or
>> an overflow on the C side) leads to unintentional infinite sleep:
>>
>> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
>
> Okay, can you explain in the comment that this maximum value prevents
> integer overflow inside fsleep?
Surely, how about the following?
pub fn fsleep(delta: Delta) {
// The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
// Considering that fsleep rounds up the duration to the nearest millisecond,
// set the maximum value to u32::MAX / 2 microseconds to prevent integer
// overflow inside fsleep, which could lead to unintentional infinite sleep.
const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
On Fri, Jan 17, 2025 at 3:20 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 14:05:52 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> On Fri, 17 Jan 2025 10:13:08 +0100
> >> Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
> >> > <fujita.tomonori@gmail.com> wrote:
> >> >>
> >> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
> >> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >> >>
> >> >> > On Thu, 16 Jan 2025 10:27:02 +0100
> >> >> > Alice Ryhl <aliceryhl@google.com> wrote:
> >> >> >
> >> >> >>> +/// This function can only be used in a nonatomic context.
> >> >> >>> +pub fn fsleep(delta: Delta) {
> >> >> >>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >> >> >>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
> >> >> >>> + // set the maximum value to u32::MAX / 2 microseconds.
> >> >> >>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> >> >> >>
> >> >> >> Hmm, is this value correct on 64-bit platforms?
> >> >> >
> >> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
> >> >> > milliseconds is long enough for fsleep's duration?
> >> >> >
> >> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
> >> >> > platforms, respectively.
> >> >>
> >> >> How about the following?
> >> >>
> >> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
> >> >
> >> > Why is there a maximum in the first place? Are you worried about
> >> > overflow on the C side?
> >>
> >> Yeah, Boqun is concerned that an incorrect input (a negative value or
> >> an overflow on the C side) leads to unintentional infinite sleep:
> >>
> >> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
> >
> > Okay, can you explain in the comment that this maximum value prevents
> > integer overflow inside fsleep?
>
> Surely, how about the following?
>
> pub fn fsleep(delta: Delta) {
> // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> // Considering that fsleep rounds up the duration to the nearest millisecond,
> // set the maximum value to u32::MAX / 2 microseconds to prevent integer
> // overflow inside fsleep, which could lead to unintentional infinite sleep.
> const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
Hmm ... this is phrased as-if the problem is on 32-bit machines, but
the problem is that fsleep casts an `unsigned long` to `unsigned int`
which can overflow on 64-bit machines. I would instead say this
prevents overflow on 64-bit machines when casting to an int.
Also, it might be cleaner to just use `i32::MAX as i64` instead of u32.
Alice
On Fri, 17 Jan 2025 15:31:07 +0100
Alice Ryhl <aliceryhl@google.com> wrote:
> On Fri, Jan 17, 2025 at 3:20 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Fri, 17 Jan 2025 14:05:52 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> > On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
>> > <fujita.tomonori@gmail.com> wrote:
>> >>
>> >> On Fri, 17 Jan 2025 10:13:08 +0100
>> >> Alice Ryhl <aliceryhl@google.com> wrote:
>> >>
>> >> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
>> >> > <fujita.tomonori@gmail.com> wrote:
>> >> >>
>> >> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
>> >> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> >> >>
>> >> >> > On Thu, 16 Jan 2025 10:27:02 +0100
>> >> >> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >> >> >
>> >> >> >>> +/// This function can only be used in a nonatomic context.
>> >> >> >>> +pub fn fsleep(delta: Delta) {
>> >> >> >>> + // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> >> >> >>> + // Considering that fsleep rounds up the duration to the nearest millisecond,
>> >> >> >>> + // set the maximum value to u32::MAX / 2 microseconds.
>> >> >> >>> + const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> >> >> >>
>> >> >> >> Hmm, is this value correct on 64-bit platforms?
>> >> >> >
>> >> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
>> >> >> > milliseconds is long enough for fsleep's duration?
>> >> >> >
>> >> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
>> >> >> > platforms, respectively.
>> >> >>
>> >> >> How about the following?
>> >> >>
>> >> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
>> >> >
>> >> > Why is there a maximum in the first place? Are you worried about
>> >> > overflow on the C side?
>> >>
>> >> Yeah, Boqun is concerned that an incorrect input (a negative value or
>> >> an overflow on the C side) leads to unintentional infinite sleep:
>> >>
>> >> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
>> >
>> > Okay, can you explain in the comment that this maximum value prevents
>> > integer overflow inside fsleep?
>>
>> Surely, how about the following?
>>
>> pub fn fsleep(delta: Delta) {
>> // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> // Considering that fsleep rounds up the duration to the nearest millisecond,
>> // set the maximum value to u32::MAX / 2 microseconds to prevent integer
>> // overflow inside fsleep, which could lead to unintentional infinite sleep.
>> const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>
> Hmm ... this is phrased as-if the problem is on 32-bit machines, but
> the problem is that fsleep casts an `unsigned long` to `unsigned int`
> which can overflow on 64-bit machines. I would instead say this
> prevents overflow on 64-bit machines when casting to an int.
Yeah, but DIV_ROUND_UP in fsync() could also cause overflow before
casting ulong to uint for calling msleep() (it could happen on both
32-bit and 64-bit).
The following looks ok?
The maximum value is set to `u32::MAX / 2` microseconds to prevent integer
overflow inside fsleep, which could lead to unintentional infinite sleep.
> Also, it might be cleaner to just use `i32::MAX as i64` instead of u32.
You meant that using i32::MAX instead of u32::MAX / 2 (and u32::MAX >>
1) might be cleaner? I might think so too.
© 2016 - 2025 Red Hat, Inc.