[PATCH v1] rust: time: Avoid 64-bit integer division

[PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

Avoid 64-bit integer division that 32-bit architectures don't
implement generally. This uses ktime_to_ms() and ktime_to_us()
instead.

The timer abstraction needs i64 / u32 division so C's div_s64() can be
used but ktime_to_ms() and ktime_to_us() provide a simpler solution
for this timer abstraction problem. On some architectures, there is
room to optimize the implementation of them, but such optimization can
be done if and when it becomes necessary.

One downside of calling the C's functions is that the as_micros/millis
methods can no longer be const fn. We stick with the simpler approach
unless there's a compelling need for a const fn.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c |  1 +
 rust/helpers/time.c    | 13 +++++++++++++
 rust/kernel/time.rs    | 10 ++++++----
 3 files changed, 20 insertions(+), 4 deletions(-)
 create mode 100644 rust/helpers/time.c

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 1e7c84df7252..2ac088de050f 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -34,6 +34,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..0a5d1773a07c
--- /dev/null
+++ b/rust/helpers/time.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ktime.h>
+
+s64 rust_helper_ktime_to_us(const ktime_t kt)
+{
+	return ktime_divns(kt, NSEC_PER_USEC);
+}
+
+s64 rust_helper_ktime_to_ms(const ktime_t kt)
+{
+	return ktime_divns(kt, NSEC_PER_MSEC);
+}
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index a8089a98da9e..e3008f6324ea 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -228,13 +228,15 @@ 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 {
+        // SAFETY: It is always safe to call `ktime_to_us()` with any value.
+        unsafe { bindings::ktime_to_us(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
+    pub fn as_millis(self) -> i64 {
+        // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
+        unsafe { bindings::ktime_to_ms(self.nanos) }
     }
 }

base-commit: 679185904972421c570a1c337a8266835045012d
-- 
2.43.0

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Thu,  1 May 2025 10:58:18 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Avoid 64-bit integer division that 32-bit architectures don't
> implement generally. This uses ktime_to_ms() and ktime_to_us()
> instead.
> 
> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> for this timer abstraction problem. On some architectures, there is
> room to optimize the implementation of them, but such optimization can
> be done if and when it becomes necessary.
> 
> One downside of calling the C's functions is that the as_micros/millis
> methods can no longer be const fn. We stick with the simpler approach
> unless there's a compelling need for a const fn.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c |  1 +
>  rust/helpers/time.c    | 13 +++++++++++++
>  rust/kernel/time.rs    | 10 ++++++----
>  3 files changed, 20 insertions(+), 4 deletions(-)
>  create mode 100644 rust/helpers/time.c
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 1e7c84df7252..2ac088de050f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -34,6 +34,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..0a5d1773a07c
> --- /dev/null
> +++ b/rust/helpers/time.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/ktime.h>
> +
> +s64 rust_helper_ktime_to_us(const ktime_t kt)
> +{
> +	return ktime_divns(kt, NSEC_PER_USEC);
> +}
> +
> +s64 rust_helper_ktime_to_ms(const ktime_t kt)
> +{
> +	return ktime_divns(kt, NSEC_PER_MSEC);
> +}

Oops, they should call ktime_to_us() and ktime_to_ms() respectively.

I'll send v2 later.

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Miguel Ojeda]

On Thu, May 1, 2025 at 4:45 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Oops, they should call ktime_to_us() and ktime_to_ms() respectively.
>
> I'll send v2 later.

What about adding a couple examples to their docs so that they are
also tested and this would be caught?

(In another patch, possibly in another series or not. We could also
open a good first issue)

Thanks!

Cheers,
Miguel

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Thu, 1 May 2025 11:24:24 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, May 1, 2025 at 4:45 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Oops, they should call ktime_to_us() and ktime_to_ms() respectively.
>>
>> I'll send v2 later.
> 
> What about adding a couple examples to their docs so that they are
> also tested and this would be caught?
> 
> (In another patch, possibly in another series or not. We could also
> open a good first issue)

I'll submit another patch to add some doctests once this is merged.

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Christian Schrefl]

On 01.05.25 3:58 AM, FUJITA Tomonori wrote:
> Avoid 64-bit integer division that 32-bit architectures don't
> implement generally. This uses ktime_to_ms() and ktime_to_us()
> instead.
> 
> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> for this timer abstraction problem. On some architectures, there is
> room to optimize the implementation of them, but such optimization can
> be done if and when it becomes necessary.
> 
> One downside of calling the C's functions is that the as_micros/millis
> methods can no longer be const fn. We stick with the simpler approach
> unless there's a compelling need for a const fn.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---

With the helpers fixed:

Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>

>  rust/helpers/helpers.c |  1 +
>  rust/helpers/time.c    | 13 +++++++++++++
>  rust/kernel/time.rs    | 10 ++++++----
>  3 files changed, 20 insertions(+), 4 deletions(-)
>  create mode 100644 rust/helpers/time.c
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 1e7c84df7252..2ac088de050f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -34,6 +34,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..0a5d1773a07c
> --- /dev/null
> +++ b/rust/helpers/time.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/ktime.h>
> +
> +s64 rust_helper_ktime_to_us(const ktime_t kt)
> +{
> +	return ktime_divns(kt, NSEC_PER_USEC);
> +}
> +
> +s64 rust_helper_ktime_to_ms(const ktime_t kt)
> +{
> +	return ktime_divns(kt, NSEC_PER_MSEC);
> +}
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index a8089a98da9e..e3008f6324ea 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -228,13 +228,15 @@ 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 {
> +        // SAFETY: It is always safe to call `ktime_to_us()` with any value.
> +        unsafe { bindings::ktime_to_us(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
> +    pub fn as_millis(self) -> i64 {
> +        // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
> +        unsafe { bindings::ktime_to_ms(self.nanos) }
>      }
>  }
> 
> base-commit: 679185904972421c570a1c337a8266835045012d

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Miguel Ojeda]

On Thu, May 1, 2025 at 3:58 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +s64 rust_helper_ktime_to_us(const ktime_t kt)
> +{
> +       return ktime_divns(kt, NSEC_PER_USEC);
> +}
> +
> +s64 rust_helper_ktime_to_ms(const ktime_t kt)
> +{
> +       return ktime_divns(kt, NSEC_PER_MSEC);
> +}

In general, we want to keep helpers mapping to the functions as close
as possible to the C side (e.g. same arguments), unless there is a
good reason not to -- but since you are changing it, let's review
v2...

Cheers,
Miguel

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
> Avoid 64-bit integer division that 32-bit architectures don't
> implement generally. This uses ktime_to_ms() and ktime_to_us()
> instead.
> 
> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> for this timer abstraction problem. On some architectures, there is
> room to optimize the implementation of them, but such optimization can
> be done if and when it becomes necessary.
> 

Nacked-by: Boqun Feng <boqun.feng@gmail.com>

As I said a few times, we should rely on compiler's optimization when
available, i.e. it's a problem that ARM compiler doesn't have this
optimization, don't punish other architecture of no reason.

Regards,
Boqun

> One downside of calling the C's functions is that the as_micros/millis
> methods can no longer be const fn. We stick with the simpler approach
> unless there's a compelling need for a const fn.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c |  1 +
>  rust/helpers/time.c    | 13 +++++++++++++
>  rust/kernel/time.rs    | 10 ++++++----
>  3 files changed, 20 insertions(+), 4 deletions(-)
>  create mode 100644 rust/helpers/time.c
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 1e7c84df7252..2ac088de050f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -34,6 +34,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..0a5d1773a07c
> --- /dev/null
> +++ b/rust/helpers/time.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/ktime.h>
> +
> +s64 rust_helper_ktime_to_us(const ktime_t kt)
> +{
> +	return ktime_divns(kt, NSEC_PER_USEC);
> +}
> +
> +s64 rust_helper_ktime_to_ms(const ktime_t kt)
> +{
> +	return ktime_divns(kt, NSEC_PER_MSEC);
> +}
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index a8089a98da9e..e3008f6324ea 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -228,13 +228,15 @@ 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 {
> +        // SAFETY: It is always safe to call `ktime_to_us()` with any value.
> +        unsafe { bindings::ktime_to_us(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
> +    pub fn as_millis(self) -> i64 {
> +        // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
> +        unsafe { bindings::ktime_to_ms(self.nanos) }
>      }
>  }
> 
> base-commit: 679185904972421c570a1c337a8266835045012d
> -- 
> 2.43.0
> 
> 

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 05:26:54AM -0700, Boqun Feng wrote:
> On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
> > Avoid 64-bit integer division that 32-bit architectures don't
> > implement generally. This uses ktime_to_ms() and ktime_to_us()
> > instead.
> > 
> > The timer abstraction needs i64 / u32 division so C's div_s64() can be
> > used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> > for this timer abstraction problem. On some architectures, there is
> > room to optimize the implementation of them, but such optimization can
> > be done if and when it becomes necessary.
> > 
> 
> Nacked-by: Boqun Feng <boqun.feng@gmail.com>
> 
> As I said a few times, we should rely on compiler's optimization when
> available, i.e. it's a problem that ARM compiler doesn't have this
> optimization, don't punish other architecture of no reason.
> 
> Regards,
> Boqun
> 
> > One downside of calling the C's functions is that the as_micros/millis
> > methods can no longer be const fn. We stick with the simpler approach
> > unless there's a compelling need for a const fn.
> > 

Btw, I think you're missing the Suggested-by tag from Arnd, of course if
Arnd is not against it.

Regards,
Boqun

> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > ---
> >  rust/helpers/helpers.c |  1 +
> >  rust/helpers/time.c    | 13 +++++++++++++
> >  rust/kernel/time.rs    | 10 ++++++----
> >  3 files changed, 20 insertions(+), 4 deletions(-)
> >  create mode 100644 rust/helpers/time.c
> > 
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 1e7c84df7252..2ac088de050f 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -34,6 +34,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..0a5d1773a07c
> > --- /dev/null
> > +++ b/rust/helpers/time.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/ktime.h>
> > +
> > +s64 rust_helper_ktime_to_us(const ktime_t kt)
> > +{
> > +	return ktime_divns(kt, NSEC_PER_USEC);
> > +}
> > +
> > +s64 rust_helper_ktime_to_ms(const ktime_t kt)
> > +{
> > +	return ktime_divns(kt, NSEC_PER_MSEC);
> > +}
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index a8089a98da9e..e3008f6324ea 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -228,13 +228,15 @@ 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 {
> > +        // SAFETY: It is always safe to call `ktime_to_us()` with any value.
> > +        unsafe { bindings::ktime_to_us(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
> > +    pub fn as_millis(self) -> i64 {
> > +        // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
> > +        unsafe { bindings::ktime_to_ms(self.nanos) }
> >      }
> >  }
> > 
> > base-commit: 679185904972421c570a1c337a8266835045012d
> > -- 
> > 2.43.0
> > 
> > 
> 

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Thu, 1 May 2025 05:37:58 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> Btw, I think you're missing the Suggested-by tag from Arnd, of course if
> Arnd is not against it.

I'll submit v2 tomorrow, including Suggested-by tag from you and Arnd.

Please let me know if you're not okay with being included in the tag.

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 10:48:15PM +0900, FUJITA Tomonori wrote:
> On Thu, 1 May 2025 05:37:58 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > Btw, I think you're missing the Suggested-by tag from Arnd, of course if
> > Arnd is not against it.
> 
> I'll submit v2 tomorrow, including Suggested-by tag from you and Arnd.
> 
> Please let me know if you're not okay with being included in the tag.
> 

Sounds good! Thanks for the hard work! ;-)

Regards,
Boqun

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Thu, 1 May 2025 05:26:54 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
>> Avoid 64-bit integer division that 32-bit architectures don't
>> implement generally. This uses ktime_to_ms() and ktime_to_us()
>> instead.
>> 
>> The timer abstraction needs i64 / u32 division so C's div_s64() can be
>> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
>> for this timer abstraction problem. On some architectures, there is
>> room to optimize the implementation of them, but such optimization can
>> be done if and when it becomes necessary.
>> 
> 
> Nacked-by: Boqun Feng <boqun.feng@gmail.com>
> 
> As I said a few times, we should rely on compiler's optimization when
> available, i.e. it's a problem that ARM compiler doesn't have this
> optimization, don't punish other architecture of no reason.

Did you mean that we should do something like the following?

pub fn as_millis(self) -> i64 {
    #[cfg(CONFIG_ARM)]
    {
        // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
        unsafe { bindings::ktime_to_ms(self.nanos) }
    }
    #[cfg(not(CONFIG_ARM))]
    {
        self.as_nanos() / NSEC_PER_MSEC
    }
}

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 10:07:17PM +0900, FUJITA Tomonori wrote:
> On Thu, 1 May 2025 05:26:54 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
> >> Avoid 64-bit integer division that 32-bit architectures don't
> >> implement generally. This uses ktime_to_ms() and ktime_to_us()
> >> instead.
> >> 
> >> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> >> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> >> for this timer abstraction problem. On some architectures, there is
> >> room to optimize the implementation of them, but such optimization can
> >> be done if and when it becomes necessary.
> >> 
> > 
> > Nacked-by: Boqun Feng <boqun.feng@gmail.com>
> > 
> > As I said a few times, we should rely on compiler's optimization when
> > available, i.e. it's a problem that ARM compiler doesn't have this
> > optimization, don't punish other architecture of no reason.
> 
> Did you mean that we should do something like the following?
> 

Yes, or

	#[cfg(CONFIG_ARM)]
	fn ns_to_ms(ns: i64) -> i64 {
	    // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
	    unsafe { bindings::ktime_to_ms(self.nanos) }
	}

	#[cfg(not(CONFIG_ARM))]
	fn ns_to_ms(ns: i64) -> i64 {
	    self.as_nanos() / NSEC_PER_MSEC
	}

	pub fn as_millis(self) -> i64 {
	    ns_to_ms(self.as_nanos())
	}

Regards,
Boqun

> pub fn as_millis(self) -> i64 {
>     #[cfg(CONFIG_ARM)]
>     {
>         // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
>         unsafe { bindings::ktime_to_ms(self.nanos) }
>     }
>     #[cfg(not(CONFIG_ARM))]
>     {
>         self.as_nanos() / NSEC_PER_MSEC
>     }
> }
> 

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 06:12:02AM -0700, Boqun Feng wrote:
> On Thu, May 01, 2025 at 10:07:17PM +0900, FUJITA Tomonori wrote:
> > On Thu, 1 May 2025 05:26:54 -0700
> > Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> > > On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
> > >> Avoid 64-bit integer division that 32-bit architectures don't
> > >> implement generally. This uses ktime_to_ms() and ktime_to_us()
> > >> instead.
> > >> 
> > >> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> > >> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> > >> for this timer abstraction problem. On some architectures, there is
> > >> room to optimize the implementation of them, but such optimization can
> > >> be done if and when it becomes necessary.
> > >> 
> > > 
> > > Nacked-by: Boqun Feng <boqun.feng@gmail.com>
> > > 
> > > As I said a few times, we should rely on compiler's optimization when
> > > available, i.e. it's a problem that ARM compiler doesn't have this
> > > optimization, don't punish other architecture of no reason.
> > 
> > Did you mean that we should do something like the following?
> > 
> 
> Yes, or
> 
> 	#[cfg(CONFIG_ARM)]
> 	fn ns_to_ms(ns: i64) -> i64 {
> 	    // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
> 	    unsafe { bindings::ktime_to_ms(self.nanos) }

Copy-paste errors:

	    unsafe { bindings::ktime_to_ms(ns) }

> 	}
> 
> 	#[cfg(not(CONFIG_ARM))]
> 	fn ns_to_ms(ns: i64) -> i64 {
> 	    self.as_nanos() / NSEC_PER_MSEC

	    ns / NSEC_PER_MSEC

;-)

Regards,
Boqun

> 	}
> 
> 	pub fn as_millis(self) -> i64 {
> 	    ns_to_ms(self.as_nanos())
> 	}
> 
> Regards,
> Boqun
> 
> > pub fn as_millis(self) -> i64 {
> >     #[cfg(CONFIG_ARM)]
> >     {
> >         // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
> >         unsafe { bindings::ktime_to_ms(self.nanos) }
> >     }
> >     #[cfg(not(CONFIG_ARM))]
> >     {
> >         self.as_nanos() / NSEC_PER_MSEC
> >     }
> > }
> > 
> 

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Arnd Bergmann]

On Thu, May 1, 2025, at 15:20, Boqun Feng wrote:
> On Thu, May 01, 2025 at 06:12:02AM -0700, Boqun Feng wrote:
>> On Thu, May 01, 2025 at 10:07:17PM +0900, FUJITA Tomonori wrote:
>> > On Thu, 1 May 2025 05:26:54 -0700
>> > Boqun Feng <boqun.feng@gmail.com> wrote:
>> > 
>> > > On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
>> > >> Avoid 64-bit integer division that 32-bit architectures don't
>> > >> implement generally. This uses ktime_to_ms() and ktime_to_us()
>> > >> instead.
>> > >> 
>> > >> The timer abstraction needs i64 / u32 division so C's div_s64() can be
>> > >> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
>> > >> for this timer abstraction problem. On some architectures, there is
>> > >> room to optimize the implementation of them, but such optimization can
>> > >> be done if and when it becomes necessary.
>> > >> 
>> > > 
>> > > Nacked-by: Boqun Feng <boqun.feng@gmail.com>
>> > > 
>> > > As I said a few times, we should rely on compiler's optimization when
>> > > available, i.e. it's a problem that ARM compiler doesn't have this
>> > > optimization, don't punish other architecture of no reason.

What is Arm specific here? I'm not aware of the compiler doing anything
different from the other 32-bit architectures, though most are missing
an optimized __arch_xprod_64() and fall back to slightly worse code
from the asm-generic version.

> Copy-paste errors:
>
> 	    unsafe { bindings::ktime_to_ms(ns) }
>
>> 	}
>> 
>> 	#[cfg(not(CONFIG_ARM))]
>> 	fn ns_to_ms(ns: i64) -> i64 {
>> 	    self.as_nanos() / NSEC_PER_MSEC
>
> 	    ns / NSEC_PER_MSEC

I'm sure this is still broken on all 32-bit targets.

     Arnd

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 05:11:44PM +0200, Arnd Bergmann wrote:
> On Thu, May 1, 2025, at 15:20, Boqun Feng wrote:
> > On Thu, May 01, 2025 at 06:12:02AM -0700, Boqun Feng wrote:
> >> On Thu, May 01, 2025 at 10:07:17PM +0900, FUJITA Tomonori wrote:
> >> > On Thu, 1 May 2025 05:26:54 -0700
> >> > Boqun Feng <boqun.feng@gmail.com> wrote:
> >> > 
> >> > > On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
> >> > >> Avoid 64-bit integer division that 32-bit architectures don't
> >> > >> implement generally. This uses ktime_to_ms() and ktime_to_us()
> >> > >> instead.
> >> > >> 
> >> > >> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> >> > >> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> >> > >> for this timer abstraction problem. On some architectures, there is
> >> > >> room to optimize the implementation of them, but such optimization can
> >> > >> be done if and when it becomes necessary.
> >> > >> 
> >> > > 
> >> > > Nacked-by: Boqun Feng <boqun.feng@gmail.com>
> >> > > 
> >> > > As I said a few times, we should rely on compiler's optimization when
> >> > > available, i.e. it's a problem that ARM compiler doesn't have this
> >> > > optimization, don't punish other architecture of no reason.
> 
> What is Arm specific here? I'm not aware of the compiler doing anything

Because Arm is the only 32bit architecture that selects CONFIG_HAVE_RUST
for non-UML cases, i.e. this is the only 32bit architecture that has
this problem. If your point is we should do this for all 32bit
architectures, then I won't disagree. Just s/CONFIG_ARM/CONFIG_32BIT
then.

Regards,
Boqun

> different from the other 32-bit architectures, though most are missing
> an optimized __arch_xprod_64() and fall back to slightly worse code
> from the asm-generic version.
> 
> > Copy-paste errors:
> >
> > 	    unsafe { bindings::ktime_to_ms(ns) }
> >
> >> 	}
> >> 
> >> 	#[cfg(not(CONFIG_ARM))]
> >> 	fn ns_to_ms(ns: i64) -> i64 {
> >> 	    self.as_nanos() / NSEC_PER_MSEC
> >
> > 	    ns / NSEC_PER_MSEC
> 
> I'm sure this is still broken on all 32-bit targets.
> 
>      Arnd

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Christian Schrefl]

On 02.05.25 1:03 AM, Boqun Feng wrote:
> On Thu, May 01, 2025 at 05:11:44PM +0200, Arnd Bergmann wrote:
>> On Thu, May 1, 2025, at 15:20, Boqun Feng wrote:
>>> On Thu, May 01, 2025 at 06:12:02AM -0700, Boqun Feng wrote:
>>>> On Thu, May 01, 2025 at 10:07:17PM +0900, FUJITA Tomonori wrote:
>>>>> On Thu, 1 May 2025 05:26:54 -0700
>>>>> Boqun Feng <boqun.feng@gmail.com> wrote:
>>>>>
>>>>>> On Thu, May 01, 2025 at 10:58:18AM +0900, FUJITA Tomonori wrote:
>>>>>>> Avoid 64-bit integer division that 32-bit architectures don't
>>>>>>> implement generally. This uses ktime_to_ms() and ktime_to_us()
>>>>>>> instead.
>>>>>>>
>>>>>>> The timer abstraction needs i64 / u32 division so C's div_s64() can be
>>>>>>> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
>>>>>>> for this timer abstraction problem. On some architectures, there is
>>>>>>> room to optimize the implementation of them, but such optimization can
>>>>>>> be done if and when it becomes necessary.
>>>>>>>
>>>>>>
>>>>>> Nacked-by: Boqun Feng <boqun.feng@gmail.com>
>>>>>>
>>>>>> As I said a few times, we should rely on compiler's optimization when
>>>>>> available, i.e. it's a problem that ARM compiler doesn't have this
>>>>>> optimization, don't punish other architecture of no reason.
>>
>> What is Arm specific here? I'm not aware of the compiler doing anything
> 
> Because Arm is the only 32bit architecture that selects CONFIG_HAVE_RUST
> for non-UML cases, i.e. this is the only 32bit architecture that has
> this problem. If your point is we should do this for all 32bit
> architectures, then I won't disagree. Just s/CONFIG_ARM/CONFIG_32BIT
> then.

I would be for using `CONFIG_32BIT` since from what I understand this
applies to all 32bit architectures. It feels a bit weird to single out
arm just because it is the only one that currently has rust support.

Cheers
Christian

> 
> Regards,
> Boqun
> 
>> different from the other 32-bit architectures, though most are missing
>> an optimized __arch_xprod_64() and fall back to slightly worse code
>> from the asm-generic version.
>>
>>> Copy-paste errors:
>>>
>>> 	    unsafe { bindings::ktime_to_ms(ns) }
>>>
>>>> 	}
>>>>
>>>> 	#[cfg(not(CONFIG_ARM))]
>>>> 	fn ns_to_ms(ns: i64) -> i64 {
>>>> 	    self.as_nanos() / NSEC_PER_MSEC
>>>
>>> 	    ns / NSEC_PER_MSEC
>>
>> I'm sure this is still broken on all 32-bit targets.
>>
>>      Arnd

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Fri, 2 May 2025 01:38:43 +0200
Christian Schrefl <chrisi.schrefl@gmail.com> wrote:

>>> What is Arm specific here? I'm not aware of the compiler doing anything
>> 
>> Because Arm is the only 32bit architecture that selects CONFIG_HAVE_RUST
>> for non-UML cases, i.e. this is the only 32bit architecture that has
>> this problem. If your point is we should do this for all 32bit
>> architectures, then I won't disagree. Just s/CONFIG_ARM/CONFIG_32BIT
>> then.
> 
> I would be for using `CONFIG_32BIT` since from what I understand this
> applies to all 32bit architectures. It feels a bit weird to single out
> arm just because it is the only one that currently has rust support.

CONFIG_32BIT doesn't work because 32-bit ARM doesn't set it. I use
CONFIG_64BIT, enabled on all 64-bit architectures supported by Rust.

https://lore.kernel.org/lkml/20250502004524.230553-1-fujita.tomonori@gmail.com/

Unlike CONFIG_ARM, the intention behind CONFIG_64BIT is clear, so I
also think it's the better choice.

Thanks!

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Miguel Ojeda]

On Thu, May 1, 2025 at 3:12 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
>         #[cfg(CONFIG_ARM)]
>         fn ns_to_ms(ns: i64) -> i64 {
>
>         #[cfg(not(CONFIG_ARM))]
>         fn ns_to_ms(ns: i64) -> i64 {

I think `cfg`s may be better inside, i.e. as local as reasonably
possible, so that we share e.g. signature as well as any attributes
and docs.

Cheers,
Miguel

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Boqun Feng]

On Thu, May 01, 2025 at 03:22:00PM +0200, Miguel Ojeda wrote:
> On Thu, May 1, 2025 at 3:12 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> >         #[cfg(CONFIG_ARM)]
> >         fn ns_to_ms(ns: i64) -> i64 {
> >
> >         #[cfg(not(CONFIG_ARM))]
> >         fn ns_to_ms(ns: i64) -> i64 {
> 
> I think `cfg`s may be better inside, i.e. as local as reasonably
> possible, so that we share e.g. signature as well as any attributes
> and docs.
> 

Fair enough.

Regards,
Boqun

> Cheers,
> Miguel
> 

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Thu, 1 May 2025 06:25:04 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Thu, May 01, 2025 at 03:22:00PM +0200, Miguel Ojeda wrote:
>> On Thu, May 1, 2025 at 3:12 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>> >
>> >         #[cfg(CONFIG_ARM)]
>> >         fn ns_to_ms(ns: i64) -> i64 {
>> >
>> >         #[cfg(not(CONFIG_ARM))]
>> >         fn ns_to_ms(ns: i64) -> i64 {
>> 
>> I think `cfg`s may be better inside, i.e. as local as reasonably
>> possible, so that we share e.g. signature as well as any attributes
>> and docs.
>> 
> 
> Fair enough.

I'll go with the following.

#[inline]
pub fn as_millis(self) -> i64 {
    #[cfg(CONFIG_ARM)]
    // SAFETY: It is always safe to call `ktime_to_ms()` with any value.
    unsafe {
        bindings::ktime_to_ms(self.as_nanos())
    }

    #[cfg(not(CONFIG_ARM))]
    {
        self.as_nanos() / NSEC_PER_MSEC
    }
}

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Andreas Hindborg]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> Avoid 64-bit integer division that 32-bit architectures don't
> implement generally. This uses ktime_to_ms() and ktime_to_us()
> instead.
>
> The timer abstraction needs i64 / u32 division so C's div_s64() can be
> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
> for this timer abstraction problem. On some architectures, there is
> room to optimize the implementation of them, but such optimization can
> be done if and when it becomes necessary.
>
> One downside of calling the C's functions is that the as_micros/millis
> methods can no longer be const fn. We stick with the simpler approach
> unless there's a compelling need for a const fn.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>


Please consult recent MAINTAINERS file when you send patches. If you
intend for me to see a patch, please use my registered email address.


Best regards,
Andreas Hindborg

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[FUJITA Tomonori]

On Mon, 05 May 2025 12:46:15 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> 
>> Avoid 64-bit integer division that 32-bit architectures don't
>> implement generally. This uses ktime_to_ms() and ktime_to_us()
>> instead.
>>
>> The timer abstraction needs i64 / u32 division so C's div_s64() can be
>> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
>> for this timer abstraction problem. On some architectures, there is
>> room to optimize the implementation of them, but such optimization can
>> be done if and when it becomes necessary.
>>
>> One downside of calling the C's functions is that the as_micros/millis
>> methods can no longer be const fn. We stick with the simpler approach
>> unless there's a compelling need for a const fn.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> 
> Please consult recent MAINTAINERS file when you send patches. If you
> intend for me to see a patch, please use my registered email address.

Sorry, I did follow that for the last two patchsets (generalizing
Instant and hrtimer), but somehow I messed up with this one.

Should I resend v2 of this fix to the correct email address?

https://lore.kernel.org/lkml/20250502004524.230553-1-fujita.tomonori@gmail.com/

Re: [PATCH v1] rust: time: Avoid 64-bit integer division

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Mon, 05 May 2025 12:46:15 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>>
>>> Avoid 64-bit integer division that 32-bit architectures don't
>>> implement generally. This uses ktime_to_ms() and ktime_to_us()
>>> instead.
>>>
>>> The timer abstraction needs i64 / u32 division so C's div_s64() can be
>>> used but ktime_to_ms() and ktime_to_us() provide a simpler solution
>>> for this timer abstraction problem. On some architectures, there is
>>> room to optimize the implementation of them, but such optimization can
>>> be done if and when it becomes necessary.
>>>
>>> One downside of calling the C's functions is that the as_micros/millis
>>> methods can no longer be const fn. We stick with the simpler approach
>>> unless there's a compelling need for a const fn.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>>
>> Please consult recent MAINTAINERS file when you send patches. If you
>> intend for me to see a patch, please use my registered email address.
>
> Sorry, I did follow that for the last two patchsets (generalizing
> Instant and hrtimer), but somehow I messed up with this one.
>
> Should I resend v2 of this fix to the correct email address?
>
> https://lore.kernel.org/lkml/20250502004524.230553-1-fujita.tomonori@gmail.com/

No I got it from list, so it's fine for this one. But please stop CC'ing
the the Samsung address.


Best regards,
Andreas Hindborg