Message-ID: <aBDi06bQibCIB4En@shell.armlinux.org.uk>
Date: Tue, 29 Apr 2025 15:31:47 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Christian Schrefl <chrisi.schrefl@...il.com>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, a.hindborg@...nel.org,
	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, linus.walleij@...aro.org
Subject: Re: [PATCH v15 5/6] rust: time: Add wrapper for fsleep() function

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@...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)

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!

Powered by blists - more mailing lists