Message-ID: <03ccb65b-a5f8-4afc-84f5-e46f1caf96b0@gmail.com>
Date: Tue, 29 Apr 2025 16:16:01 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, a.hindborg@...nel.org
Cc: rust-for-linux@...r.kernel.org, gary@...yguo.net, aliceryhl@...gle.com,
 me@...enk.dev, daniel.almeida@...labora.com, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com,
 tmgross@...ch.edu, ojeda@...nel.org, alex.gaynor@...il.com,
 bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, a.hindborg@...sung.com,
 anna-maria@...utronix.de, frederic@...nel.org, tglx@...utronix.de,
 arnd@...db.de, jstultz@...gle.com, sboyd@...nel.org, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, vschneid@...hat.com, tgunders@...hat.com,
 david.laight.linux@...il.com, boqun.feng@...il.com, pbonzini@...hat.com,
 jfalempe@...hat.com, linux@...linux.org.uk, linus.walleij@...aro.org
Subject: Re: [PATCH v15 5/6] rust: time: Add wrapper for fsleep() function

On 29.04.25 3:17 PM, FUJITA Tomonori wrote:
> On Mon, 28 Apr 2025 20:16:47 +0200
> Andreas Hindborg <a.hindborg@...nel.org> wrote:
> 
>> Hi Tomonori,
>>
>> "FUJITA Tomonori" <fujita.tomonori@...il.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

Powered by blists - more mailing lists