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]
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
rust/helpers/helpers.c | 1 +
rust/helpers/time.c | 8 +++++++
rust/kernel/time.rs | 1 +
rust/kernel/time/delay.rs | 49 +++++++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+)
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 e1c21eba9b15..48143cdd26b3 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -33,6 +33,7 @@
#include "spinlock.c"
#include "sync.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 a8089a98da9e..863385905029 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -24,6 +24,7 @@
//! 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;
pub mod hrtimer;
/// The number of nanoseconds per microsecond.
diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
new file mode 100644
index 000000000000..02b8731433c7
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,49 @@
+// 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 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) {
+ // The maximum value is set to `i32::MAX` microseconds to prevent integer
+ // overflow inside fsleep, which could lead to unintentional infinite sleep.
+ const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
+
+ let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
+ delta
+ } else {
+ // TODO: Add WARN_ONCE() when it's supported.
+ MAX_DELTA
+ };
+
+ // SAFETY: It is always safe to call `fsleep()` with any duration.
+ 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)
+ }
+}
--
2.43.0Hi Tomonori, "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes: > 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. I get an error when building this patch for arm32: + kernel-make -j 96 O=/home/aeh/src/linux-rust/test-build-arm-1.78.0 vmlinux modules ld.lld: error: undefined symbol: __aeabi_uldivmod >>> referenced by kernel.df165ca450b1fd1-cgu.0 >>> rust/kernel.o:(kernel::time::delay::fsleep) in archive vmlinux.a >>> did you mean: __aeabi_uidivmod >>> defined in: vmlinux.a(arch/arm/lib/lib1funcs.o) Looks like a division function of some sort is not defined. Can you reproduce? Best regards, Andreas Hindborg
On Mon, 28 Apr 2025 20:16:47 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:
> Hi Tomonori,
>
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
>> 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.
>
> I get an error when building this patch for arm32:
>
> + kernel-make -j 96 O=/home/aeh/src/linux-rust/test-build-arm-1.78.0 vmlinux modules
> ld.lld: error: undefined symbol: __aeabi_uldivmod
> >>> referenced by kernel.df165ca450b1fd1-cgu.0
> >>> rust/kernel.o:(kernel::time::delay::fsleep) in archive vmlinux.a
> >>> did you mean: __aeabi_uidivmod
> >>> defined in: vmlinux.a(arch/arm/lib/lib1funcs.o)
>
> Looks like a division function of some sort is not defined. Can you
> reproduce?
Ah, 64-bit integer division on 32-bit architectures.
I think that the DRM QR driver has the same problem:
https://lore.kernel.org/rust-for-linux/CANiq72ke45eOwckMhWHvmwxc03dxr4rnxxKvx+HvWdBLopZfrQ@mail.gmail.com/
It appears that there is still no consensus on how to resolve it. CC
the participants in the above thread.
I think that we can drop this patch and better to focus on Instant and
Delta types in this merge window.
With the patch below, this issue could be resolved like the C side,
but I'm not sure whether we can reach a consensus quickly.
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 48143cdd26b3..c44d45960eb1 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -19,6 +19,7 @@
#include "io.c"
#include "jump_label.c"
#include "kunit.c"
+#include "math64.c"
#include "mutex.c"
#include "page.c"
#include "platform.c"
diff --git a/rust/helpers/math64.c b/rust/helpers/math64.c
new file mode 100644
index 000000000000..f94708cf8fcb
--- /dev/null
+++ b/rust/helpers/math64.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/math64.h>
+
+s64 rust_helper_div64_s64(s64 dividend, s64 divisor)
+{
+ return div64_s64(dividend, divisor);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index de07aadd1ff5..d272e0b0b05d 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -60,6 +60,7 @@
#[cfg(CONFIG_KUNIT)]
pub mod kunit;
pub mod list;
+pub mod math64;
pub mod miscdevice;
#[cfg(CONFIG_NET)]
pub mod net;
diff --git a/rust/kernel/math64.rs b/rust/kernel/math64.rs
new file mode 100644
index 000000000000..523e47911859
--- /dev/null
+++ b/rust/kernel/math64.rs
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! 64-bit integer arithmetic helpers.
+//!
+//! C header: [`include/linux/math64.h`](srctree/include/linux/math64.h)
+
+/// Divide a signed 64-bit integer by another signed 64-bit integer.
+#[inline]
+pub fn div64_s64(dividend: i64, divisor: i64) -> i64 {
+ // SAFETY: Calling `div64_s64()` is safe as long as `divisor` is non-zero.
+ unsafe { bindings::div64_s64(dividend, divisor) }
+}
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 863385905029..7b5255893929 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -24,6 +24,8 @@
//! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
//! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
+use crate::math64;
+
pub mod delay;
pub mod hrtimer;
@@ -229,13 +231,16 @@ pub const fn as_nanos(self) -> i64 {
/// Return the smallest number of microseconds greater than or equal
/// to the value in the [`Delta`].
#[inline]
- pub const fn as_micros_ceil(self) -> i64 {
- self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+ pub fn as_micros_ceil(self) -> i64 {
+ math64::div64_s64(
+ self.as_nanos().saturating_add(NSEC_PER_USEC - 1),
+ NSEC_PER_USEC,
+ )
}
/// Return the number of milliseconds in the [`Delta`].
#[inline]
- pub const fn as_millis(self) -> i64 {
- self.as_nanos() / NSEC_PER_MSEC
+ pub fn as_millis(self) -> i64 {
+ math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
}
}
base-commit: da37ddd3f607897d039d82e6621671c3f7baa886
--
2.43.0
On Tue, Apr 29, 2025, at 15:17, FUJITA Tomonori wrote:
> On Mon, 28 Apr 2025 20:16:47 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote:
> /// Return the number of milliseconds in the [`Delta`].
> #[inline]
> - pub const fn as_millis(self) -> i64 {
> - self.as_nanos() / NSEC_PER_MSEC
> + pub fn as_millis(self) -> i64 {
> + math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
> }
> }
I think simply calling ktime_to_ms()/ktime_to_us() should result
in reasonably efficient code, since the C version is able to
convert the constant divisor into a multiply/shift operation.
div64_s64() itself is never optimized for constant arguments since
the C version is not inline, if any driver needs those, this is
better done open-coded so hopefully it gets caught in review
when this is called in a fast path.
Arnd
On Tue, Apr 29, 2025, at 8:51 AM, Arnd Bergmann wrote:
> On Tue, Apr 29, 2025, at 15:17, FUJITA Tomonori wrote:
>> On Mon, 28 Apr 2025 20:16:47 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> /// Return the number of milliseconds in the [`Delta`].
>> #[inline]
>> - pub const fn as_millis(self) -> i64 {
>> - self.as_nanos() / NSEC_PER_MSEC
>> + pub fn as_millis(self) -> i64 {
>> + math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
>> }
>> }
>
> I think simply calling ktime_to_ms()/ktime_to_us() should result
> in reasonably efficient code, since the C version is able to
> convert the constant divisor into a multiply/shift operation.
>
Well, before we jump into this, I would like to understand why
this is not optimized with multiply/shift operations on arm in
Rust code. Ideally all the dividing constants cases should not
need to call a C function.
Regards,
Boqun
> div64_s64() itself is never optimized for constant arguments since
> the C version is not inline, if any driver needs those, this is
> better done open-coded so hopefully it gets caught in review
> when this is called in a fast path.
>
> Arnd
On Tue, Apr 29, 2025, at 18:03, Boqun Feng wrote:
> On Tue, Apr 29, 2025, at 8:51 AM, Arnd Bergmann wrote:
>> On Tue, Apr 29, 2025, at 15:17, FUJITA Tomonori wrote:
>>> On Mon, 28 Apr 2025 20:16:47 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>> /// Return the number of milliseconds in the [`Delta`].
>>> #[inline]
>>> - pub const fn as_millis(self) -> i64 {
>>> - self.as_nanos() / NSEC_PER_MSEC
>>> + pub fn as_millis(self) -> i64 {
>>> + math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
>>> }
>>> }
>>
>> I think simply calling ktime_to_ms()/ktime_to_us() should result
>> in reasonably efficient code, since the C version is able to
>> convert the constant divisor into a multiply/shift operation.
>>
>
> Well, before we jump into this, I would like to understand why
> this is not optimized with multiply/shift operations on arm in
> Rust code. Ideally all the dividing constants cases should not
> need to call a C function.
I think it's just because nobody has rewritten the
macros from include/asm-generic/div64.h into rust code.
The compiler could do the same transformation, but they
generally just fall back to calling a libgcc function.
Arnd
On Tue, Apr 29, 2025 at 06:11:02PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 29, 2025, at 18:03, Boqun Feng wrote:
> > On Tue, Apr 29, 2025, at 8:51 AM, Arnd Bergmann wrote:
> >> On Tue, Apr 29, 2025, at 15:17, FUJITA Tomonori wrote:
> >>> On Mon, 28 Apr 2025 20:16:47 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>> /// Return the number of milliseconds in the [`Delta`].
> >>> #[inline]
> >>> - pub const fn as_millis(self) -> i64 {
> >>> - self.as_nanos() / NSEC_PER_MSEC
> >>> + pub fn as_millis(self) -> i64 {
> >>> + math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
> >>> }
> >>> }
> >>
> >> I think simply calling ktime_to_ms()/ktime_to_us() should result
> >> in reasonably efficient code, since the C version is able to
> >> convert the constant divisor into a multiply/shift operation.
> >>
> >
> > Well, before we jump into this, I would like to understand why
> > this is not optimized with multiply/shift operations on arm in
> > Rust code. Ideally all the dividing constants cases should not
> > need to call a C function.
>
> I think it's just because nobody has rewritten the
> macros from include/asm-generic/div64.h into rust code.
>
> The compiler could do the same transformation, but they
> generally just fall back to calling a libgcc function.
>
FWIW, I found this:
https://github.com/llvm/llvm-project/issues/63731
seems a WIP though.
Would it make sense if we rely on compiler optimization when it's
avaiable (for x86_64, arm64, riscv, etc), and only call ktime_to_ms() if
not? The downside of calling ktime_to_ms() are:
* it's a call function, and cannot be inlined with LTO or INLINE_HELPER:
https://lore.kernel.org/all/20250319205141.3528424-1-gary@garyguo.net/
* it doesn't provide the overflow checking even if
CONFIG_RUST_OVERFLOW_CHECKS=y
Thoughts?
Regards,
Boqun
> Arnd
On Tue, Apr 29, 2025, at 19:15, Boqun Feng wrote:
> On Tue, Apr 29, 2025 at 06:11:02PM +0200, Arnd Bergmann wrote:
>> On Tue, Apr 29, 2025, at 18:03, Boqun Feng wrote:
>
> Would it make sense if we rely on compiler optimization when it's
> avaiable (for x86_64, arm64, riscv, etc), and only call ktime_to_ms() if
> not? The downside of calling ktime_to_ms() are:
>
> * it's a call function, and cannot be inlined with LTO or INLINE_HELPER:
>
> https://lore.kernel.org/all/20250319205141.3528424-1-gary@garyguo.net/
>
> * it doesn't provide the overflow checking even if
> CONFIG_RUST_OVERFLOW_CHECKS=y
>
> Thoughts?
The function call overhead is tiny compared to replacing a 64-bit
division with a constant mult/shift.
What is the possible overflow that can happen here? For a constant
division at least there is no chance of divide-by-zero. Do you mean
truncating to 32 bit?
Arnd
On Tue, Apr 29, 2025 at 08:33:47PM +0200, Arnd Bergmann wrote: > On Tue, Apr 29, 2025, at 19:15, Boqun Feng wrote: > > On Tue, Apr 29, 2025 at 06:11:02PM +0200, Arnd Bergmann wrote: > >> On Tue, Apr 29, 2025, at 18:03, Boqun Feng wrote: > > > > Would it make sense if we rely on compiler optimization when it's > > avaiable (for x86_64, arm64, riscv, etc), and only call ktime_to_ms() if > > not? The downside of calling ktime_to_ms() are: > > > > * it's a call function, and cannot be inlined with LTO or INLINE_HELPER: > > > > https://lore.kernel.org/all/20250319205141.3528424-1-gary@garyguo.net/ > > > > * it doesn't provide the overflow checking even if > > CONFIG_RUST_OVERFLOW_CHECKS=y > > > > Thoughts? > > The function call overhead is tiny compared to replacing a 64-bit > division with a constant mult/shift. > Just to be clear, are you essientially saying that even in C, ktime_to_ms() is not worth inlining? Because the call overhead is tiny compared to the function own cost? My impression is that on x86 at least, function call is 10+ cycles, and multiply is 3 cycles, so I would think that ktime_to_ms() itself is at most 10 cycles. Maybe I'm out of date of the modern micro-architecture? > What is the possible overflow that can happen here? For a constant > division at least there is no chance of divide-by-zero. Do you mean > truncating to 32 bit? > I was referring the last part of Miguel's email: https://lore.kernel.org/rust-for-linux/CANiq72mMRpY4NC4_8v_wDpq6Z3qs99Y8gXd-7XL_3Bed58gkJg@mail.gmail.com/ Regards, Boqun > Arnd
On Tue, Apr 29, 2025 at 12:14:04PM -0700, Boqun Feng wrote: > On Tue, Apr 29, 2025 at 08:33:47PM +0200, Arnd Bergmann wrote: > > On Tue, Apr 29, 2025, at 19:15, Boqun Feng wrote: > > > On Tue, Apr 29, 2025 at 06:11:02PM +0200, Arnd Bergmann wrote: > > >> On Tue, Apr 29, 2025, at 18:03, Boqun Feng wrote: > > > > > > Would it make sense if we rely on compiler optimization when it's > > > avaiable (for x86_64, arm64, riscv, etc), and only call ktime_to_ms() if In case I wasn't clear, nowadays Rust compiler supports optimizating constant division into multi/shift on x86_64, arm64, riscv already, the optimization is only not availabe for arm32. (It's actually an optimization provided by LLVM I think) Regards, Boqun > > > not? The downside of calling ktime_to_ms() are: > > > > > > * it's a call function, and cannot be inlined with LTO or INLINE_HELPER: > > > > > > https://lore.kernel.org/all/20250319205141.3528424-1-gary@garyguo.net/ > > > > > > * it doesn't provide the overflow checking even if > > > CONFIG_RUST_OVERFLOW_CHECKS=y > > > > > > Thoughts? > > > > The function call overhead is tiny compared to replacing a 64-bit > > division with a constant mult/shift. > > > > Just to be clear, are you essientially saying that even in C, > ktime_to_ms() is not worth inlining? Because the call overhead is tiny > compared to the function own cost? > > My impression is that on x86 at least, function call is 10+ cycles, and > multiply is 3 cycles, so I would think that ktime_to_ms() itself is at > most 10 cycles. Maybe I'm out of date of the modern micro-architecture? > > > What is the possible overflow that can happen here? For a constant > > division at least there is no chance of divide-by-zero. Do you mean > > truncating to 32 bit? > > > > I was referring the last part of Miguel's email: > > https://lore.kernel.org/rust-for-linux/CANiq72mMRpY4NC4_8v_wDpq6Z3qs99Y8gXd-7XL_3Bed58gkJg@mail.gmail.com/ > > Regards, > Boqun > > > Arnd
On Tue, Apr 29, 2025 at 3:17 PM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Ah, 64-bit integer division on 32-bit architectures. > > I think that the DRM QR driver has the same problem: > > https://lore.kernel.org/rust-for-linux/CANiq72ke45eOwckMhWHvmwxc03dxr4rnxxKvx+HvWdBLopZfrQ@mail.gmail.com/ Yeah. > It appears that there is still no consensus on how to resolve it. CC > the participants in the above thread. > > I think that we can drop this patch and better to focus on Instant and > Delta types in this merge window. > > With the patch below, this issue could be resolved like the C side, > but I'm not sure whether we can reach a consensus quickly. I think using the C ones is fine for the moment, but up to what arm and others think. This one is also a constant, so something simpler may be better (and it is also a power of 10 divisor, so the other suggestions on that thread would apply too). > +/// Divide a signed 64-bit integer by another signed 64-bit integer. Perhaps an example wouldn't hurt. And if `unsafe` or fallible is picked, then the example allows to showcase (and test) what happens in the zero divisor case that Christian points out. By the way, apart from that case, we should also consider the min/-1 case. We may want an assert under `CONFIG_RUST_OVERFLOW_CHECKS=y`, too. And it wouldn't hurt to test a few other boundary values, with the new `#[test]` support. Thanks! Cheers, Miguel
On Tue, 29 Apr 2025 16:35:09 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>> It appears that there is still no consensus on how to resolve it. CC
>> the participants in the above thread.
>>
>> I think that we can drop this patch and better to focus on Instant and
>> Delta types in this merge window.
>>
>> With the patch below, this issue could be resolved like the C side,
>> but I'm not sure whether we can reach a consensus quickly.
>
> I think using the C ones is fine for the moment, but up to what arm
> and others think.
I don't think anyone would disagree with the rule Russell mentioned
that expensive 64-bit by 64-bit division should be avoided unless
absolutely necessary so we should go with function div_s64() instead
of function div64_s64().
The DRM QR driver was already fixed by avoiding 64-bit division, so
for now, the only place where we still need to solve this issue is the
time abstraction. So, it seems like Arnd's suggestion to simply call
ktime_to_ms() or ktime_to_us() is the right way to go.
> This one is also a constant, so something simpler may be better (and
> it is also a power of 10 divisor, so the other suggestions on that
> thread would apply too).
One downside of calling the C's functions is that the as_micros/millis
methods can no longer be const fn (or is there a way to make that
work?). We could implement the method Paolo suggested from Hacker's
Delight, 2nd edition in Rust and keep using const fn, but I'm not sure
if it's really worth it.
Any thoughts?
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index a8089a98da9e..daf9e5925e47 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -229,12 +229,116 @@ pub const fn as_nanos(self) -> i64 {
/// to the value in the [`Delta`].
#[inline]
pub const fn as_micros_ceil(self) -> i64 {
- self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+ const NSEC_PER_USEC_EXP: u32 = 3;
+
+ div10::<NSEC_PER_USEC_EXP>(self.as_nanos().saturating_add(NSEC_PER_USEC - 1))
}
/// Return the number of milliseconds in the [`Delta`].
#[inline]
pub const fn as_millis(self) -> i64 {
- self.as_nanos() / NSEC_PER_MSEC
+ const NSEC_PER_MSEC_EXP: u32 = 6;
+
+ div10::<NSEC_PER_MSEC_EXP>(self.as_nanos())
+ }
+}
+
+/// Precomputed magic constants for division by powers of 10.
+///
+/// Each entry corresponds to dividing a number by `10^exp`, where `exp` ranges from 0 to 18.
+/// These constants were computed using the algorithm from Hacker's Delight, 2nd edition.
+struct MagicMul {
+ mult: u64,
+ shift: u32,
+}
+
+const DIV10: [MagicMul; 19] = [
+ MagicMul {
+ mult: 0x1,
+ shift: 0,
+ },
+ MagicMul {
+ mult: 0x6666666666666667,
+ shift: 66,
+ },
+ MagicMul {
+ mult: 0xA3D70A3D70A3D70B,
+ shift: 70,
+ },
+ MagicMul {
+ mult: 0x20C49BA5E353F7CF,
+ shift: 71,
+ },
+ MagicMul {
+ mult: 0x346DC5D63886594B,
+ shift: 75,
+ },
+ MagicMul {
+ mult: 0x29F16B11C6D1E109,
+ shift: 78,
+ },
+ MagicMul {
+ mult: 0x431BDE82D7B634DB,
+ shift: 82,
+ },
+ MagicMul {
+ mult: 0xD6BF94D5E57A42BD,
+ shift: 87,
+ },
+ MagicMul {
+ mult: 0x55E63B88C230E77F,
+ shift: 89,
+ },
+ MagicMul {
+ mult: 0x112E0BE826D694B3,
+ shift: 90,
+ },
+ MagicMul {
+ mult: 0x036F9BFB3AF7B757,
+ shift: 91,
+ },
+ MagicMul {
+ mult: 0x00AFEBFF0BCB24AB,
+ shift: 92,
+ },
+ MagicMul {
+ mult: 0x232F33025BD42233,
+ shift: 101,
+ },
+ MagicMul {
+ mult: 0x384B84D092ED0385,
+ shift: 105,
+ },
+ MagicMul {
+ mult: 0x0B424DC35095CD81,
+ shift: 106,
+ },
+ MagicMul {
+ mult: 0x480EBE7B9D58566D,
+ shift: 112,
+ },
+ MagicMul {
+ mult: 0x39A5652FB1137857,
+ shift: 115,
+ },
+ MagicMul {
+ mult: 0x5C3BD5191B525A25,
+ shift: 119,
+ },
+ MagicMul {
+ mult: 0x12725DD1D243ABA1,
+ shift: 120,
+ },
+];
+
+const fn div10<const EXP: u32>(val: i64) -> i64 {
+ crate::build_assert!(EXP <= 18);
+ let MagicMul { mult, shift } = DIV10[EXP as usize];
+ let abs_val = val.wrapping_abs() as u64;
+ let ret = ((abs_val as u128 * mult as u128) >> shift) as u64;
+ if val < 0 {
+ -(ret as i64)
+ } else {
+ ret as i64
}
}
On 30.04.25 3:51 PM, FUJITA Tomonori wrote:
> On Tue, 29 Apr 2025 16:35:09 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
>>> It appears that there is still no consensus on how to resolve it. CC
>>> the participants in the above thread.
>>>
>>> I think that we can drop this patch and better to focus on Instant and
>>> Delta types in this merge window.
>>>
>>> With the patch below, this issue could be resolved like the C side,
>>> but I'm not sure whether we can reach a consensus quickly.
>>
>> I think using the C ones is fine for the moment, but up to what arm
>> and others think.
>
> I don't think anyone would disagree with the rule Russell mentioned
> that expensive 64-bit by 64-bit division should be avoided unless
> absolutely necessary so we should go with function div_s64() instead
> of function div64_s64().
>
> The DRM QR driver was already fixed by avoiding 64-bit division, so
> for now, the only place where we still need to solve this issue is the
> time abstraction. So, it seems like Arnd's suggestion to simply call
> ktime_to_ms() or ktime_to_us() is the right way to go.
>
>> This one is also a constant, so something simpler may be better (and
>> it is also a power of 10 divisor, so the other suggestions on that
>> thread would apply too).
>
> One downside of calling the C's functions is that the as_micros/millis
> methods can no longer be const fn (or is there a way to make that
> work?).
It would theoretically be possible to use the unstable `const_eval_select`
to use the C implementation at runtume and just divide at compile time.
I don't think that we want to do this (or that it should be done in any
serious project should do this) but it would be technically possible.
I'm also not sure how/if const-eval would handle the 64 by 64 bit division.
> We could implement the method Paolo suggested from Hacker's
> Delight, 2nd edition in Rust and keep using const fn, but I'm not sure
> if it's really worth it.
>
> Any thoughts?
`const fn` would be nice, but if it is not currently needed and would
complicate the implementation we should probably keep it non-const
until someone needs it to be const.
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index a8089a98da9e..daf9e5925e47 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -229,12 +229,116 @@ pub const fn as_nanos(self) -> i64 {
> /// to the value in the [`Delta`].
> #[inline]
> pub const fn as_micros_ceil(self) -> i64 {
> - self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> + const NSEC_PER_USEC_EXP: u32 = 3;
> +
> + div10::<NSEC_PER_USEC_EXP>(self.as_nanos().saturating_add(NSEC_PER_USEC - 1))
> }
>
> /// Return the number of milliseconds in the [`Delta`].
> #[inline]
> pub const fn as_millis(self) -> i64 {
> - self.as_nanos() / NSEC_PER_MSEC
> + const NSEC_PER_MSEC_EXP: u32 = 6;
> +
> + div10::<NSEC_PER_MSEC_EXP>(self.as_nanos())
> + }
> +}
> +
> +/// Precomputed magic constants for division by powers of 10.
> +///
> +/// Each entry corresponds to dividing a number by `10^exp`, where `exp` ranges from 0 to 18.
> +/// These constants were computed using the algorithm from Hacker's Delight, 2nd edition.
> +struct MagicMul {
> + mult: u64,
> + shift: u32,
> +}
> +
> +const DIV10: [MagicMul; 19] = [
> + MagicMul {
> + mult: 0x1,
> + shift: 0,
> + },
> + MagicMul {
> + mult: 0x6666666666666667,
> + shift: 66,
> + },
> + MagicMul {
> + mult: 0xA3D70A3D70A3D70B,
> + shift: 70,
> + },
> + MagicMul {
> + mult: 0x20C49BA5E353F7CF,
> + shift: 71,
> + },
> + MagicMul {
> + mult: 0x346DC5D63886594B,
> + shift: 75,
> + },
> + MagicMul {
> + mult: 0x29F16B11C6D1E109,
> + shift: 78,
> + },
> + MagicMul {
> + mult: 0x431BDE82D7B634DB,
> + shift: 82,
> + },
> + MagicMul {
> + mult: 0xD6BF94D5E57A42BD,
> + shift: 87,
> + },
> + MagicMul {
> + mult: 0x55E63B88C230E77F,
> + shift: 89,
> + },
> + MagicMul {
> + mult: 0x112E0BE826D694B3,
> + shift: 90,
> + },
> + MagicMul {
> + mult: 0x036F9BFB3AF7B757,
> + shift: 91,
> + },
> + MagicMul {
> + mult: 0x00AFEBFF0BCB24AB,
> + shift: 92,
> + },
> + MagicMul {
> + mult: 0x232F33025BD42233,
> + shift: 101,
> + },
> + MagicMul {
> + mult: 0x384B84D092ED0385,
> + shift: 105,
> + },
> + MagicMul {
> + mult: 0x0B424DC35095CD81,
> + shift: 106,
> + },
> + MagicMul {
> + mult: 0x480EBE7B9D58566D,
> + shift: 112,
> + },
> + MagicMul {
> + mult: 0x39A5652FB1137857,
> + shift: 115,
> + },
> + MagicMul {
> + mult: 0x5C3BD5191B525A25,
> + shift: 119,
> + },
> + MagicMul {
> + mult: 0x12725DD1D243ABA1,
> + shift: 120,
> + },
> +];
> +
> +const fn div10<const EXP: u32>(val: i64) -> i64 {
> + crate::build_assert!(EXP <= 18);
> + let MagicMul { mult, shift } = DIV10[EXP as usize];
> + let abs_val = val.wrapping_abs() as u64;
> + let ret = ((abs_val as u128 * mult as u128) >> shift) as u64;
> + if val < 0 {
> + -(ret as i64)
> + } else {
> + ret as i64
> }
> }
On Wed, Apr 30, 2025 at 10:51:31PM +0900, FUJITA Tomonori wrote:
> On Tue, 29 Apr 2025 16:35:09 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> >> It appears that there is still no consensus on how to resolve it. CC
> >> the participants in the above thread.
> >>
> >> I think that we can drop this patch and better to focus on Instant and
> >> Delta types in this merge window.
> >>
> >> With the patch below, this issue could be resolved like the C side,
> >> but I'm not sure whether we can reach a consensus quickly.
> >
> > I think using the C ones is fine for the moment, but up to what arm
> > and others think.
>
> I don't think anyone would disagree with the rule Russell mentioned
> that expensive 64-bit by 64-bit division should be avoided unless
> absolutely necessary so we should go with function div_s64() instead
> of function div64_s64().
>
> The DRM QR driver was already fixed by avoiding 64-bit division, so
> for now, the only place where we still need to solve this issue is the
> time abstraction. So, it seems like Arnd's suggestion to simply call
> ktime_to_ms() or ktime_to_us() is the right way to go.
>
> > This one is also a constant, so something simpler may be better (and
> > it is also a power of 10 divisor, so the other suggestions on that
> > thread would apply too).
>
> One downside of calling the C's functions is that the as_micros/millis
> methods can no longer be const fn (or is there a way to make that
> work?). We could implement the method Paolo suggested from Hacker's
> Delight, 2nd edition in Rust and keep using const fn, but I'm not sure
> if it's really worth it.
>
> Any thoughts?
>
For ARM, where the constant division optimization (into to mult/shift)
is not available, I'm OK with anything you and others come out with
(calling a C function, implementing our own optimization, etc.) For
other architectures where the compilers can do the right thing, I
suggest we use the compiler optimization and don't re-invent the wheel.
For example, in your div10() solution below, we can have a generic
version which just uses a normal division for x86, arm64, riscv, etc,
and an ARM specific version.
Btw, the const fn point is a good one, thanks for bringing that up.
Regards,
Boqun
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index a8089a98da9e..daf9e5925e47 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -229,12 +229,116 @@ pub const fn as_nanos(self) -> i64 {
> /// to the value in the [`Delta`].
> #[inline]
> pub const fn as_micros_ceil(self) -> i64 {
> - self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> + const NSEC_PER_USEC_EXP: u32 = 3;
> +
> + div10::<NSEC_PER_USEC_EXP>(self.as_nanos().saturating_add(NSEC_PER_USEC - 1))
> }
>
> /// Return the number of milliseconds in the [`Delta`].
> #[inline]
> pub const fn as_millis(self) -> i64 {
> - self.as_nanos() / NSEC_PER_MSEC
> + const NSEC_PER_MSEC_EXP: u32 = 6;
> +
> + div10::<NSEC_PER_MSEC_EXP>(self.as_nanos())
> + }
> +}
> +
> +/// Precomputed magic constants for division by powers of 10.
> +///
> +/// Each entry corresponds to dividing a number by `10^exp`, where `exp` ranges from 0 to 18.
> +/// These constants were computed using the algorithm from Hacker's Delight, 2nd edition.
> +struct MagicMul {
> + mult: u64,
> + shift: u32,
> +}
> +
> +const DIV10: [MagicMul; 19] = [
> + MagicMul {
> + mult: 0x1,
> + shift: 0,
> + },
> + MagicMul {
> + mult: 0x6666666666666667,
> + shift: 66,
> + },
> + MagicMul {
> + mult: 0xA3D70A3D70A3D70B,
> + shift: 70,
> + },
> + MagicMul {
> + mult: 0x20C49BA5E353F7CF,
> + shift: 71,
> + },
> + MagicMul {
> + mult: 0x346DC5D63886594B,
> + shift: 75,
> + },
> + MagicMul {
> + mult: 0x29F16B11C6D1E109,
> + shift: 78,
> + },
> + MagicMul {
> + mult: 0x431BDE82D7B634DB,
> + shift: 82,
> + },
> + MagicMul {
> + mult: 0xD6BF94D5E57A42BD,
> + shift: 87,
> + },
> + MagicMul {
> + mult: 0x55E63B88C230E77F,
> + shift: 89,
> + },
> + MagicMul {
> + mult: 0x112E0BE826D694B3,
> + shift: 90,
> + },
> + MagicMul {
> + mult: 0x036F9BFB3AF7B757,
> + shift: 91,
> + },
> + MagicMul {
> + mult: 0x00AFEBFF0BCB24AB,
> + shift: 92,
> + },
> + MagicMul {
> + mult: 0x232F33025BD42233,
> + shift: 101,
> + },
> + MagicMul {
> + mult: 0x384B84D092ED0385,
> + shift: 105,
> + },
> + MagicMul {
> + mult: 0x0B424DC35095CD81,
> + shift: 106,
> + },
> + MagicMul {
> + mult: 0x480EBE7B9D58566D,
> + shift: 112,
> + },
> + MagicMul {
> + mult: 0x39A5652FB1137857,
> + shift: 115,
> + },
> + MagicMul {
> + mult: 0x5C3BD5191B525A25,
> + shift: 119,
> + },
> + MagicMul {
> + mult: 0x12725DD1D243ABA1,
> + shift: 120,
> + },
> +];
> +
> +const fn div10<const EXP: u32>(val: i64) -> i64 {
> + crate::build_assert!(EXP <= 18);
> + let MagicMul { mult, shift } = DIV10[EXP as usize];
> + let abs_val = val.wrapping_abs() as u64;
> + let ret = ((abs_val as u128 * mult as u128) >> shift) as u64;
> + if val < 0 {
> + -(ret as i64)
> + } else {
> + ret as i64
> }
> }
On 29.04.25 3:17 PM, FUJITA Tomonori wrote:
> On Mon, 28 Apr 2025 20:16:47 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> Hi Tomonori,
>>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> 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.
>>
>> I get an error when building this patch for arm32:
>>
>> + kernel-make -j 96 O=/home/aeh/src/linux-rust/test-build-arm-1.78.0 vmlinux modules
>> ld.lld: error: undefined symbol: __aeabi_uldivmod
>> >>> referenced by kernel.df165ca450b1fd1-cgu.0
>> >>> rust/kernel.o:(kernel::time::delay::fsleep) in archive vmlinux.a
>> >>> did you mean: __aeabi_uidivmod
>> >>> defined in: vmlinux.a(arch/arm/lib/lib1funcs.o)
>>
>> Looks like a division function of some sort is not defined. Can you
>> reproduce?
>
> Ah, 64-bit integer division on 32-bit architectures.
>
> I think that the DRM QR driver has the same problem:
>
> https://lore.kernel.org/rust-for-linux/CANiq72ke45eOwckMhWHvmwxc03dxr4rnxxKvx+HvWdBLopZfrQ@mail.gmail.com/
>
> It appears that there is still no consensus on how to resolve it. CC
> the participants in the above thread.
From what I remember from the thread is that generally 64 bit divisions
should be avoided (like the solution for DRM).
> I think that we can drop this patch and better to focus on Instant and
> Delta types in this merge window.
>
> With the patch below, this issue could be resolved like the C side,
> but I'm not sure whether we can reach a consensus quickly.
I think adding rust bindings for this is fine (and most likely needed),
for cases where it is required.
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 48143cdd26b3..c44d45960eb1 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -19,6 +19,7 @@
> #include "io.c"
> #include "jump_label.c"
> #include "kunit.c"
> +#include "math64.c"
> #include "mutex.c"
> #include "page.c"
> #include "platform.c"
> diff --git a/rust/helpers/math64.c b/rust/helpers/math64.c
> new file mode 100644
> index 000000000000..f94708cf8fcb
> --- /dev/null
> +++ b/rust/helpers/math64.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/math64.h>
> +
> +s64 rust_helper_div64_s64(s64 dividend, s64 divisor)
> +{
> + return div64_s64(dividend, divisor);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index de07aadd1ff5..d272e0b0b05d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -60,6 +60,7 @@
> #[cfg(CONFIG_KUNIT)]
> pub mod kunit;
> pub mod list;
> +pub mod math64;
> pub mod miscdevice;
> #[cfg(CONFIG_NET)]
> pub mod net;
> diff --git a/rust/kernel/math64.rs b/rust/kernel/math64.rs
> new file mode 100644
> index 000000000000..523e47911859
> --- /dev/null
> +++ b/rust/kernel/math64.rs
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! 64-bit integer arithmetic helpers.
> +//!
> +//! C header: [`include/linux/math64.h`](srctree/include/linux/math64.h)
> +
> +/// Divide a signed 64-bit integer by another signed 64-bit integer.
> +#[inline]
> +pub fn div64_s64(dividend: i64, divisor: i64) -> i64 {
> + // SAFETY: Calling `div64_s64()` is safe as long as `divisor` is non-zero.
The safety comment is not valid, nowhere is it guaranteed divisor is non-zero.
There's three solutions I can think of:
* Mark this function as `unsafe` and give the responsibility of checking
this to the caller,
* return a `Result` with a division by zero error type or
* change the type of divisor to `NonZeroI64` [0].
Probably the best way is to use `NonZeroI64` since that way
it's statically guaranteed.
In that case it would also make sense to change `NSEC_PER_USEC` to be `NonZeroI64`.
Link: https://doc.rust-lang.org/nightly/core/num/type.NonZeroI64.html [0]
> + unsafe { bindings::div64_s64(dividend, divisor) }
Is `s64` just a typedef for `int64_t` and if so this true for every
architecture? (I don't know the C side very well).
If not there might need to be some kind of conversion to make sure
they are passed correctly.
> +}
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 863385905029..7b5255893929 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -24,6 +24,8 @@
> //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>
> +use crate::math64;
> +
> pub mod delay;
> pub mod hrtimer;
>
> @@ -229,13 +231,16 @@ pub const fn as_nanos(self) -> i64 {
> /// Return the smallest number of microseconds greater than or equal
> /// to the value in the [`Delta`].
> #[inline]
> - pub const fn as_micros_ceil(self) -> i64 {
> - self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> + pub fn as_micros_ceil(self) -> i64 {
> + math64::div64_s64(
It would make sense to change `NSEC_PER_USEC` to be `NonZeroI64`.
> + self.as_nanos().saturating_add(NSEC_PER_USEC - 1),
> + NSEC_PER_USEC,
> + )
> }
>
> /// Return the number of milliseconds in the [`Delta`].
> #[inline]
> - pub const fn as_millis(self) -> i64 {
> - self.as_nanos() / NSEC_PER_MSEC
> + pub fn as_millis(self) -> i64 {
> + math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
> }
> }
>
> base-commit: da37ddd3f607897d039d82e6621671c3f7baa886
Cheers
Christian
On Tue, Apr 29, 2025 at 04:16:01PM +0200, Christian Schrefl wrote:
> On 29.04.25 3:17 PM, FUJITA Tomonori wrote:
> > On Mon, 28 Apr 2025 20:16:47 +0200
> > Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >
> >> Hi Tomonori,
> >>
> >> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> >>
> >>> 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.
> >>
> >> I get an error when building this patch for arm32:
> >>
> >> + kernel-make -j 96 O=/home/aeh/src/linux-rust/test-build-arm-1.78.0 vmlinux modules
> >> ld.lld: error: undefined symbol: __aeabi_uldivmod
> >> >>> referenced by kernel.df165ca450b1fd1-cgu.0
> >> >>> rust/kernel.o:(kernel::time::delay::fsleep) in archive vmlinux.a
> >> >>> did you mean: __aeabi_uidivmod
> >> >>> defined in: vmlinux.a(arch/arm/lib/lib1funcs.o)
> >>
> >> Looks like a division function of some sort is not defined. Can you
> >> reproduce?
> >
> > Ah, 64-bit integer division on 32-bit architectures.
> >
> > I think that the DRM QR driver has the same problem:
> >
> > https://lore.kernel.org/rust-for-linux/CANiq72ke45eOwckMhWHvmwxc03dxr4rnxxKvx+HvWdBLopZfrQ@mail.gmail.com/
> >
> > It appears that there is still no consensus on how to resolve it. CC
> > the participants in the above thread.
>
> From what I remember from the thread is that generally 64 bit divisions
> should be avoided (like the solution for DRM).
>
> > I think that we can drop this patch and better to focus on Instant and
> > Delta types in this merge window.
> >
> > With the patch below, this issue could be resolved like the C side,
> > but I'm not sure whether we can reach a consensus quickly.
>
> I think adding rust bindings for this is fine (and most likely needed),
> for cases where it is required.
>
> >
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 48143cdd26b3..c44d45960eb1 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -19,6 +19,7 @@
> > #include "io.c"
> > #include "jump_label.c"
> > #include "kunit.c"
> > +#include "math64.c"
> > #include "mutex.c"
> > #include "page.c"
> > #include "platform.c"
> > diff --git a/rust/helpers/math64.c b/rust/helpers/math64.c
> > new file mode 100644
> > index 000000000000..f94708cf8fcb
> > --- /dev/null
> > +++ b/rust/helpers/math64.c
> > @@ -0,0 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/math64.h>
> > +
> > +s64 rust_helper_div64_s64(s64 dividend, s64 divisor)
> > +{
> > + return div64_s64(dividend, divisor);
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index de07aadd1ff5..d272e0b0b05d 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -60,6 +60,7 @@
> > #[cfg(CONFIG_KUNIT)]
> > pub mod kunit;
> > pub mod list;
> > +pub mod math64;
> > pub mod miscdevice;
> > #[cfg(CONFIG_NET)]
> > pub mod net;
> > diff --git a/rust/kernel/math64.rs b/rust/kernel/math64.rs
> > new file mode 100644
> > index 000000000000..523e47911859
> > --- /dev/null
> > +++ b/rust/kernel/math64.rs
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! 64-bit integer arithmetic helpers.
> > +//!
> > +//! C header: [`include/linux/math64.h`](srctree/include/linux/math64.h)
> > +
> > +/// Divide a signed 64-bit integer by another signed 64-bit integer.
> > +#[inline]
> > +pub fn div64_s64(dividend: i64, divisor: i64) -> i64 {
> > + // SAFETY: Calling `div64_s64()` is safe as long as `divisor` is non-zero.
> The safety comment is not valid, nowhere is it guaranteed divisor is non-zero.
>
> There's three solutions I can think of:
> * Mark this function as `unsafe` and give the responsibility of checking
> this to the caller,
> * return a `Result` with a division by zero error type or
> * change the type of divisor to `NonZeroI64` [0].
>
> Probably the best way is to use `NonZeroI64` since that way
> it's statically guaranteed.
>
> In that case it would also make sense to change `NSEC_PER_USEC` to be `NonZeroI64`.
>
>
> Link: https://doc.rust-lang.org/nightly/core/num/type.NonZeroI64.html [0]
> > + unsafe { bindings::div64_s64(dividend, divisor) }
>
> Is `s64` just a typedef for `int64_t` and if so this true for every
> architecture? (I don't know the C side very well).
>
> If not there might need to be some kind of conversion to make sure
> they are passed correctly.
>
> > +}
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index 863385905029..7b5255893929 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -24,6 +24,8 @@
> > //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
> > //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
> >
> > +use crate::math64;
> > +
> > pub mod delay;
> > pub mod hrtimer;
> >
> > @@ -229,13 +231,16 @@ pub const fn as_nanos(self) -> i64 {
> > /// Return the smallest number of microseconds greater than or equal
> > /// to the value in the [`Delta`].
> > #[inline]
> > - pub const fn as_micros_ceil(self) -> i64 {
> > - self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> > + pub fn as_micros_ceil(self) -> i64 {
> > + math64::div64_s64(
> It would make sense to change `NSEC_PER_USEC` to be `NonZeroI64`.
>
> > + self.as_nanos().saturating_add(NSEC_PER_USEC - 1),
> > + NSEC_PER_USEC,
> > + )
> > }
> >
> > /// Return the number of milliseconds in the [`Delta`].
> > #[inline]
> > - pub const fn as_millis(self) -> i64 {
> > - self.as_nanos() / NSEC_PER_MSEC
> > + pub fn as_millis(self) -> i64 {
> > + math64::div64_s64(self.as_nanos(), NSEC_PER_MSEC)
There is no way this should ever be an expensive 64-bit by 64-bit
division. Think about value of the divisor here - there's 1000us
in 1ms, and 1000ns in 1us, so this has the value of 1000000,
which is 20 bits.
So for a 32-bit architecture, trying to do a 64-bit by 64-bit
division is expensive, and 32-bit architectures don't implement
this as a general rule because of this - most times you do not
have a 64-bit divisor, but something much smaller, making a
64-bit by 32-bit division more suitable. That is a lot faster on
32-bit architectures.
So, I think more thought is needed to be put into this by Rust
folk, rather than blindly forcing everything to be 64-bit by
64-bit even when the divisor is 20-bit.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
© 2016 - 2025 Red Hat, Inc.